Add state getter in TimelockController using OperationState enum (#4358)

Co-authored-by: Francisco <fg@frang.io>
Co-authored-by: Ernesto García <ernestognw@gmail.com>

.changeset/loud-shrimps-play.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`TimelockController`: Add a state getter that returns an `OperationState` enum.

contracts/governance/README.adoc

@@ -52,8 +52,6 @@ NOTE: Functions of the `Governor` contract do not include access control. If you
 
 === Core
 
-{{IGovernor}}
-
 {{Governor}}
 
 === Modules
@@ -92,8 +90,9 @@ In a governance system, the {TimelockController} contract is in charge of introd
 * *Operation:* A transaction (or a set of transactions) that is the subject of the timelock. It has to be scheduled by a proposer and executed by an executor. The timelock enforces a minimum delay between the proposition and the execution (see xref:access-control.adoc#operation_lifecycle[operation lifecycle]). If the operation contains multiple transactions (batch mode), they are executed atomically. Operations are identified by the hash of their content.
 * *Operation status:*
 ** *Unset:* An operation that is not part of the timelock mechanism.
-** *Pending:* An operation that has been scheduled, before the timer expires.
+** *Waiting:* An operation that has been scheduled, before the timer expires.
 ** *Ready:* An operation that has been scheduled, after the timer expires.
+** *Pending:* An operation that is either waiting or ready.
 ** *Done:* An operation that has been executed.
 * *Predecessor*: An (optional) dependency between operations. An operation can depend on another operation (its predecessor), forcing the execution order of these two operations.
 * *Role*:

contracts/governance/TimelockController.sol

@@ -35,7 +35,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
 
     enum OperationState {
         Unset,
-        Pending,
+        Waiting,
         Ready,
         Done
     }
@@ -52,8 +52,12 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
 
     /**
      * @dev The current state of an operation is not as required.
+     * The `expectedStates` is a bitmap with the bits enabled for each OperationState enum position
+     * counting from right to left.
+     *
+     * See {_encodeStateBitmap}.
      */
-    error TimelockUnexpectedOperationState(bytes32 operationId, OperationState expected);
+    error TimelockUnexpectedOperationState(bytes32 operationId, bytes32 expectedStates);
 
     /**
      * @dev The predecessor to an operation not yet done.
@@ -166,30 +170,30 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
      * @dev Returns whether an id correspond to a registered operation. This
      * includes both Pending, Ready and Done operations.
      */
-    function isOperation(bytes32 id) public view virtual returns (bool) {
+    function isOperation(bytes32 id) public view returns (bool) {
-        return getTimestamp(id) > 0;
+        return getOperationState(id) != OperationState.Unset;
     }
 
     /**
      * @dev Returns whether an operation is pending or not. Note that a "pending" operation may also be "ready".
      */
-    function isOperationPending(bytes32 id) public view virtual returns (bool) {
+    function isOperationPending(bytes32 id) public view returns (bool) {
-        return getTimestamp(id) > _DONE_TIMESTAMP;
+        OperationState state = getOperationState(id);
+        return state == OperationState.Waiting || state == OperationState.Ready;
     }
 
     /**
      * @dev Returns whether an operation is ready for execution. Note that a "ready" operation is also "pending".
      */
-    function isOperationReady(bytes32 id) public view virtual returns (bool) {
+    function isOperationReady(bytes32 id) public view returns (bool) {
-        uint256 timestamp = getTimestamp(id);
+        return getOperationState(id) == OperationState.Ready;
-        return timestamp > _DONE_TIMESTAMP && timestamp <= block.timestamp;
     }
 
     /**
      * @dev Returns whether an operation is done or not.
      */
-    function isOperationDone(bytes32 id) public view virtual returns (bool) {
+    function isOperationDone(bytes32 id) public view returns (bool) {
-        return getTimestamp(id) == _DONE_TIMESTAMP;
+        return getOperationState(id) == OperationState.Done;
     }
 
     /**
@@ -200,6 +204,22 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
         return _timestamps[id];
     }
 
+    /**
+     * @dev Returns operation state.
+     */
+    function getOperationState(bytes32 id) public view virtual returns (OperationState) {
+        uint256 timestamp = getTimestamp(id);
+        if (timestamp == 0) {
+            return OperationState.Unset;
+        } else if (timestamp == _DONE_TIMESTAMP) {
+            return OperationState.Done;
+        } else if (timestamp > block.timestamp) {
+            return OperationState.Waiting;
+        } else {
+            return OperationState.Ready;
+        }
+    }
+
     /**
      * @dev Returns the minimum delay for an operation to become valid.
      *
@@ -298,7 +318,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
      */
     function _schedule(bytes32 id, uint256 delay) private {
         if (isOperation(id)) {
-            revert TimelockUnexpectedOperationState(id, OperationState.Unset);
+            revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Unset));
         }
         uint256 minDelay = getMinDelay();
         if (delay < minDelay) {
@@ -316,7 +336,10 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
      */
     function cancel(bytes32 id) public virtual onlyRole(CANCELLER_ROLE) {
         if (!isOperationPending(id)) {
-            revert TimelockUnexpectedOperationState(id, OperationState.Pending);
+            revert TimelockUnexpectedOperationState(
+                id,
+                _encodeStateBitmap(OperationState.Waiting) | _encodeStateBitmap(OperationState.Ready)
+            );
         }
         delete _timestamps[id];
 
@@ -399,7 +422,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
      */
     function _beforeCall(bytes32 id, bytes32 predecessor) private view {
         if (!isOperationReady(id)) {
-            revert TimelockUnexpectedOperationState(id, OperationState.Ready);
+            revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Ready));
         }
         if (predecessor != bytes32(0) && !isOperationDone(predecessor)) {
             revert TimelockUnexecutedPredecessor(predecessor);
@@ -411,7 +434,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
      */
     function _afterCall(bytes32 id) private {
         if (!isOperationReady(id)) {
-            revert TimelockUnexpectedOperationState(id, OperationState.Ready);
+            revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Ready));
         }
         _timestamps[id] = _DONE_TIMESTAMP;
     }
@@ -434,4 +457,19 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {
         emit MinDelayChange(_minDelay, newDelay);
         _minDelay = newDelay;
     }
+
+    /**
+     * @dev Encodes a `OperationState` into a `bytes32` representation where each bit enabled corresponds to
+     * the underlying position in the `OperationState` enum. For example:
+     *
+     * 0x000...1000
+     *   ^^^^^^----- ...
+     *         ^---- Done
+     *          ^--- Ready
+     *           ^-- Waiting
+     *            ^- Unset
+     */
+    function _encodeStateBitmap(OperationState operationState) internal pure returns (bytes32) {
+        return bytes32(1 << uint8(operationState));
+    }
 }

test/governance/TimelockController.test.js

@@ -1,5 +1,6 @@
 const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers');
 const { ZERO_ADDRESS, ZERO_BYTES32 } = constants;
+const { proposalStatesToBitMap } = require('../helpers/governance');
 
 const { expect } = require('chai');
 
@@ -195,7 +196,7 @@ contract('TimelockController', function (accounts) {
               { from: proposer },
             ),
             'TimelockUnexpectedOperationState',
-            [this.operation.id, OperationState.Unset],
+            [this.operation.id, proposalStatesToBitMap(OperationState.Unset)],
           );
         });
 
@@ -267,7 +268,7 @@ contract('TimelockController', function (accounts) {
               { from: executor },
             ),
             'TimelockUnexpectedOperationState',
-            [this.operation.id, OperationState.Ready],
+            [this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
           );
         });
 
@@ -295,7 +296,7 @@ contract('TimelockController', function (accounts) {
                 { from: executor },
               ),
               'TimelockUnexpectedOperationState',
-              [this.operation.id, OperationState.Ready],
+              [this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
             );
           });
 
@@ -313,7 +314,7 @@ contract('TimelockController', function (accounts) {
                 { from: executor },
               ),
               'TimelockUnexpectedOperationState',
-              [this.operation.id, OperationState.Ready],
+              [this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
             );
           });
 
@@ -408,7 +409,7 @@ contract('TimelockController', function (accounts) {
                   { from: executor },
                 ),
                 'TimelockUnexpectedOperationState',
-                [reentrantOperation.id, OperationState.Ready],
+                [reentrantOperation.id, proposalStatesToBitMap(OperationState.Ready)],
               );
 
               // Disable reentrancy
@@ -505,7 +506,7 @@ contract('TimelockController', function (accounts) {
               { from: proposer },
             ),
             'TimelockUnexpectedOperationState',
-            [this.operation.id, OperationState.Unset],
+            [this.operation.id, proposalStatesToBitMap(OperationState.Unset)],
           );
         });
 
@@ -596,7 +597,7 @@ contract('TimelockController', function (accounts) {
               { from: executor },
             ),
             'TimelockUnexpectedOperationState',
-            [this.operation.id, OperationState.Ready],
+            [this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
           );
         });
 
@@ -624,7 +625,7 @@ contract('TimelockController', function (accounts) {
                 { from: executor },
               ),
               'TimelockUnexpectedOperationState',
-              [this.operation.id, OperationState.Ready],
+              [this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
             );
           });
 
@@ -642,7 +643,7 @@ contract('TimelockController', function (accounts) {
                 { from: executor },
               ),
               'TimelockUnexpectedOperationState',
-              [this.operation.id, OperationState.Ready],
+              [this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
             );
           });
 
@@ -784,7 +785,7 @@ contract('TimelockController', function (accounts) {
                   { from: executor },
                 ),
                 'TimelockUnexpectedOperationState',
-                [reentrantBatchOperation.id, OperationState.Ready],
+                [reentrantBatchOperation.id, proposalStatesToBitMap(OperationState.Ready)],
               );
 
               // Disable reentrancy
@@ -881,7 +882,7 @@ contract('TimelockController', function (accounts) {
         await expectRevertCustomError(
           this.mock.cancel(constants.ZERO_BYTES32, { from: canceller }),
           'TimelockUnexpectedOperationState',
-          [constants.ZERO_BYTES32, OperationState.Pending],
+          [constants.ZERO_BYTES32, proposalStatesToBitMap([OperationState.Waiting, OperationState.Ready])],
         );
       });
 

test/governance/extensions/GovernorTimelockControl.test.js

@@ -159,7 +159,7 @@ contract('GovernorTimelockControl', function (accounts) {
 
             await expectRevertCustomError(this.helper.execute(), 'TimelockUnexpectedOperationState', [
               this.proposal.timelockid,
-              Enums.OperationState.Ready,
+              proposalStatesToBitMap(Enums.OperationState.Ready),
             ]);
           });
 
@@ -174,7 +174,7 @@ contract('GovernorTimelockControl', function (accounts) {
 
             await expectRevertCustomError(this.helper.execute(), 'TimelockUnexpectedOperationState', [
               this.proposal.timelockid,
-              Enums.OperationState.Ready,
+              proposalStatesToBitMap(Enums.OperationState.Ready),
             ]);
           });
 

test/helpers/enums.js

@@ -7,5 +7,5 @@ module.exports = {
   ProposalState: Enum('Pending', 'Active', 'Canceled', 'Defeated', 'Succeeded', 'Queued', 'Expired', 'Executed'),
   VoteType: Enum('Against', 'For', 'Abstain'),
   Rounding: Enum('Down', 'Up', 'Zero'),
-  OperationState: Enum('Unset', 'Pending', 'Ready', 'Done'),
+  OperationState: Enum('Unset', 'Waiting', 'Ready', 'Done'),
 };