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 a canceller role to the TimelockController #3165

Merged

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Feb 3, 2022

Fixes #2980

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx force-pushed the fix/TimelockController/CancellorRole branch from 78c09a8 to 91b1f1f Compare February 3, 2022 16:14
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

"Cancellor" is a typo, it should be "canceller".

In the changelog, we should explain that if an instance is upgraded the role needs to be set up (admin and members).

contracts/governance/TimelockController.sol Outdated Show resolved Hide resolved
@Amxx
Copy link
Collaborator Author

Amxx commented Feb 3, 2022

AFAIK, both cancellor and canceller are valid. One is American english, the other is British english.

@frangio
Copy link
Contributor

frangio commented Feb 3, 2022

No, you may be confusing it with "chancellor"? "Cancellor" doesn't seem to exist at all.

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 4, 2022

I did put "Cancellor" in my translator, and got what I expected.

I'm going to change it just to be safe.

(and no, I'm not confusing chancellor Palpatine with cancellor Palpatine :P)

@frangio frangio changed the title Add a cancellor role to the TimelockController Add a canceller role to the TimelockController Feb 16, 2022
@frangio
Copy link
Contributor

frangio commented Feb 16, 2022

I would expect the tests to set up different proposer and canceller accounts, so we can separately test them. Note: it also makes sense to test that in the default constructor the proposer array is assigned canceller roles.

@frangio frangio marked this pull request as ready for review February 21, 2022 21:15
@frangio
Copy link
Contributor

frangio commented Feb 21, 2022

This is good but needs a changelog entry.

@frangio frangio merged commit 8b162e3 into OpenZeppelin:master Mar 8, 2022
@Amxx Amxx deleted the fix/TimelockController/CancellorRole branch March 9, 2022 08:35
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.

Add CANCEL_ROLE to TimelockController
2 participants