Add GovernorSequentialProposalId extension for sequential numbers on proposals (#5290)
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> Co-authored-by: Ernesto García <ernestognw@gmail.com>pull/5325/head^2
Arr00
1 month ago
committed by
GitHub
parent
3b240d7e6a
commit
03e06bf08c
@ -0,0 +1,5 @@ |
||||
--- |
||||
'openzeppelin-solidity': minor |
||||
--- |
||||
|
||||
`GovernorSequentialProposalId`: Adds a `Governor` extension that sequentially numbers proposal ids instead of using the hash. |
||||
@ -0,0 +1,5 @@ |
||||
--- |
||||
'openzeppelin-solidity': minor |
||||
--- |
||||
|
||||
`IGovernor`: Add the `getProposalId` function to the governor interface. |
||||
@ -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; |
||||
} |
||||
} |
||||
@ -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); |
||||
} |
||||
} |
||||
@ -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, |
||||
}, |
||||
], |
||||
'<proposal description>', |
||||
); |
||||
}); |
||||
|
||||
it('sequential proposal ids', async function () { |
||||
for (const i of iterate.range(1, 10)) { |
||||
this.proposal.description = `<proposal description #${i}>`; |
||||
|
||||
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 = `<proposal description #${i}>`; |
||||
|
||||
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'); |
||||
}); |
||||
}); |
||||
} |
||||
}); |
||||
Loading…
Reference in new issue