Add GovernorSequentialProposalId extension for sequential numbers on proposals (#5290)

Co-authored-by: Hadrien Croubois <[email protected]>
Co-authored-by: Ernesto García <[email protected]>

Author: 3 people

.changeset/brave-islands-sparkle.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`GovernorSequentialProposalId`: Adds a `Governor` extension that sequentially numbers proposal ids instead of using the hash.

.changeset/ten-fishes-fold.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`IGovernor`: Add the `getProposalId` function to the governor interface.

contracts/governance/Governor.sol

@@ -92,6 +92,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
     function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) {
         return
             interfaceId == type(IGovernor).interfaceId ||
+            interfaceId == type(IGovernor).interfaceId ^ IGovernor.getProposalId.selector ||
             interfaceId == type(IERC1155Receiver).interfaceId ||
             super.supportsInterface(interfaceId);
     }
@@ -132,6 +133,18 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
         return uint256(keccak256(abi.encode(targets, values, calldatas, descriptionHash)));
     }
 
+    /**
+     * @dev See {IGovernor-getProposalId}.
+     */
+    function getProposalId(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        bytes32 descriptionHash
+    ) public view virtual returns (uint256) {
+        return hashProposal(targets, values, calldatas, descriptionHash);
+    }
+
     /**
      * @dev See {IGovernor-state}.
      */
@@ -317,7 +330,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
         string memory description,
         address proposer
     ) internal virtual returns (uint256 proposalId) {
-        proposalId = hashProposal(targets, values, calldatas, keccak256(bytes(description)));
+        proposalId = getProposalId(targets, values, calldatas, keccak256(bytes(description)));
 
         if (targets.length != values.length || targets.length != calldatas.length || targets.length == 0) {
             revert GovernorInvalidProposalLength(targets.length, calldatas.length, values.length);
@@ -358,7 +371,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
         bytes[] memory calldatas,
         bytes32 descriptionHash
     ) public virtual returns (uint256) {
-        uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);
+        uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash);
 
         _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Succeeded));
 
@@ -406,7 +419,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
         bytes[] memory calldatas,
         bytes32 descriptionHash
     ) public payable virtual returns (uint256) {
-        uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);
+        uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash);
 
         _validateStateBitmap(
             proposalId,
@@ -468,8 +481,8 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
     ) public virtual returns (uint256) {
         // The proposalId will be recomputed in the `_cancel` call further down. However we need the value before we
         // do the internal call, because we need to check the proposal state BEFORE the internal `_cancel` call
-        // changes it. The `hashProposal` duplication has a cost that is limited, and that we accept.
-        uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);
+        // changes it. The `getProposalId` duplication has a cost that is limited, and that we accept.
+        uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash);
 
         // public cancel restrictions (on top of existing _cancel restrictions).
         _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Pending));
@@ -492,7 +505,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
         bytes[] memory calldatas,
         bytes32 descriptionHash
     ) internal virtual returns (uint256) {
-        uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);
+        uint256 proposalId = getProposalId(targets, values, calldatas, descriptionHash);
 
         _validateStateBitmap(
             proposalId,

contracts/governance/IGovernor.sol

@@ -203,7 +203,9 @@ interface IGovernor is IERC165, IERC6372 {
 
     /**
      * @notice module:core
-     * @dev Hashing function used to (re)build the proposal id from the proposal details..
+     * @dev Hashing function used to (re)build the proposal id from the proposal details.
+     *
+     * NOTE: For all off-chain and external calls, use {getProposalId}.
      */
     function hashProposal(
         address[] memory targets,
@@ -212,6 +214,17 @@ interface IGovernor is IERC165, IERC6372 {
         bytes32 descriptionHash
     ) external pure returns (uint256);
 
+    /**
+     * @notice module:core
+     * @dev Function used to get the proposal id from the proposal details.
+     */
+    function getProposalId(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        bytes32 descriptionHash
+    ) external view returns (uint256);
+
     /**
      * @notice module:core
      * @dev Current state of a proposal, following Compound's convention

contracts/governance/extensions/GovernorSequentialProposalId.sol

@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.20;
+
+import {Governor} from "../Governor.sol";
+
+/**
+ * @dev Extension of {Governor} that changes the numbering of proposal ids from the default hash-based approach to
+ * sequential ids.
+ */
+abstract contract GovernorSequentialProposalId is Governor {
+    uint256 private _latestProposalId;
+    mapping(uint256 proposalHash => uint256 proposalId) private _proposalIds;
+
+    /**
+     * @dev The {latestProposalId} may only be initialized if it hasn't been set yet
+     * (through initialization or the creation of a proposal).
+     */
+    error GovernorAlreadyInitializedLatestProposalId();
+
+    /**
+     * @dev See {IGovernor-getProposalId}.
+     */
+    function getProposalId(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        bytes32 descriptionHash
+    ) public view virtual override returns (uint256) {
+        uint256 proposalHash = hashProposal(targets, values, calldatas, descriptionHash);
+        uint256 storedProposalId = _proposalIds[proposalHash];
+        if (storedProposalId == 0) {
+            revert GovernorNonexistentProposal(0);
+        }
+        return storedProposalId;
+    }
+
+    /**
+     * @dev Returns the latest proposal id. A return value of 0 means no proposals have been created yet.
+     */
+    function latestProposalId() public view virtual returns (uint256) {
+        return _latestProposalId;
+    }
+
+    /**
+     * @dev See {IGovernor-_propose}.
+     * Hook into the proposing mechanism to increment proposal count.
+     */
+    function _propose(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        string memory description,
+        address proposer
+    ) internal virtual override returns (uint256) {
+        uint256 proposalHash = hashProposal(targets, values, calldatas, keccak256(bytes(description)));
+        uint256 storedProposalId = _proposalIds[proposalHash];
+        if (storedProposalId == 0) {
+            _proposalIds[proposalHash] = ++_latestProposalId;
+        }
+        return super._propose(targets, values, calldatas, description, proposer);
+    }
+
+    /**
+     * @dev Internal function to set the {latestProposalId}. This function is helpful when transitioning
+     * from another governance system. The next proposal id will be `newLatestProposalId` + 1.
+     *
+     * May only call this function if the current value of {latestProposalId} is 0.
+     */
+    function _initializeLatestProposalId(uint256 newLatestProposalId) internal virtual {
+        if (_latestProposalId != 0) {
+            revert GovernorAlreadyInitializedLatestProposalId();
+        }
+        _latestProposalId = newLatestProposalId;
+    }
+}

contracts/mocks/governance/GovernorSequentialProposalIdMock.sol

@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.20;
+
+import {Governor} from "../../governance/Governor.sol";
+import {GovernorSettings} from "../../governance/extensions/GovernorSettings.sol";
+import {GovernorCountingSimple} from "../../governance/extensions/GovernorCountingSimple.sol";
+import {GovernorVotesQuorumFraction} from "../../governance/extensions/GovernorVotesQuorumFraction.sol";
+import {GovernorSequentialProposalId} from "../../governance/extensions/GovernorSequentialProposalId.sol";
+
+abstract contract GovernorSequentialProposalIdMock is
+    GovernorSettings,
+    GovernorVotesQuorumFraction,
+    GovernorCountingSimple,
+    GovernorSequentialProposalId
+{
+    function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) {
+        return super.proposalThreshold();
+    }
+
+    function getProposalId(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        bytes32 descriptionHash
+    ) public view virtual override(Governor, GovernorSequentialProposalId) returns (uint256) {
+        return super.getProposalId(targets, values, calldatas, descriptionHash);
+    }
+
+    function _propose(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        string memory description,
+        address proposer
+    ) internal virtual override(Governor, GovernorSequentialProposalId) returns (uint256 proposalId) {
+        return super._propose(targets, values, calldatas, description, proposer);
+    }
+}

test/governance/Governor.test.js

@@ -96,7 +96,7 @@ describe('Governor', function () {
         );
       });
 
-      shouldSupportInterfaces(['ERC1155Receiver', 'Governor']);
+      shouldSupportInterfaces(['ERC1155Receiver', 'Governor', 'Governor_5_3']);
       shouldBehaveLikeERC6372(mode);
 
       it('deployment check', async function () {