From e4157c96f85d53914c58efef47878f9d6bf07046 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 30 Sep 2021 23:32:54 +0200 Subject: [PATCH 1/9] Delay the Pending state until strictly after proposal.voteStart to avoid issue with potential invalid implementations of the voting token --- contracts/governance/Governor.sol | 3 ++- test/governance/Governor.test.js | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 741e02f7d98..84967801b8a 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -115,7 +115,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { return ProposalState.Executed; } else if (proposal.canceled) { return ProposalState.Canceled; - } else if (proposal.voteStart.isPending()) { + } else if (proposal.voteStart.getDeadline() >= block.number) { + // prevent voting until voteStart is passed, so that getVotes never tries to get value for the current block. return ProposalState.Pending; } else if (proposal.voteEnd.isPending()) { return ProposalState.Active; diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index ed58eea571d..da3e0f1414f 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -559,6 +559,10 @@ contract('Governor', function (accounts) { await time.advanceBlockTo(this.snapshot); + expect(await this.mock.state(this.id)).to.be.bignumber.equal(Enums.ProposalState.Pending); + + await time.advanceBlock(); + expect(await this.mock.state(this.id)).to.be.bignumber.equal(Enums.ProposalState.Active); }); runGovernorWorkflow(); From 5b5a6b8c99792f15cbff9ed9f827b53ed4d11f72 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 4 Oct 2021 19:50:06 +0200 Subject: [PATCH 2/9] change voteEnd detection to match bravo --- contracts/governance/Governor.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 84967801b8a..f1411ffa220 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -116,9 +116,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { } else if (proposal.canceled) { return ProposalState.Canceled; } else if (proposal.voteStart.getDeadline() >= block.number) { - // prevent voting until voteStart is passed, so that getVotes never tries to get value for the current block. return ProposalState.Pending; - } else if (proposal.voteEnd.isPending()) { + } else if (proposal.voteEnd.getDeadline() >= block.number) { return ProposalState.Active; } else if (proposal.voteEnd.isExpired()) { return From bc6b88b28e502b7fa7ae60b1d3ee246991fd875b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 5 Oct 2021 16:19:59 +0200 Subject: [PATCH 3/9] fix governor behavior --- contracts/governance/extensions/GovernorTimelockCompound.sol | 3 +-- test/governance/GovernorWorkflow.behavior.js | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index 415a52b998c..e28951f17fc 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -47,8 +47,7 @@ interface ICompoundTimelock { address target, uint256 value, string memory signature, - bytes memory data, - uint256 eta + bytes memory data,' ) external; function executeTransaction( diff --git a/test/governance/GovernorWorkflow.behavior.js b/test/governance/GovernorWorkflow.behavior.js index 433a4e6a2fb..70319cd44d3 100644 --- a/test/governance/GovernorWorkflow.behavior.js +++ b/test/governance/GovernorWorkflow.behavior.js @@ -58,7 +58,7 @@ function runGovernorWorkflow () { tryGet(this.settings, 'steps.propose.error') === undefined && tryGet(this.settings, 'steps.propose.noadvance') !== true ) { - await time.advanceBlockTo(this.snapshot); + await time.advanceBlockTo(this.snapshot.addn(1)); } } @@ -92,7 +92,7 @@ function runGovernorWorkflow () { // fast forward if (tryGet(this.settings, 'steps.wait.enable') !== false) { - await time.advanceBlockTo(this.deadline); + await time.advanceBlockTo(this.deadline.addn(1)); } // queue From 7f12df8401e3fceba2cf1160465f2d67ba699bfc Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 5 Oct 2021 16:21:37 +0200 Subject: [PATCH 4/9] fix typo --- contracts/governance/extensions/GovernorTimelockCompound.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index e28951f17fc..415a52b998c 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -47,7 +47,8 @@ interface ICompoundTimelock { address target, uint256 value, string memory signature, - bytes memory data,' + bytes memory data, + uint256 eta ) external; function executeTransaction( From a3cb90113a82d0e5a341a429842a0806e69caab2 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 6 Oct 2021 10:27:45 -0300 Subject: [PATCH 5/9] force github action From 26bca3b07c79d24d2ebae8ff7f8950bd28f04b28 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 6 Oct 2021 21:47:37 +0200 Subject: [PATCH 6/9] changelog and doc --- CHANGELOG.md | 1 + contracts/governance/IGovernor.sol | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 607d864b75e..f3f46a086d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ * `Ownable`: add an internal `_transferOwnership(address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568)) * `AccessControl`: add internal `_grantRole(bytes32,address)` and `_revokeRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568)) * `AccessControl`: mark `_setupRole(bytes32,address)` as deprecated in favor of `_grantRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568)) + * `Governor`: sligtly change proposal snapshot and deadline mechanism to better match Compound's governor bravo and prevent voting (at the governor level) if the voting snapshot is not ready. ([#2892](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2892)) ## 4.3.2 (2021-09-14) diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index ee5180dfc2d..8d8da977ff6 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -103,13 +103,16 @@ abstract contract IGovernor is IERC165 { /** * @notice module:core - * @dev block number used to retrieve user's votes and quorum. + * @dev block number used to retrieve user's votes and quorum. As per Compound's Comp and OpenZeppelin's + * ERC20Votes, the snapshot is performed at the end of this block. Hence, voting for this proposal start at the + * beginning of the following block. */ function proposalSnapshot(uint256 proposalId) public view virtual returns (uint256); /** * @notice module:core - * @dev timestamp at which votes close. + * @dev block number at which votes close. Votes close at the end of this block, so it is possible to cast a vote + * during this block. */ function proposalDeadline(uint256 proposalId) public view virtual returns (uint256); From 246b6999632aef424e37c7575eff2e3684d8064d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 6 Oct 2021 21:49:23 +0200 Subject: [PATCH 7/9] working in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3f46a086d9..02862a71d70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ * `Ownable`: add an internal `_transferOwnership(address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568)) * `AccessControl`: add internal `_grantRole(bytes32,address)` and `_revokeRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568)) * `AccessControl`: mark `_setupRole(bytes32,address)` as deprecated in favor of `_grantRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568)) - * `Governor`: sligtly change proposal snapshot and deadline mechanism to better match Compound's governor bravo and prevent voting (at the governor level) if the voting snapshot is not ready. ([#2892](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2892)) + * `Governor`: sligtly change proposals' snapshot and deadline mechanisms to better match Compound's governor bravo and prevent voting (at the governor level) if the voting snapshot is not ready. ([#2892](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2892)) ## 4.3.2 (2021-09-14) From 624f31089a4b55046770a209eec66ce11a4ad45e Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 6 Oct 2021 16:56:11 -0300 Subject: [PATCH 8/9] capitalization and typos --- contracts/governance/IGovernor.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 8d8da977ff6..90f4549395b 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -103,31 +103,31 @@ abstract contract IGovernor is IERC165 { /** * @notice module:core - * @dev block number used to retrieve user's votes and quorum. As per Compound's Comp and OpenZeppelin's - * ERC20Votes, the snapshot is performed at the end of this block. Hence, voting for this proposal start at the + * @dev Block number used to retrieve user's votes and quorum. As per Compound's Comp and OpenZeppelin's + * ERC20Votes, the snapshot is performed at the end of this block. Hence, voting for this proposal starts at the * beginning of the following block. */ function proposalSnapshot(uint256 proposalId) public view virtual returns (uint256); /** * @notice module:core - * @dev block number at which votes close. Votes close at the end of this block, so it is possible to cast a vote + * @dev Block number at which votes close. Votes close at the end of this block, so it is possible to cast a vote * during this block. */ function proposalDeadline(uint256 proposalId) public view virtual returns (uint256); /** * @notice module:user-config - * @dev delay, in number of block, between the proposal is created and the vote starts. This can be increassed to + * @dev Delay, in number of block, between the proposal is created and the vote starts. This can be increassed to * leave time for users to buy voting power, of delegate it, before the voting of a proposal starts. */ function votingDelay() public view virtual returns (uint256); /** * @notice module:user-config - * @dev delay, in number of blocks, between the vote start and vote ends. + * @dev Delay, in number of blocks, between the vote start and vote ends. * - * Note: the {votingDelay} can delay the start of the vote. This must be considered when setting the voting + * NOTE: The {votingDelay} can delay the start of the vote. This must be considered when setting the voting * duration compared to the voting delay. */ function votingPeriod() public view virtual returns (uint256); From 0e1d4a060bf6c8e37552dd3df7c96429e86235cd Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 6 Oct 2021 17:06:39 -0300 Subject: [PATCH 9/9] reword changelog entry for clarity --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f13cb43a95..eddab7cc487 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ * `AccessControl`: mark `_setupRole(bytes32,address)` as deprecated in favor of `_grantRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568)) * `EIP712`: cache `address(this)` to immutable storage to avoid potential issues if a vanilla contract is used in a delegatecall context. ([#2852](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2852)) * Add internal `_setApprovalForAll` to `ERC721` and `ERC1155`. ([#2834](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2834)) - * `Governor`: slightly change proposals' snapshot and deadline mechanisms to better match Compound's governor bravo and prevent voting (at the governor level) if the voting snapshot is not ready. ([#2892](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2892)) + * `Governor`: shift vote start and end by one block to better match Compound's GovernorBravo and prevent voting at the Governor level if the voting snapshot is not ready. ([#2892](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2892)) ## 4.3.2 (2021-09-14)