use internal getProposalId function

OpenZeppelin · Nov 1, 2024 · 9063080 · 9063080
1 parent 3cf1b80
commit 9063080
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 18 deletions.
diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
@@ -110,6 +110,15 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
         return "1";
     }
 
+    function _getProposalId(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        bytes32 descriptionHash
+    ) internal virtual returns (uint256) {
+        return hashProposal(targets, values, calldatas, descriptionHash);
+    }
+
     /**
      * @dev See {IGovernor-hashProposal}.
      *
@@ -128,7 +137,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) public virtual returns (uint256) {
+    ) public pure virtual returns (uint256) {
         return uint256(keccak256(abi.encode(targets, values, calldatas, descriptionHash)));
     }
 
@@ -317,7 +326,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 +367,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 +415,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,
@@ -469,7 +478,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
         // 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);
+        uint256 proposalId = _getProposalId(targets, values, calldatas, descriptionHash);
 
         // public cancel restrictions (on top of existing _cancel restrictions).
         _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Pending));
@@ -492,7 +501,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,

diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol
@@ -210,7 +210,7 @@ interface IGovernor is IERC165, IERC6372 {
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) external returns (uint256);
+    ) external pure returns (uint256);
 
     /**
      * @notice module:core

diff --git a/contracts/governance/extensions/GovernorSequentialProposalId.sol b/contracts/governance/extensions/GovernorSequentialProposalId.sol
@@ -8,17 +8,15 @@ abstract contract GovernorSequentialProposalId is Governor {
     uint256 private _numberOfProposals;
     mapping(uint256 proposalHash => uint256 proposalId) private _proposalIds;
 
-    function hashProposal(
+    function _getProposalId(
         address[] memory targets,
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) public virtual override returns (uint256) {
-        uint256 proposalHash = super.hashProposal(targets, values, calldatas, descriptionHash);
+    ) internal virtual override returns (uint256) {
+        uint256 proposalHash = super._getProposalId(targets, values, calldatas, descriptionHash);
 
         uint256 storedProposalId = _proposalIds[proposalHash];
-        return storedProposalId == 0
-            ? (_proposalIds[proposalHash] = ++_numberOfProposals)
-            : storedProposalId;
+        return storedProposalId == 0 ? (_proposalIds[proposalHash] = ++_numberOfProposals) : storedProposalId;
     }
 }
diff --git a/contracts/mocks/governance/GovernorSequentialProposalIdMock.sol b/contracts/mocks/governance/GovernorSequentialProposalIdMock.sol
@@ -18,12 +18,12 @@ abstract contract GovernorSequentialProposalIdMock is
         return super.proposalThreshold();
     }
 
-    function hashProposal(
+    function _getProposalId(
         address[] memory targets,
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) public virtual override(Governor, GovernorSequentialProposalId) returns (uint256) {
-        return super.hashProposal(targets, values, calldatas, descriptionHash);
+    ) internal virtual override(Governor, GovernorSequentialProposalId) returns (uint256) {
+        return super._getProposalId(targets, values, calldatas, descriptionHash);
     }
 }
diff --git a/test/governance/extensions/GovernorSequentialProposalId.test.js b/test/governance/extensions/GovernorSequentialProposalId.test.js
@@ -137,7 +137,7 @@ describe('GovernorSequentialProposalId', function () {
       it('nominal workflow', async function () {
         await this.helper.connect(this.proposer).propose();
         let timepoint = await this.mock.proposalSnapshot(1);
-        time.increaseTo[mode](timepoint);
+        await time.increaseTo[mode](timepoint);
 
         await expect(this.mock.connect(this.voter1).castVote(1, VoteType.For))
           .to.emit(this.mock, 'VoteCast')
@@ -156,7 +156,7 @@ describe('GovernorSequentialProposalId', function () {
           .withArgs(this.voter4, 1, VoteType.Abstain, ethers.parseEther('2'), '');
 
         timepoint = await this.mock.proposalDeadline(1);
-        time.increaseTo[mode](timepoint);
+        await time.increaseTo[mode](timepoint);
 
         expect(this.helper.execute())
           .to.eventually.emit(this.mock, 'ProposalExecuted')