bool → eta

.changeset/brave-lobsters-punch.md

@@ -2,4 +2,4 @@
 'openzeppelin-solidity': major
 ---
 
-`Governor`: Add a public `queue` and an internal `_queue` function in the governor core. Module that implemement queuing are expected to override the internal one.
+`Governor`: Add a public `queue` and an internal `_queue` function in the governor core. Module that implement queuing are expected to override the internal one.

contracts/governance/Governor.sol

@@ -96,27 +96,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
      * @dev See {IERC165-supportsInterface}.
      */
     function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) {
-        bytes4 governorCancelId = this.cancel.selector ^ this.proposalProposer.selector;
-
-        bytes4 governorParamsId = this.castVoteWithReasonAndParams.selector ^
-            this.castVoteWithReasonAndParamsBySig.selector ^
-            this.getVotesWithParams.selector;
-
-        // The original interface id in v4.3.
-        bytes4 governor43Id = type(IGovernor).interfaceId ^
-            type(IERC6372).interfaceId ^
-            governorCancelId ^
-            governorParamsId;
-
-        // An updated interface id in v4.6, with params added.
-        bytes4 governor46Id = type(IGovernor).interfaceId ^ type(IERC6372).interfaceId ^ governorCancelId;
-
-        // For the updated interface id in v4.9, we use governorCancelId directly.
-
         return
-            interfaceId == governor43Id ||
-            interfaceId == governor46Id ||
-            interfaceId == governorCancelId ||
+            interfaceId == type(IGovernor).interfaceId ||
             interfaceId == type(IERC1155Receiver).interfaceId ||
             super.supportsInterface(interfaceId);
     }
@@ -321,19 +302,22 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
     }
 
     /**
-     * @dev See {IGovernorTimelock-queue}.
+     * @dev See {IGovernor-queue}.
      */
     function queue(
         address[] memory targets,
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) public virtual returns (uint256 proposalId) {
+    ) public virtual override returns (uint256 proposalId) {
         proposalId = hashProposal(targets, values, calldatas, descriptionHash);
         _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Succeeded));
 
-        if (!_queue(proposalId, targets, values, calldatas, descriptionHash)) {
+        uint256 eta = _queue(proposalId, targets, values, calldatas, descriptionHash);
+        if (eta == 0) {
             revert GovernorQueueNotImplemented();
+        } else {
+            emit ProposalQueued(proposalId, eta);
         }
     }
 
@@ -432,20 +416,20 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
     }
 
     /**
-     * @dev Internal queuing mechanism. This is empty by default, and must be overriden to implement queuing.
+     * @dev Internal queuing mechanism. This is empty by default, and must be overridden to implement queuing.
      *
-     * This function returns a boolean that describing weither a queuing logic is implemented or not. This function
-     * returns false by default, which causes the public {queue} to revert. When overriding this, the overriden
-     * version should discard the value returned by the super call and return true instead.
+     * This function returns a timestamp that describes the expected eta for execution. If the returned value is 0
+     * (which is the default value), the core will consider queueing to not be implemented, and the public {queue}
+     * function will revert.
      */
     function _queue(
         uint256 /* proposalId */,
         address[] memory /* targets */,
         uint256[] memory /* values */,
         bytes[] memory /* calldatas */,
         bytes32 /*descriptionHash*/
-    ) internal virtual returns (bool enabled) {
-        return false;
+    ) internal virtual returns (uint256 eta) {
+        return 0;
     }
 
     /**

contracts/governance/IGovernor.sol

@@ -101,15 +101,20 @@ abstract contract IGovernor is IERC165, IERC6372 {
     );
 
     /**
-     * @dev Emitted when a proposal is canceled.
+     * @dev Emitted when a proposal is queued.
      */
-    event ProposalCanceled(uint256 proposalId);
+    event ProposalQueued(uint256 proposalId, uint256 eta);
 
     /**
      * @dev Emitted when a proposal is executed.
      */
     event ProposalExecuted(uint256 proposalId);
 
+    /**
+     * @dev Emitted when a proposal is canceled.
+     */
+    event ProposalCanceled(uint256 proposalId);
+
     /**
      * @dev Emitted when a vote is cast without params.
      *
@@ -295,9 +300,24 @@ abstract contract IGovernor is IERC165, IERC6372 {
     ) public virtual returns (uint256 proposalId);
 
     /**
-     * @dev Execute a successful proposal. This requires the quorum to be reached, the vote to be successful, and the
+     * @dev Queue a proposal. Some governors require this step to be performed before execution can happens. Some
+     * governors do not. Queuing a proposal requires the quorum to be reached, the vote to be successful, and the
      * deadline to be reached.
      *
+     * Emits a {ProposalQueued} event.
+     */
+    function queue(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        bytes32 descriptionHash
+    ) public virtual returns (uint256 proposalId);
+
+    /**
+     * @dev Execute a successful proposal. This requires the quorum to be reached, the vote to be successful, and the
+     * deadline to be reached. Depending on the governor it might also be required that the proposal was queued and
+     * that some delay passed.
+     *
      * Emits a {ProposalExecuted} event.
      *
      * Note: some module can modify the requirements for execution, for example by adding an additional timelock.

contracts/governance/extensions/GovernorTimelockCompound.sol

@@ -80,18 +80,6 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor {
         return _proposalEta[proposalId];
     }
 
-    /**
-     * @dev {IGovernor-queue} override resolution
-     */
-    function queue(
-        address[] memory targets,
-        uint256[] memory values,
-        bytes[] memory calldatas,
-        bytes32 descriptionHash
-    ) public virtual override(IGovernorTimelock, Governor) returns (uint256) {
-        return super.queue(targets, values, calldatas, descriptionHash);
-    }
-
     /**
      * @dev Function to queue a proposal to the timelock.
      */
@@ -101,10 +89,12 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor {
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) internal virtual override returns (bool) {
-        super._queue(proposalId, targets, values, calldatas, descriptionHash);
+    ) internal virtual override returns (uint256) {
+        uint256 eta = Math.max(
+            super._queue(proposalId, targets, values, calldatas, descriptionHash),
+            block.timestamp + _timelock.delay()
+        );
 
-        uint256 eta = block.timestamp + _timelock.delay();
         _proposalEta[proposalId] = eta;
 
         for (uint256 i = 0; i < targets.length; ++i) {
@@ -114,9 +104,7 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor {
             _timelock.queueTransaction(targets[i], values[i], "", calldatas[i], eta);
         }
 
-        emit ProposalQueued(proposalId, eta);
-
-        return true;
+        return eta;
     }
 
     /**

contracts/governance/extensions/GovernorTimelockControl.sol

@@ -86,18 +86,6 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor {
         return eta == 1 ? 0 : eta; // _DONE_TIMESTAMP (1) should be replaced with a 0 value
     }
 
-    /**
-     * @dev {IGovernor-queue} override resolution
-     */
-    function queue(
-        address[] memory targets,
-        uint256[] memory values,
-        bytes[] memory calldatas,
-        bytes32 descriptionHash
-    ) public virtual override(IGovernorTimelock, Governor) returns (uint256) {
-        return super.queue(targets, values, calldatas, descriptionHash);
-    }
-
     /**
      * @dev Function to queue a proposal to the timelock.
      */
@@ -107,16 +95,14 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor {
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) internal virtual override returns (bool) {
-        super._queue(proposalId, targets, values, calldatas, descriptionHash);
+    ) internal virtual override returns (uint256) {
+        uint256 superEta = super._queue(proposalId, targets, values, calldatas, descriptionHash);
 
         uint256 delay = _timelock.getMinDelay();
         _timelockIds[proposalId] = _timelock.hashOperationBatch(targets, values, calldatas, 0, descriptionHash);
         _timelock.scheduleBatch(targets, values, calldatas, 0, descriptionHash, delay);
 
-        emit ProposalQueued(proposalId, block.timestamp + delay);
-
-        return true;
+        return Math.max(superEta, block.timestamp + delay);
     }
 
     /**

contracts/governance/extensions/IGovernorTimelock.sol

@@ -21,16 +21,7 @@ abstract contract IGovernorTimelock is IGovernor {
      */
     error GovernorAlreadyQueuedProposal(uint256 proposalId);
 
-    event ProposalQueued(uint256 proposalId, uint256 eta);
-
     function timelock() public view virtual returns (address);
 
     function proposalEta(uint256 proposalId) public view virtual returns (uint256);
-
-    function queue(
-        address[] memory targets,
-        uint256[] memory values,
-        bytes[] memory calldatas,
-        bytes32 descriptionHash
-    ) public virtual returns (uint256 proposalId);
 }

contracts/mocks/docs/governance/MyGovernor.sol

@@ -46,15 +46,6 @@ contract MyGovernor is
         return super.propose(targets, values, calldatas, description);
     }
 
-    function queue(
-        address[] memory targets,
-        uint256[] memory values,
-        bytes[] memory calldatas,
-        bytes32 descriptionHash
-    ) public override(Governor, GovernorTimelockControl) returns (uint256) {
-        return super.queue(targets, values, calldatas, descriptionHash);
-    }
-
     function cancel(
         address[] memory targets,
         uint256[] memory values,
@@ -70,7 +61,7 @@ contract MyGovernor is
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) internal override(Governor, GovernorTimelockControl) returns (bool enabled) {
+    ) internal override(Governor, GovernorTimelockControl) returns (uint256) {
         return super._queue(proposalId, targets, values, calldatas, descriptionHash);
     }
 

contracts/mocks/governance/GovernorTimelockCompoundMock.sol

@@ -35,22 +35,13 @@ abstract contract GovernorTimelockCompoundMock is
         return super.proposalThreshold();
     }
 
-    function queue(
-        address[] memory targets,
-        uint256[] memory values,
-        bytes[] memory calldatas,
-        bytes32 descriptionHash
-    ) public override(Governor, GovernorTimelockCompound) returns (uint256) {
-        return super.queue(targets, values, calldatas, descriptionHash);
-    }
-
     function _queue(
         uint256 proposalId,
         address[] memory targets,
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) internal override(Governor, GovernorTimelockCompound) returns (bool enabled) {
+    ) internal override(Governor, GovernorTimelockCompound) returns (uint256) {
         return super._queue(proposalId, targets, values, calldatas, descriptionHash);
     }
 

contracts/mocks/governance/GovernorTimelockControlMock.sol

@@ -33,22 +33,13 @@ abstract contract GovernorTimelockControlMock is
         return super.proposalThreshold();
     }
 
-    function queue(
-        address[] memory targets,
-        uint256[] memory values,
-        bytes[] memory calldatas,
-        bytes32 descriptionHash
-    ) public override(Governor, GovernorTimelockControl) returns (uint256) {
-        return super.queue(targets, values, calldatas, descriptionHash);
-    }
-
     function _queue(
         uint256 proposalId,
         address[] memory targets,
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) internal override(Governor, GovernorTimelockControl) returns (bool enabled) {
+    ) internal override(Governor, GovernorTimelockControl) returns (uint256) {
         return super._queue(proposalId, targets, values, calldatas, descriptionHash);
     }
 

test/governance/Governor.test.js

@@ -79,7 +79,7 @@ contract('Governor', function (accounts) {
         );
       });
 
-      shouldSupportInterfaces(['ERC165', 'ERC1155Receiver', 'Governor', 'GovernorWithParams', 'GovernorCancel']);
+      shouldSupportInterfaces(['ERC165', 'ERC1155Receiver', 'Governor']);
       shouldBehaveLikeEIP6372(mode);
 
       it('deployment check', async function () {

test/governance/extensions/GovernorTimelockCompound.test.js

@@ -86,7 +86,7 @@ contract('GovernorTimelockCompound', function (accounts) {
         );
       });
 
-      shouldSupportInterfaces(['ERC165', 'Governor', 'GovernorWithParams', 'GovernorTimelock']);
+      shouldSupportInterfaces(['ERC165', 'Governor', 'GovernorTimelock']);
 
       it("doesn't accept ether transfers", async function () {
         await expectRevert.unspecified(web3.eth.sendTransaction({ from: owner, to: this.mock.address, value: 1 }));

test/governance/extensions/GovernorTimelockControl.test.js

@@ -93,7 +93,7 @@ contract('GovernorTimelockControl', function (accounts) {
         );
       });
 
-      shouldSupportInterfaces(['ERC165', 'Governor', 'GovernorWithParams', 'GovernorTimelock']);
+      shouldSupportInterfaces(['ERC165', 'Governor', 'GovernorTimelock']);
 
       it("doesn't accept ether transfers", async function () {
         await expectRevert.unspecified(web3.eth.sendTransaction({ from: owner, to: this.mock.address, value: 1 }));

test/utils/introspection/SupportsInterface.behavior.js

@@ -52,46 +52,31 @@ const INTERFACES = {
   Governor: [
     'name()',
     'version()',
+    'clock()', // part of ERC-6372, but redeclared as public
+    'CLOCK_MODE()', // part of ERC-6372, but redeclared as public
     'COUNTING_MODE()',
     'hashProposal(address[],uint256[],bytes[],bytes32)',
     'state(uint256)',
     'proposalSnapshot(uint256)',
     'proposalDeadline(uint256)',
-    'votingDelay()',
-    'votingPeriod()',
-    'quorum(uint256)',
-    'getVotes(address,uint256)',
-    'hasVoted(uint256,address)',
-    'propose(address[],uint256[],bytes[],string)',
-    'execute(address[],uint256[],bytes[],bytes32)',
-    'castVote(uint256,uint8)',
-    'castVoteWithReason(uint256,uint8,string)',
-    'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)',
-  ],
-  GovernorWithParams: [
-    'name()',
-    'version()',
-    'COUNTING_MODE()',
-    'hashProposal(address[],uint256[],bytes[],bytes32)',
-    'state(uint256)',
-    'proposalSnapshot(uint256)',
-    'proposalDeadline(uint256)',
+    'proposalProposer(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,uint8,bytes32,bytes32)',
     'castVoteWithReasonAndParamsBySig(uint256,uint8,string,bytes,uint8,bytes32,bytes32)',
   ],
-  GovernorCancel: ['proposalProposer(uint256)', 'cancel(address[],uint256[],bytes[],bytes32)'],
-  GovernorTimelock: ['timelock()', 'proposalEta(uint256)', 'queue(address[],uint256[],bytes[],bytes32)'],
+  GovernorTimelock: ['timelock()', 'proposalEta(uint256)'],
   ERC2981: ['royaltyInfo(uint256,uint256)'],
 };