Add Governor module connecting with AccessManager

contracts/access/manager/AccessManager.sol

@@ -89,7 +89,14 @@ contract AccessManager is Context, Multicall, IAccessManager {
     mapping(address target => AccessMode mode) private _contractMode;
     mapping(uint64 classId => Class) private _classes;
     mapping(uint64 groupId => Group) private _groups;
-    mapping(bytes32 operationId => uint48 schedule) private _schedules;
+
+    struct Schedule {
+        uint48 timepoint;
+        uint32 nonce;
+    }
+
+    mapping(bytes32 operationId => Schedule) private _schedules;
+
     mapping(bytes4 selector => Time.Delay delay) private _adminDelays;
 
     // This should be transcient storage when supported by the EVM.
@@ -568,44 +575,57 @@ contract AccessManager is Context, Multicall, IAccessManager {
      * operation is not yet scheduled, has expired, was executed, or was canceled.
      */
     function getSchedule(bytes32 id) public view virtual returns (uint48) {
-        uint48 timepoint = _schedules[id];
+        uint48 timepoint = _schedules[id].timepoint;
         return _isExpired(timepoint) ? 0 : timepoint;
     }
 
+    /**
+     * @dev Return the nonce for the latest scheduled operation with a given id. Returns 0 if the operation has never
+     * been scheduled.
+     */
+    function getNonce(bytes32 id) public view virtual returns (uint32) {
+        return _schedules[id].nonce;
+    }
+
     /**
      * @dev Schedule a delayed operation for future execution, and return the operation identifier. It is possible to
      * choose the timestamp at which the operation becomes executable as long as it satisfies the execution delays
      * required for the caller. The special value zero will automatically set the earliest possible time.
      *
      * Emits a {OperationScheduled} event.
      */
-    function schedule(address target, bytes calldata data, uint48 when) public virtual returns (bytes32) {
+    function schedule(address target, bytes calldata data, uint48 when) public virtual returns (bytes32 operationId, uint32 nonce) {
         address caller = _msgSender();
 
         // Fetch restriction to that apply to the caller on the targeted function
         (bool allowed, uint32 setback) = _canCallExtended(caller, target, data);
 
         uint48 minWhen = Time.timestamp() + setback;
 
+        if (when == 0) {
+            when = minWhen;
+        }
+
         // If caller is not authorised, revert
-        if (!allowed && (setback == 0 || when.isSetAndPast(minWhen - 1))) {
+        if (!allowed && (setback == 0 || when < minWhen)) {
             revert AccessManagerUnauthorizedCall(caller, target, bytes4(data[0:4]));
         }
 
         // If caller is authorised, schedule operation
-        bytes32 operationId = _hashOperation(caller, target, data);
+        operationId = _hashOperation(caller, target, data);
 
         // Cannot reschedule unless the operation has expired
-        uint48 prevTimepoint = _schedules[operationId];
+        uint48 prevTimepoint = _schedules[operationId].timepoint;
         if (prevTimepoint != 0 && !_isExpired(prevTimepoint)) {
             revert AccessManagerAlreadyScheduled(operationId);
         }
 
-        uint48 timepoint = when == 0 ? minWhen : when;
-        _schedules[operationId] = timepoint;
-        emit OperationScheduled(operationId, timepoint, caller, target, data);
+        nonce = _schedules[operationId].nonce + 1;
+        _schedules[operationId].timepoint = when;
+        _schedules[operationId].nonce = nonce;
+        emit OperationScheduled(operationId, nonce, when, caller, target, data);
 
-        return operationId;
+        // Using named return values because otherwise we get stack too deep
     }
 
     /**
@@ -617,7 +637,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
     // Reentrancy is not an issue because permissions are checked on msg.sender. Additionally,
     // _consumeScheduledOp guarantees a scheduled operation is only executed once.
     // slither-disable-next-line reentrancy-no-eth
-    function relay(address target, bytes calldata data) public payable virtual {
+    function relay(address target, bytes calldata data) public payable virtual returns (uint32) {
         address caller = _msgSender();
 
         // Fetch restriction to that apply to the caller on the targeted function
@@ -630,9 +650,10 @@ contract AccessManager is Context, Multicall, IAccessManager {
 
         // If caller is authorised, check operation was scheduled early enough
         bytes32 operationId = _hashOperation(caller, target, data);
+        uint32 nonce;
 
         if (setback != 0) {
-            _consumeScheduledOp(operationId);
+            nonce = _consumeScheduledOp(operationId);
         }
 
         // Mark the target and selector as authorised
@@ -644,6 +665,8 @@ contract AccessManager is Context, Multicall, IAccessManager {
 
         // Reset relay identifier
         _relayIdentifier = relayIdentifierBefore;
+
+        return nonce;
     }
 
     /**
@@ -664,8 +687,9 @@ contract AccessManager is Context, Multicall, IAccessManager {
     /**
      * @dev Internal variant of {consumeScheduledOp} that operates on bytes32 operationId.
      */
-    function _consumeScheduledOp(bytes32 operationId) internal virtual {
-        uint48 timepoint = _schedules[operationId];
+    function _consumeScheduledOp(bytes32 operationId) internal virtual returns (uint32) {
+        uint48 timepoint = _schedules[operationId].timepoint;
+        uint32 nonce = _schedules[operationId].nonce;
 
         if (timepoint == 0) {
             revert AccessManagerNotScheduled(operationId);
@@ -676,7 +700,9 @@ contract AccessManager is Context, Multicall, IAccessManager {
         }
 
         delete _schedules[operationId];
-        emit OperationExecuted(operationId, timepoint);
+        emit OperationExecuted(operationId, nonce, timepoint);
+
+        return nonce;
     }
 
     /**
@@ -688,12 +714,12 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {OperationCanceled} event.
      */
-    function cancel(address caller, address target, bytes calldata data) public virtual {
+    function cancel(address caller, address target, bytes calldata data) public virtual returns (uint32) {
         address msgsender = _msgSender();
         bytes4 selector = bytes4(data[0:4]);
 
         bytes32 operationId = _hashOperation(caller, target, data);
-        if (_schedules[operationId] == 0) {
+        if (_schedules[operationId].timepoint == 0) {
             revert AccessManagerNotScheduled(operationId);
         } else if (caller != msgsender) {
             // calls can only be canceled by the account that scheduled them, a global admin, or by a guardian of the required group.
@@ -705,15 +731,18 @@ contract AccessManager is Context, Multicall, IAccessManager {
             }
         }
 
-        uint48 timepoint = _schedules[operationId];
-        delete _schedules[operationId];
-        emit OperationCanceled(operationId, timepoint);
+        uint32 nonce = _schedules[operationId].nonce;
+        uint48 timepoint = _schedules[operationId].timepoint;
+        delete _schedules[operationId].timepoint;
+        emit OperationCanceled(operationId, nonce, timepoint);
+
+        return nonce;
     }
 
     /**
      * @dev Hashing function for delayed operations
      */
-    function _hashOperation(address caller, address target, bytes calldata data) private pure returns (bytes32) {
+    function _hashOperation(address caller, address target, bytes calldata data) internal pure virtual returns (bytes32) {
         return keccak256(abi.encode(caller, target, data));
     }
 

contracts/access/manager/IAccessManager.sol

@@ -9,17 +9,17 @@ interface IAccessManager {
     /**
      * @dev A delayed operation was scheduled.
      */
-    event OperationScheduled(bytes32 indexed operationId, uint48 schedule, address caller, address target, bytes data);
+    event OperationScheduled(bytes32 indexed operationId, uint32 indexed nonce, uint48 schedule, address caller, address target, bytes data);
 
     /**
      * @dev A scheduled operation was executed.
      */
-    event OperationExecuted(bytes32 indexed operationId, uint48 schedule);
+    event OperationExecuted(bytes32 indexed operationId, uint32 indexed nonce, uint48 schedule);
 
     /**
      * @dev A scheduled operation was canceled.
      */
-    event OperationCanceled(bytes32 indexed operationId, uint48 schedule);
+    event OperationCanceled(bytes32 indexed operationId, uint32 indexed nonce, uint48 schedule);
 
     event GroupLabel(uint64 indexed groupId, string label);
     event GroupGranted(uint64 indexed groupId, address indexed account, uint32 delay, uint48 since);
@@ -94,11 +94,13 @@ interface IAccessManager {
 
     function getSchedule(bytes32 id) external returns (uint48);
 
-    function schedule(address target, bytes calldata data, uint48 when) external returns (bytes32);
+    function getNonce(bytes32 id) external returns (uint32);
 
-    function relay(address target, bytes calldata data) external payable;
+    function schedule(address target, bytes calldata data, uint48 when) external returns (bytes32, uint32);
 
-    function cancel(address caller, address target, bytes calldata data) external;
+    function relay(address target, bytes calldata data) external payable returns (uint32);
+
+    function cancel(address caller, address target, bytes calldata data) external returns (uint32);
 
     function consumeScheduledOp(address caller, bytes calldata data) external;
 

contracts/governance/extensions/GovernorTimelockAccess.sol

@@ -17,16 +17,18 @@ import {Time} from "../../utils/types/Time.sol";
  */
 abstract contract GovernorTimelockAccess is Governor {
     struct ExecutionPlan {
+        uint16 length;
         uint32 delay;
-        OperationDetails[] operations;
+        // We use mappings instead of arrays because it allows us to pack values in storage more tightly without storing
+        // the length redundantly.
+        // We pack 8 operations' data in each bucket. Each uint32 value is set to 1 upon proposal creation if it has to
+        // be scheduled and relayed through the manager. Upon queuing, the value is set to nonce + 1, where the nonce is
+        // that which we get back from the manager when scheduling the operation.
+        mapping (uint256 bucket => uint32[8]) managerData;
     }
 
-    struct OperationDetails {
-        bool delayed;
-        bytes32 id;
-    }
+    mapping(uint256 proposalId => ExecutionPlan) private _executionPlan;
 
-    mapping(uint256 => ExecutionPlan) private _executionPlan;
     mapping(address target => address) private _authorityOverride;
 
     uint32 _baseDelay;
@@ -77,10 +79,21 @@ abstract contract GovernorTimelockAccess is Governor {
     }
 
     /**
-     * @dev Public accessor to check the execution details.
+     * @dev Public accessor to check the execution plan, including the number of seconds that the proposal will be
+     * delayed since queuing, and an array indicating which of the proposal actions will be executed indirectly through
+     * the associated {AccessManager}.
      */
-    function proposalExecutionPlan(uint256 proposalId) public view returns (ExecutionPlan memory) {
-        return _executionPlan[proposalId];
+    function proposalExecutionPlan(uint256 proposalId) public view returns (uint32, bool[] memory) {
+        ExecutionPlan storage plan = _executionPlan[proposalId];
+
+        uint32 delay = plan.delay;
+        uint32 length = plan.length;
+        bool[] memory indirect = new bool[](length);
+        for (uint256 i = 0; i < length; ++i) {
+            (indirect[i], ) = _getManagerData(plan, i);
+        }
+
+        return (delay, indirect);
     }
 
     /**
@@ -97,12 +110,12 @@ abstract contract GovernorTimelockAccess is Governor {
         uint32 neededDelay = baseDelaySeconds();
 
         ExecutionPlan storage plan = _executionPlan[proposalId];
+        plan.length = SafeCast.toUint16(targets.length);
 
         for (uint256 i = 0; i < targets.length; ++i) {
-            plan.operations.push();
             uint32 delay = _detectExecutionRequirements(targets[i], bytes4(calldatas[i]));
             if (delay > 0) {
-                plan.operations[i].delayed = true;
+                _setManagerData(plan, i, 0);
             }
             // downcast is safe because both arguments are uint32
             neededDelay = uint32(Math.max(delay, neededDelay));
@@ -130,8 +143,10 @@ abstract contract GovernorTimelockAccess is Governor {
         uint48 eta = Time.timestamp() + plan.delay;
 
         for (uint256 i = 0; i < targets.length; ++i) {
-            if (plan.operations[i].delayed) {
-                plan.operations[i].id = _manager.schedule(targets[i], calldatas[i], eta);
+            (bool delayed,) = _getManagerData(plan, i);
+            if (delayed) {
+                (, uint32 nonce) = _manager.schedule(targets[i], calldatas[i], eta);
+                _setManagerData(plan, i, nonce);
             }
         }
 
@@ -153,15 +168,13 @@ abstract contract GovernorTimelockAccess is Governor {
         ExecutionPlan storage plan = _executionPlan[proposalId];
 
         for (uint256 i = 0; i < targets.length; ++i) {
-            if (plan.operations[i].delayed) {
-                bytes32 operationId = plan.operations[i].id;
-
-                uint48 schedule = _manager.getSchedule(operationId);
-                if (schedule != eta) {
-                    revert GovernorNotQueuedProposal(proposalId);
+            (bool delayed, uint32 nonce) = _getManagerData(plan, i);
+            if (delayed) {
+                uint32 relayedNonce = _manager.relay{value: values[i]}(targets[i], calldatas[i]);
+                if (relayedNonce != nonce) {
+                    // TODO: custom error
+                    revert();
                 }
-
-                _manager.relay{value: values[i]}(targets[i], calldatas[i]);
             } else {
                 (bool success, bytes memory returndata) = targets[i].call{value: values[i]}(calldatas[i]);
                 Address.verifyCallResult(success, returndata);
@@ -189,12 +202,13 @@ abstract contract GovernorTimelockAccess is Governor {
         // If the proposal has been scheduled it will have an ETA and we have to externally cancel
         if (eta != 0) {
             for (uint256 i = 0; i < targets.length; ++i) {
-                if (plan.operations[i].delayed) {
-                    bytes32 operationId = plan.operations[i].id;
+                (bool delayed, uint32 nonce) = _getManagerData(plan, i);
+                if (delayed) {
                     // Attempt to cancel considering the operation could have been cancelled and rescheduled already
-                    uint48 schedule = _manager.getSchedule(operationId);
-                    if (schedule == eta) {
-                        _manager.cancel(address(this), targets[i], calldatas[i]);
+                    uint32 canceledNonce = _manager.cancel(address(this), targets[i], calldatas[i]);
+                    if (canceledNonce != nonce) {
+                        // TODO: custom error
+                        revert();
                     }
                 }
             }
@@ -220,4 +234,31 @@ abstract contract GovernorTimelockAccess is Governor {
     function _detectExecutionRequirements(address target, bytes4 selector) private view returns (uint32 delay) {
         (, delay) = AuthorityUtils.canCallWithDelay(address(_manager), address(this), target, selector);
     }
+
+    /**
+     * @dev Returns whether the operation at an index is delayed by the manager, and its scheduling nonce once queued.
+     */
+    function _getManagerData(ExecutionPlan storage plan, uint256 index) private view returns (bool, uint32) {
+        (uint256 bucket, uint256 subindex) = _getManagerDataIndices(index);
+        uint32 nonce = plan.managerData[bucket][subindex];
+        unchecked {
+            return nonce > 0 ? (true, nonce - 1) : (false, 0);
+        }
+    }
+
+    /**
+     * @dev Marks an operation at an index as delayed by the manager, and sets its scheduling nonce.
+     */
+    function _setManagerData(ExecutionPlan storage plan, uint256 index, uint32 nonce) private {
+        (uint256 bucket, uint256 subindex) = _getManagerDataIndices(index);
+        plan.managerData[bucket][subindex] = nonce + 1;
+    }
+
+    /**
+     * @dev Returns bucket and subindex for reading manager data from the packed array mapping.
+     */
+    function _getManagerDataIndices(uint256 index) private pure returns (uint256 bucket, uint256 subindex) {
+        bucket = index >> 3;  // index / 8
+        subindex = index & 7; // index % 8
+    }
 }