diff --git a/.changeset/brave-islands-sparkle.md b/.changeset/brave-islands-sparkle.md new file mode 100644 index 00000000000..54cd6fb3a49 --- /dev/null +++ b/.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. diff --git a/.changeset/ten-fishes-fold.md b/.changeset/ten-fishes-fold.md new file mode 100644 index 00000000000..5c87ff36370 --- /dev/null +++ b/.changeset/ten-fishes-fold.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`IGovernor`: Add the `getProposalId` function to the governor interface. diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 390411556b0..f45aba096eb 100644 --- a/contracts/governance/Governor.sol +++ b/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, diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 28f8aaac044..36ef099a7d5 100644 --- a/contracts/governance/IGovernor.sol +++ b/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 diff --git a/contracts/governance/extensions/GovernorSequentialProposalId.sol b/contracts/governance/extensions/GovernorSequentialProposalId.sol new file mode 100644 index 00000000000..36e8698ba86 --- /dev/null +++ b/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; + } +} diff --git a/contracts/mocks/governance/GovernorSequentialProposalIdMock.sol b/contracts/mocks/governance/GovernorSequentialProposalIdMock.sol new file mode 100644 index 00000000000..882ee0b2483 --- /dev/null +++ b/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); + } +} diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 3e48ccfee7f..ea1c3a32457 100644 --- a/test/governance/Governor.test.js +++ b/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 () { diff --git a/test/governance/extensions/GovernorSequentialProposalId.test.js b/test/governance/extensions/GovernorSequentialProposalId.test.js new file mode 100644 index 00000000000..0fb7fb269b3 --- /dev/null +++ b/test/governance/extensions/GovernorSequentialProposalId.test.js @@ -0,0 +1,202 @@ +const { ethers } = require('hardhat'); +const { expect } = require('chai'); +const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); +const { anyValue } = require('@nomicfoundation/hardhat-chai-matchers/withArgs'); + +const { GovernorHelper } = require('../../helpers/governance'); +const { VoteType } = require('../../helpers/enums'); +const iterate = require('../../helpers/iterate'); + +const TOKENS = [ + { Token: '$ERC20Votes', mode: 'blocknumber' }, + { Token: '$ERC20VotesTimestampMock', mode: 'timestamp' }, +]; + +const name = 'OZ-Governor'; +const version = '1'; +const tokenName = 'MockToken'; +const tokenSymbol = 'MTKN'; +const tokenSupply = ethers.parseEther('100'); +const votingDelay = 4n; +const votingPeriod = 16n; +const value = ethers.parseEther('1'); + +async function deployToken(contractName) { + try { + return await ethers.deployContract(contractName, [tokenName, tokenSymbol, tokenName, version]); + } catch (error) { + if (error.message == 'incorrect number of arguments to constructor') { + // ERC20VotesLegacyMock has a different construction that uses version='1' by default. + return ethers.deployContract(contractName, [tokenName, tokenSymbol, tokenName]); + } + throw error; + } +} + +describe('GovernorSequentialProposalId', function () { + for (const { Token, mode } of TOKENS) { + const fixture = async () => { + const [owner, proposer, voter1, voter2, voter3, voter4, userEOA] = await ethers.getSigners(); + const receiver = await ethers.deployContract('CallReceiverMock'); + + const token = await deployToken(Token, [tokenName, tokenSymbol, version]); + const mock = await ethers.deployContract('$GovernorSequentialProposalIdMock', [ + name, // name + votingDelay, // initialVotingDelay + votingPeriod, // initialVotingPeriod + 0n, // initialProposalThreshold + token, // tokenAddress + 10n, // quorumNumeratorValue + ]); + + await owner.sendTransaction({ to: mock, value }); + await token.$_mint(owner, tokenSupply); + + const helper = new GovernorHelper(mock, mode); + await helper.connect(owner).delegate({ token: token, to: voter1, value: ethers.parseEther('10') }); + await helper.connect(owner).delegate({ token: token, to: voter2, value: ethers.parseEther('7') }); + await helper.connect(owner).delegate({ token: token, to: voter3, value: ethers.parseEther('5') }); + await helper.connect(owner).delegate({ token: token, to: voter4, value: ethers.parseEther('2') }); + + return { + owner, + proposer, + voter1, + voter2, + voter3, + voter4, + userEOA, + receiver, + token, + mock, + helper, + }; + }; + + describe(`using ${Token}`, function () { + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + + this.proposal = this.helper.setProposal( + [ + { + target: this.receiver.target, + data: this.receiver.interface.encodeFunctionData('mockFunction'), + value, + }, + ], + '', + ); + }); + + it('sequential proposal ids', async function () { + for (const i of iterate.range(1, 10)) { + this.proposal.description = ``; + + expect(this.mock.hashProposal(...this.proposal.shortProposal)).to.eventually.equal(this.proposal.hash); + await expect(this.mock.getProposalId(...this.proposal.shortProposal)).revertedWithCustomError( + this.mock, + 'GovernorNonexistentProposal', + ); + expect(this.mock.latestProposalId()).to.eventually.equal(i - 1); + + await expect(this.helper.connect(this.proposer).propose()) + .to.emit(this.mock, 'ProposalCreated') + .withArgs( + i, + this.proposer, + this.proposal.targets, + this.proposal.values, + this.proposal.signatures, + this.proposal.data, + anyValue, + anyValue, + this.proposal.description, + ); + + expect(this.mock.hashProposal(...this.proposal.shortProposal)).to.eventually.equal(this.proposal.hash); + expect(this.mock.getProposalId(...this.proposal.shortProposal)).to.eventually.equal(i); + expect(this.mock.latestProposalId()).to.eventually.equal(i); + } + }); + + it('sequential proposal ids with offset start', async function () { + const offset = 69420; + await this.mock.$_initializeLatestProposalId(offset); + + for (const i of iterate.range(offset + 1, offset + 10)) { + this.proposal.description = ``; + + expect(this.mock.hashProposal(...this.proposal.shortProposal)).to.eventually.equal(this.proposal.hash); + await expect(this.mock.getProposalId(...this.proposal.shortProposal)).revertedWithCustomError( + this.mock, + 'GovernorNonexistentProposal', + ); + expect(this.mock.latestProposalId()).to.eventually.equal(i - 1); + + await expect(this.helper.connect(this.proposer).propose()) + .to.emit(this.mock, 'ProposalCreated') + .withArgs( + i, + this.proposer, + this.proposal.targets, + this.proposal.values, + this.proposal.signatures, + this.proposal.data, + anyValue, + anyValue, + this.proposal.description, + ); + + expect(this.mock.hashProposal(...this.proposal.shortProposal)).to.eventually.equal(this.proposal.hash); + expect(this.mock.getProposalId(...this.proposal.shortProposal)).to.eventually.equal(i); + expect(this.mock.latestProposalId()).to.eventually.equal(i); + } + }); + + it('can only initialize latest proposal id from 0', async function () { + await this.helper.propose(); + expect(this.mock.latestProposalId()).to.eventually.equal(1); + await expect(this.mock.$_initializeLatestProposalId(2)).to.be.revertedWithCustomError( + this.mock, + 'GovernorAlreadyInitializedLatestProposalId', + ); + }); + + it('cannot repropose same proposal', async function () { + await this.helper.connect(this.proposer).propose(); + await expect(this.helper.connect(this.proposer).propose()) + .to.be.revertedWithCustomError(this.mock, 'GovernorUnexpectedProposalState') + .withArgs(await this.proposal.id, 0, ethers.ZeroHash); + }); + + it('nominal workflow', async function () { + await this.helper.connect(this.proposer).propose(); + await this.helper.waitForSnapshot(); + + await expect(this.mock.connect(this.voter1).castVote(1, VoteType.For)) + .to.emit(this.mock, 'VoteCast') + .withArgs(this.voter1, 1, VoteType.For, ethers.parseEther('10'), ''); + + await expect(this.mock.connect(this.voter2).castVote(1, VoteType.For)) + .to.emit(this.mock, 'VoteCast') + .withArgs(this.voter2, 1, VoteType.For, ethers.parseEther('7'), ''); + + await expect(this.mock.connect(this.voter3).castVote(1, VoteType.For)) + .to.emit(this.mock, 'VoteCast') + .withArgs(this.voter3, 1, VoteType.For, ethers.parseEther('5'), ''); + + await expect(this.mock.connect(this.voter4).castVote(1, VoteType.Abstain)) + .to.emit(this.mock, 'VoteCast') + .withArgs(this.voter4, 1, VoteType.Abstain, ethers.parseEther('2'), ''); + + await this.helper.waitForDeadline(); + + expect(this.helper.execute()) + .to.eventually.emit(this.mock, 'ProposalExecuted') + .withArgs(1) + .emit(this.receiver, 'MockFunctionCalled'); + }); + }); + } +}); diff --git a/test/helpers/governance.js b/test/helpers/governance.js index 540967af49d..e0686445c28 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -35,12 +35,16 @@ class GovernorHelper { return this; } - get id() { + get hash() { return ethers.keccak256( ethers.AbiCoder.defaultAbiCoder().encode(['address[]', 'uint256[]', 'bytes[]', 'bytes32'], this.shortProposal), ); } + get id() { + return this.governor.latestProposalId ? this.governor.getProposalId(...this.shortProposal) : this.hash; + } + // used for checking events get signatures() { return this.data.map(() => ''); @@ -106,10 +110,10 @@ class GovernorHelper { async vote(vote = {}) { let method = 'castVote'; // default - let args = [this.id, vote.support]; // base + let args = [await this.id, vote.support]; // base if (vote.signature) { - const sign = await vote.signature(this.governor, this.forgeMessage(vote)); + const sign = await this.forgeMessage(vote).then(msg => vote.signature(this.governor, msg)); if (vote.params || vote.reason) { method = 'castVoteWithReasonAndParamsBySig'; args.push(vote.voter, vote.reason ?? '', vote.params ?? '0x', sign); @@ -130,14 +134,12 @@ class GovernorHelper { async overrideVote(vote = {}) { let method = 'castOverrideVote'; - let args = [this.id, vote.support]; + let args = [await this.id, vote.support]; vote.reason = vote.reason ?? ''; if (vote.signature) { - let message = this.forgeMessage(vote); - message.reason = message.reason ?? ''; - const sign = await vote.signature(this.governor, message); + const sign = await this.forgeMessage(vote).then(msg => vote.signature(this.governor, { reason: '', ...msg })); method = 'castOverrideVoteBySig'; args.push(vote.voter, vote.reason ?? '', sign); } @@ -147,23 +149,23 @@ class GovernorHelper { /// Clock helpers async waitForSnapshot(offset = 0n) { - const timepoint = await this.governor.proposalSnapshot(this.id); + const timepoint = await this.governor.proposalSnapshot(await this.id); return time.increaseTo[this.mode](timepoint + offset); } async waitForDeadline(offset = 0n) { - const timepoint = await this.governor.proposalDeadline(this.id); + const timepoint = await this.governor.proposalDeadline(await this.id); return time.increaseTo[this.mode](timepoint + offset); } async waitForEta(offset = 0n) { - const timestamp = await this.governor.proposalEta(this.id); + const timestamp = await this.governor.proposalEta(await this.id); return time.increaseTo.timestamp(timestamp + offset); } /// Other helpers - forgeMessage(vote = {}) { - const message = { proposalId: this.id, support: vote.support, voter: vote.voter, nonce: vote.nonce }; + async forgeMessage(vote = {}) { + const message = { proposalId: await this.id, support: vote.support, voter: vote.voter, nonce: vote.nonce }; if (vote.params || vote.reason) { message.reason = vote.reason ?? ''; diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 51948279c50..bfcddee7a5e 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -3,6 +3,34 @@ const { interfaceId } = require('../../helpers/methods'); const { mapValues } = require('../../helpers/iterate'); const INVALID_ID = '0xffffffff'; +const GOVERNOR_INTERFACE = [ + 'name()', + 'version()', + 'COUNTING_MODE()', + 'hashProposal(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)', +]; const SIGNATURES = { ERC165: ['supportsInterface(bytes4)'], ERC721: [ @@ -59,34 +87,8 @@ const SIGNATURES = { 'acceptDefaultAdminTransfer()', 'cancelDefaultAdminTransfer()', ], - Governor: [ - 'name()', - 'version()', - 'COUNTING_MODE()', - 'hashProposal(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)', - ], + Governor: GOVERNOR_INTERFACE, + Governor_5_3: GOVERNOR_INTERFACE.concat('getProposalId(address[],uint256[],bytes[],bytes32)'), ERC2981: ['royaltyInfo(uint256,uint256)'], };