Governor Sequential Proposal id #5280
Conversation
|
|
For reference, here is a rough diff of what is required if we do not want to change the mutability to write... and this still requires internal vars. The approach implemented here seems like the best option. |
contracts/governance/Governor.sol
Outdated
| @@ -128,7 +128,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 | |||
| uint256[] memory values, | |||
| bytes[] memory calldatas, | |||
| bytes32 descriptionHash | |||
| ) public pure virtual returns (uint256) { | |||
| ) public virtual returns (uint256) { | |||
There was a problem hiding this comment.
Not even view ... I get why its needed, but wow that is a change of behavior !
contracts/governance/extensions/GovernorSequentialProposalId.sol
Outdated
Show resolved
Hide resolved
Co-authored-by: Hadrien Croubois <[email protected]>
arr00
changed the title
| * @dev Internal function to set the sequential proposal ID for the next proposal. This is helpful for transitioning from another governing system. | ||
| */ | ||
| function _setNextProposalId(uint256 nextProposalId) internal virtual { | ||
| if (_numberOfProposals != 0) revert NextProposalIdCanOnlyBeSetOnce(); |
There was a problem hiding this comment.
IMO this works, but there's a slightly annoying attack surface here. if an attacker can create a proposal on this governor before the upgrade occurs, then any setNextProposalId call will revert. this setNextProposalId call might be part of the upgrade proposal, which would then fail.
we had the same problem. we managed it by initializing nextProposalId to type(uint256).max. then, proposals couldn't be created unless setNextProposalId had been called.
There was a problem hiding this comment.
Given that the default flow would be proposal ids starting from 1, we don't want to require calls to _setNextProposalId. Also note that _setNextProposalId is an internal function which would need to be called in a bespoke public function, which is where this issue should be addressed. We should add another function to this extension getNextProposalId to make disabling governance until the proposal id is set easier.
| uint256 proposalHash = super._getProposalId(targets, values, calldatas, descriptionHash); | ||
|
|
||
| uint256 storedProposalId = _proposalIds[proposalHash]; | ||
| return storedProposalId == 0 ? (_proposalIds[proposalHash] = ++_numberOfProposals) : storedProposalId; |
There was a problem hiding this comment.
My idea was that this function should be a view getter, it shouldn't automatically generate a new proposal id (propose would do that explicitly), and it should revert if there is no proposal id stored (because the proposal was never created).
Governance extension that enables sequential proposal ids. This implementation requires making
hashProposala write function which may not be acceptable.PR Checklist
npx changeset add)