-
Notifications
You must be signed in to change notification settings - Fork 9
use _validateCancel instead of overriding cancel
#10
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
use _validateCancel instead of overriding cancel
#10
Conversation
446b171 to
13f18c5
Compare
| address proposer = proposalProposer(proposalId); | ||
| if (caller != proposer) revert GovernorOnlyProposer(caller); | ||
| return _cancel(targets, values, calldatas, descriptionHash); | ||
| if (caller == proposer) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the behavior we want is (no fallback)
return caller == proposalProposer(proposalId);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and if we want to make do things differently, then I would make it explicit:
return caller == proposalProposer(proposalId) || super._validateCancel(proposalId, caller);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The powerful aspect of this method of validation is that additional overrides can be explicitly more permissive. So calling super in all cases that it would be false allows for it to play nice with another possibly more permissive cancelation policy (say anyone can cancel a proposal with very low participation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can understand the logic, but then it has to be documented accordingly.
Currently the documentation says:
if there is no proposal guardian only the proposer can cancel
But that is forgetting that the super call could add additional "permission".
Note that there is a case to make that in certain conditions, we could want to activelly disable permission. For example there would be a module that has a flag disableAllCancel
function _validateCancel(uint256 proposalId, address caller) internal view virtual override returns (bool) {
reuturn !disableAllCancel && super._validateCancel(proposalId, caller);In that case you don't want the super call to say yes when disableAllCancel says "no way". Not saying that is what we want here, but it should be discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the docs accordingly. If we are going in the right direction, we should merge this PR and have further conversation on the main PR?
|
|
||
| // if there is no guardian and the caller isn't the proposer or | ||
| // there is a proposal guardian, and the caller is not the proposal guardian | ||
| // ... apply default behavior | ||
| return super._validateCancel(proposalId, caller); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // if there is no guardian and the caller isn't the proposer or | |
| // there is a proposal guardian, and the caller is not the proposal guardian | |
| // ... apply default behavior | |
| return super._validateCancel(proposalId, caller); | |
| } else { | |
| // if there is no guardian and the caller isn't the proposer or | |
| // there is a proposal guardian, and the caller is not the proposal guardian | |
| // ... apply default behavior | |
| return super._validateCancel(proposalId, caller); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach here was to not require having two separate calls to super. If its required for readability we can do it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a consequence of this discussion: https://github.com/Amxx/openzeppelin-contracts/pull/10/files#r1925693063.
(let keep it to one thread)
e65a38a to
ce162a6
Compare
Fixes #????
PR Checklist
npx changeset add)