Governor: Sequential Proposal Ids #5290

arr00 · 2024-11-01T07:59:38Z

Alternative solution to #5280

PR Checklist

Tests
Documentation
Changeset entry (run npx changeset add)

Co-authored-by: Hadrien Croubois <[email protected]>

changeset-bot · 2024-11-01T07:59:41Z

🦋 Changeset detected

Latest commit: a04b47f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

contracts/governance/extensions/GovernorSequentialProposalId.sol

contracts/governance/Governor.sol

contracts/governance/extensions/GovernorSequentialProposalId.sol

test/helpers/governance.js

contracts/governance/extensions/GovernorSequentialProposalId.sol

frangio · 2024-11-22T22:24:00Z

contracts/governance/extensions/GovernorSequentialProposalId.sol

+    }
+
+    /**
+     * @dev Internal function to set the sequential proposal ID for the next proposal. This is helpful for transitioning from another governing system.


I would add a big caveat about decreasing the proposal count. I'd consider enforcing it in a require statement too, not sure if there are significant downsides to that.

The use-case that @arr00 metionned that someone may set the count to type(uint256).max to (temporarily) disable proposing until the correct number is set.

IMO that usecase (prevent any proposal from being submitted could be more cleanly done using the proposal threshold).

This is the require I proposed: #5290 (comment)

Would like to get @wildmolasses thoughts here. I'm leaning towards pushing the above approach of just using the proposal threshold.

Yeah I'd keep that use case separate. But I do think it's a concern if setting the count to the max uint permanently blocks the governor from functioning... So this is why I'm not all in on enforcing that the update is an increase. A safer thing would be to allow increases of some reasonable size (like uint32), it seems arbitrary but I think it works.

ernestognw · 2024-11-29T19:33:21Z

contracts/governance/IGovernor.sol

@@ -212,6 +212,16 @@ interface IGovernor is IERC165, IERC6372 {
        bytes32 descriptionHash
    ) external pure returns (uint256);

+    /**
+     * @dev Function used to get the proposal id from the proposal details


Suggested change

* @dev Function used to get the proposal id from the proposal details

* @dev Function used to get the proposal id from the proposal details.

ernestognw · 2024-11-29T19:37:48Z

.changeset/brave-islands-sparkle.md

I think we're missing another check to mention the new getProposalId function

ernestognw · 2024-11-29T19:39:09Z

contracts/governance/extensions/GovernorSequentialProposalId.sol

+        _proposalCount = newProposalCount;
+    }
+
+    function proposalCount() public view virtual returns (uint256) {


We need to document this getter. Also, I'd put it up before the _propose function

ernestognw · 2024-11-29T19:41:05Z

contracts/governance/extensions/GovernorSequentialProposalId.sol

+    }
+
+    /**
+     * @dev Internal function to set the current proposal count--the next proposal id will be `newProposalCount` + 1.


I don't understand this sentence.

Suggested change

* @dev Internal function to set the current proposal count--the next proposal id will be `newProposalCount` + 1.

* @dev Internal function to set the current proposal count--the next proposal id will be `newProposalCount` + 1.

ernestognw · 2024-11-29T19:42:35Z

contracts/governance/extensions/GovernorSequentialProposalId.sol

+     *
+     * May only call this function if the current proposal count is 0.
+     */
+    function _setProposalCount(uint256 newProposalCount) internal virtual {


I assume the purpose of including this function is to allow DAOs that are upgrading to set an initial proposal count in their initializers. If this is the case, I think this should be documented somewhere

ernestognw · 2024-11-29T19:42:49Z

contracts/governance/extensions/GovernorSequentialProposalId.sol

+
+import {Governor} from "../Governor.sol";
+
+abstract contract GovernorSequentialProposalId is Governor {


We're missing NatSpec here

ernestognw · 2024-11-29T19:43:08Z

contracts/governance/extensions/GovernorSequentialProposalId.sol

+        return storedProposalId;
+    }
+
+    function _propose(


Missing NatSpec here too

ernestognw · 2024-11-29T19:46:50Z

contracts/governance/extensions/GovernorSequentialProposalId.sol

+    /**
+     * @dev The proposal count may only be set if the current proposal count is 0 in {_setProposalCount}.
+     */
+    error GovernorCanNotSetProposalCount();


I'd rename it "uninitialized" since it better indicates what's wrong. Also, it goes well with the change I'm proposing to rename {_setProposalCount} to {_initializeProposalCount}. I think that instead of explaining the error literally, we should give a hint to solve it.

Suggested change

/**

* @dev The proposal count may only be set if the current proposal count is 0 in {_setProposalCount}.

*/

error GovernorCanNotSetProposalCount();

/**

* @dev The proposal count may only be set if the current proposal count is uninitialized.

*/

error GovernorUninitializedProposalCount();

ernestognw · 2024-11-29T19:47:37Z

contracts/governance/extensions/GovernorSequentialProposalId.sol

+     *
+     * May only call this function if the current proposal count is 0.
+     */
+    function _setProposalCount(uint256 newProposalCount) internal virtual {


I would be in favor of changing this function's name to indicate better what to do with it. In this case, it suggests developers that it should be used within an initializer.

Suggested change

function _setProposalCount(uint256 newProposalCount) internal virtual {

function _initializeProposalCount(uint256 newProposalCount) internal virtual {

ernestognw · 2024-11-29T19:50:20Z

test/utils/introspection/SupportsInterface.behavior.js

+  Governor_5_3: [
+    'name()',
+    'version()',
+    'COUNTING_MODE()',
+    'hashProposal(address[],uint256[],bytes[],bytes32)',
+    'getProposalId(address[],uint256[],bytes[],bytes32)',
+    'state(uint256)',
+    'proposalThreshold()',
+    'proposalSnapshot(uint256)',
+    'proposalDeadline(uint256)',
+    'proposalProposer(uint256)',
+    'proposalEta(uint256)',
+    'proposalNeedsQueuing(uint256)',
+    'votingDelay()',
+    'votingPeriod()',
+    'quorum(uint256)',
+    'getVotes(address,uint256)',
+    'getVotesWithParams(address,uint256,bytes)',
+    'hasVoted(uint256,address)',
+    'propose(address[],uint256[],bytes[],string)',
+    'queue(address[],uint256[],bytes[],bytes32)',
+    'execute(address[],uint256[],bytes[],bytes32)',
+    'cancel(address[],uint256[],bytes[],bytes32)',
+    'castVote(uint256,uint8)',
+    'castVoteWithReason(uint256,uint8,string)',
+    'castVoteWithReasonAndParams(uint256,uint8,string,bytes)',
+    'castVoteBySig(uint256,uint8,address,bytes)',
+    'castVoteWithReasonAndParamsBySig(uint256,uint8,address,string,bytes,bytes)',
+  ],


I'm not really a fan of repeating all this interface, I'd look for a way to reuse it easily

arr00 and others added 6 commits October 25, 2024 10:59

feat: governor sequential proposal id

a25d139

use super hash

a4ee283

Make numberOfProposals private

e34a8b9

Update contracts/governance/extensions/GovernorSequentialProposalId.sol

3cf1b80

Co-authored-by: Hadrien Croubois <[email protected]>

use internal getProposalId function

9063080

hash proposal is view

8755aa6

ernestognw reviewed Nov 1, 2024

View reviewed changes

contracts/governance/extensions/GovernorSequentialProposalId.sol Outdated Show resolved Hide resolved

ernestognw mentioned this pull request Nov 1, 2024

Update hashProposal visibility to view #5097

Open

3 tasks

arr00 added 4 commits November 4, 2024 09:53

Add _setNextProposalId helper func

c52f001

Revert if next proposal id set and current proposal not 0

d9f3477

Add getNextProposalId func

b063a61

Add view function getProposalId

19dfdf8

arr00 mentioned this pull request Nov 18, 2024

Governor Sequential Proposal id #5280

Closed

3 tasks

arr00 changed the title ~~Sequential Proposal Ids Alt~~ Governor: Sequential Proposal Ids Nov 18, 2024

Set proposal count instead of next

185b6a3

arr00 requested a review from ernestognw November 18, 2024 08:26

Amxx reviewed Nov 18, 2024

View reviewed changes

contracts/governance/extensions/GovernorSequentialProposalId.sol Outdated Show resolved Hide resolved

Amxx reviewed Nov 18, 2024

View reviewed changes

contracts/governance/extensions/GovernorSequentialProposalId.sol Outdated Show resolved Hide resolved

tests

cb91e76

Amxx added this to the 5.3 milestone Nov 18, 2024

Amxx added the area: governance label Nov 18, 2024

revert when calling getProposalId before propose

5ff56da

Amxx reviewed Nov 18, 2024

View reviewed changes

contracts/governance/Governor.sol Show resolved Hide resolved

frangio reviewed Nov 18, 2024

View reviewed changes

contracts/governance/extensions/GovernorSequentialProposalId.sol Outdated Show resolved Hide resolved

Rename getProposalCount to proposalCount

f0e9fb5

Amxx reviewed Nov 19, 2024

View reviewed changes

test/helpers/governance.js Outdated Show resolved Hide resolved

Amxx and others added 2 commits November 19, 2024 10:54

Update test/helpers/governance.js

becc77a

fix test

8ffad72

Amxx mentioned this pull request Nov 19, 2024

Add a governor extension that implements a security council #5303

Open

3 tasks

add patch

3e49fb9

arr00 requested review from frangio and Amxx November 22, 2024 16:24

frangio reviewed Nov 22, 2024

View reviewed changes

contracts/governance/extensions/GovernorSequentialProposalId.sol Outdated Show resolved Hide resolved

frangio reviewed Nov 22, 2024

View reviewed changes

arr00 and others added 2 commits November 22, 2024 17:57

fix repropose bug

41d49c4

interface Id

f8cfe02

OpenZeppelin deleted a comment from 19714 Nov 25, 2024

Amxx and others added 7 commits November 25, 2024 15:01

code style uniformity

76d57a2

require proposal count to be strictly increasing

74bdb35

Merge branch 'master' into feat/sequential-proposal-id-alt-2

3206080

update comment

c174090

only allow setting the proposal count from 0

9786e76

update docs

df711fe

fix test

a04b47f

ernestognw reviewed Nov 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Governor: Sequential Proposal Ids #5290

Governor: Sequential Proposal Ids #5290

arr00 commented Nov 1, 2024 •

edited

Loading

changeset-bot bot commented Nov 1, 2024 •

edited

Loading

frangio Nov 22, 2024

Amxx Nov 25, 2024

arr00 Nov 25, 2024

frangio Nov 26, 2024

ernestognw Nov 29, 2024

ernestognw Nov 29, 2024

ernestognw Nov 29, 2024

ernestognw Nov 29, 2024

ernestognw Nov 29, 2024

ernestognw Nov 29, 2024

ernestognw Nov 29, 2024

ernestognw Nov 29, 2024

ernestognw Nov 29, 2024

ernestognw Nov 29, 2024

	* @dev Function used to get the proposal id from the proposal details
	* @dev Function used to get the proposal id from the proposal details.

	* @dev Internal function to set the current proposal count--the next proposal id will be `newProposalCount` + 1.
	* @dev Internal function to set the current proposal count--the next proposal id will be `newProposalCount` + 1.


		import {Governor} from "../Governor.sol";

		abstract contract GovernorSequentialProposalId is Governor {

	function _setProposalCount(uint256 newProposalCount) internal virtual {
	function _initializeProposalCount(uint256 newProposalCount) internal virtual {

Governor: Sequential Proposal Ids #5290

Are you sure you want to change the base?

Governor: Sequential Proposal Ids #5290

Conversation

arr00 commented Nov 1, 2024 • edited Loading

PR Checklist

changeset-bot bot commented Nov 1, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arr00 commented Nov 1, 2024 •

edited

Loading

changeset-bot bot commented Nov 1, 2024 •

edited

Loading