Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Governor #417

Merged
merged 22 commits into from
Dec 12, 2024
Merged

Conversation

ericnordelo
Copy link
Member

@ericnordelo ericnordelo commented Dec 3, 2024

This PR adds Governor to Cairo, and also refactors some code. Summary below:

Added:

  • section member to Impl and ImplementedTrait interfaces allowing to group in sections under a comment.
  • embed member to Impl interface replacing the internalImpl mechanism when adding components since there could be multiple internal impls as shown in Governor.
  • addUseClause logic replacing the way imports were handled, allowing us to group imports.
  • Governor tab.

This PR is still missing:

  • To refactor the existing tests and add new ones.
  • Use macro for pow instead of the 2e18 notation (this macro must be implemented first).

With this said, it is ready for a first round of reviews since the main logic is finished.

@ericnordelo ericnordelo marked this pull request as ready for review December 5, 2024 14:26
@ericnordelo
Copy link
Member Author

@ericglau having the deploy preview for this first round of reviews would be very helpful.

Copy link
Contributor

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start, Eric! I left some comments and suggestions

packages/core-cairo/src/print.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/print.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/governor.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/governor.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/governor.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/governor.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add InternalExtendedImpl for assert_only_governance to be in scope for upgrades. We also need to import (and the option to set) ImmutableConfig

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Added both InternalExtendedImpl and DefaultConfig. I don't think we should add the option for configuring DEFAULT_PARAMS on wizard though, since the format is Span<felt252> (most probably a serialized struct).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should add the option for configuring DEFAULT_PARAMS on wizard though, since the format is Span (most probably a serialized struct).

Hmm I'm wondering if we should say more about DEFAULT_PARAMS in the docs (or in a future guide). IMO it's not obvious what this should be and how it should be used. I agree though in that it's prob not an option we need in wizard

packages/ui/src/cairo/GovernorControls.svelte Show resolved Hide resolved
packages/core-cairo/src/governor.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/governor.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/print.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/print.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/governor.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/print.ts Show resolved Hide resolved
packages/core-cairo/src/print.ts Show resolved Hide resolved
@ericnordelo
Copy link
Member Author

Added an addUseClause logic replacing the addStandaloneImport one. This allows us to optionally group imports.

Copy link
Member

@ericglau ericglau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good, thanks! Just some minor comments.

packages/core-cairo/src/contract.ts Outdated Show resolved Hide resolved
packages/core-cairo/src/print.ts Show resolved Hide resolved
packages/core-cairo/src/print.ts Outdated Show resolved Hide resolved
@ericnordelo ericnordelo requested a review from ericglau December 11, 2024 13:45
Copy link
Member

@ericglau ericglau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

Copy link
Contributor

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improvements look great! I just left a non-blocking question

packages/ui/src/cairo/GovernorControls.svelte Show resolved Hide resolved
@ericnordelo
Copy link
Member Author

I'm having issues building the test_project locally (the process exits after a couple of hours of running). Do you guys have a strategy for this? I'm on an M3 MacBook Pro 😢. cc @andrew-fleming @immrsd. Also, @ericglau should I add an entry to the CHANGELOG? If that is so, should it be under Unreleased? I'm not familiar with the wizard release process

@ericglau
Copy link
Member

@ericnordelo Can you add to the changelog entries under 0.20.0? I haven't published that version yet so we might as well release these together. We will be looking into a more formal release process going forward.

@andrew-fleming
Copy link
Contributor

I'm having issues building the test_project locally (the process exits after a couple of hours of running). Do you guys have a strategy for this? I'm on an M3 MacBook Pro 😢

Ahh hours!? I unfortunately don't have a strat for this :( I'm on an m2 and the last PR took "only" 30 min to compile

@ericglau
Copy link
Member

I'm having issues building the test_project locally (the process exits after a couple of hours of running).

Compile time seems non-linearly increasing for each added contract. Perhaps try removing the other contract types other than Governor when testing it locally. That might be sufficient for now.

@ericglau
Copy link
Member

ericglau commented Dec 11, 2024

We could also change the script to generate sources for each contract kind separately, into different projects. There there would be like 6 projects that need to be compiled. That should cut down each project's compile time though. (Does not need to be in this PR)

@ericnordelo
Copy link
Member Author

ericnordelo commented Dec 12, 2024

Compile time seems non-linearly increasing for each added contract. Perhaps try removing the other contract types other than Governor when testing it locally. That might be sufficient for now.

With only Governor sources it compiled in two minutes!

Update:

  • ERC20 compiled in 3 minutes.
  • Account in 66 seconds.

@ericnordelo ericnordelo merged commit 355d098 into OpenZeppelin:master Dec 12, 2024
9 checks passed
@ericnordelo ericnordelo deleted the feat/add-governor-#415 branch December 16, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants