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

💥 Refactor TimelockController Using Stateful Modules #202

Closed

Conversation

charles-cooper
Copy link
Contributor

uses modules from vyper 0.4.0
this is kind of a demonstration of the new feature
rename some interface files to *.vyi. (i was not thorough, i just did
enough to get TimelockController.vy to compile)

🐶 Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

uses modules from vyper 0.4.0
this is kind of a demonstration of the new feature
rename some interface files to `*.vyi`. (i was not thorough, i just did
    enough to get TimelockController.vy to compile)
@pcaversaccio pcaversaccio changed the title refactor TimelockController.vy to use vyper 0.4.0 modules 💥 Refactor TimelockController Using Stateful Modules Feb 5, 2024
@pcaversaccio pcaversaccio added documentation 📖 Improvements or additions to documentation feature 💥 New feature or request labels Feb 5, 2024
@pcaversaccio
Copy link
Owner

image

That's so cool! I thought we wanted to get rid of .vyi files now. Once vyperlang/vyper#3729 is merged, we should adjust the CI pipeline here (it's easier once merged than now), and ensure all tests pass.

# Set the optional admin.
if (admin_ != empty(address)):
self._grant_role(DEFAULT_ADMIN_ROLE, admin_)
acl.__init__()
Copy link
Owner

@pcaversaccio pcaversaccio Feb 5, 2024

Choose a reason for hiding this comment

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

As a note for later: since I currently allocate ADDITIONAL_ROLE_1 and ADDITIONAL_ROLE_2 at construction-time (see here), this will also allocate these additional roles if called acl.__init__. We should refactor AccessControl by only allocating DEFAULT_ADMIN_ROLE and let the user set up further roles.

Copy link
Owner

Choose a reason for hiding this comment

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

I think for this line:

if (admin_ != empty(address)):
    self._grant_role(DEFAULT_ADMIN_ROLE, admin_)

We could use

if (admin_ != empty(address)):
    acl._grant_role(DEFAULT_ADMIN_ROLE, admin_)

# the following Twitter thread:
# https://twitter.com/pcaversaccio/status/1626514029094047747.
enum OperationState:
flag OperationState:
Copy link
Owner

Choose a reason for hiding this comment

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

why do you delete my comment here lol? flags still behave like the description ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i assumed it wasn't needed anymore since the naming is no longer a point of confusion -- i can add it back though

charles-cooper and others added 2 commits February 12, 2024 18:59
Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
@pcaversaccio
Copy link
Owner

pcaversaccio commented Feb 13, 2024

For testing purposes, this PR now runs against Vyper's master branch and only tests TimelockController (69d25d3, 46ad334), and it looks like it's raising correctly here 😄:

2024-02-13T09:14:49.446621Z ERROR cheatcodes: non-empty stderr input=["vyper", "src/governance/TimelockController.vy"] stderr="vyper.exceptions.InitializerException: module `src/auth/AccessControl.vy` is used but never initialized!\n\n  (hint: add `initializes: acl` to the top level of your main contract)\n\n  contract \"src/governance/TimelockController.vy:54\", line 54:0 \n       53\n  ---> 54 uses: acl\n  --------^\n       55\n\n"

pcaversaccio and others added 2 commits February 13, 2024 10:11
Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
@pcaversaccio
Copy link
Owner

Closing in favour of #207.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📖 Improvements or additions to documentation feature 💥 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants