Delay the Pending state until strictly after proposal.voteStart (#2892)

Co-authored-by: Francisco Giordano <[email protected]>

Author: Amxx

CHANGELOG.md

@@ -7,6 +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`: 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)
 

contracts/governance/Governor.sol

@@ -115,9 +115,9 @@ 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) {
             return ProposalState.Pending;
-        } else if (proposal.voteEnd.isPending()) {
+        } else if (proposal.voteEnd.getDeadline() >= block.number) {
             return ProposalState.Active;
         } else if (proposal.voteEnd.isExpired()) {
             return

contracts/governance/IGovernor.sol

@@ -103,28 +103,31 @@ 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 starts 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);
 
     /**
      * @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);

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();

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