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.
I think the behavior we want is (no fallback)
return caller == proposalProposer(proposalId);There was a problem hiding this comment.
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.
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.
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.
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.
| // 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.
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.
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)