ERC7579DelayedExecutor - Refactored validation & executing logic

contracts/account/modules/ERC7579DelayedExecutor.sol

@@ -80,6 +80,9 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
     /// @dev Emitted when the expiration delay is updated.
     event ERC7579ExecutorExpirationUpdated(address indexed account, uint32 newExpiration);
 
+    /// @dev The module is not installed.
+    error ERC7579ExecutorModuleNotInstalled();
+
     /**
      * @dev The current state of a operation is not the expected. The `expectedStates` is a bitmap with the
      * bits enabled for each OperationState enum position counting from right to left. See {_encodeStateBitmap}.
@@ -122,21 +125,19 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
         return OperationState.Ready;
     }
 
-    /// @dev See {ERC7579Executor-canExecute}. Allows anyone to execute an operation if it's `Ready`.
+    /// @dev See {ERC7579Executor-canExecute}. This implementation allows anyone to execute an operation.
     function canExecute(
-        address account,
-        Mode mode,
-        bytes calldata executionCalldata,
-        bytes32 salt
+        address /* account */,
+        Mode /* mode */,
+        bytes calldata /* executionCalldata */,
+        bytes32 /* salt */
     ) public view virtual override returns (bool) {
-        return
-            state(account, mode, executionCalldata, salt) == OperationState.Ready ||
-            super.canExecute(account, mode, executionCalldata, salt);
+        return true;
     }
 
     /**
-     * @dev Whether the caller is authorized to cancel operations.
-     * By default, this checks if the caller is the account itself. Derived contracts can
+     * @dev Checks whether the caller is authorized to cancel operations.
+     * By default, checks if the caller is the account itself. Derived contracts can
      * override this to implement custom authorization logic.
      *
      * Example extension:
@@ -163,8 +164,8 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
     }
 
     /**
-     * @dev Whether the caller is authorized to schedule operations.
-     * By default, this checks if the caller is the account itself. Derived contracts can
+     * @dev Checks whether the caller is authorized to schedule operations.
+     * By default, checks if the caller is the account itself. Derived contracts can
      * override this to implement custom authorization logic.
      *
      * Example extension:
@@ -195,6 +196,16 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
         return 1 days; // Up to ~136 years
     }
 
+    /// @dev Default delay for account operations. Set if not provided during {onInstall}.
+    function defaultDelay() public view virtual returns (uint32) {
+        return 5 days;
+    }
+
+    /// @dev Default expiration for account operations. Set if not provided during {onInstall}.
+    function defaultExpiration() public view virtual returns (uint32) {
+        return 60 days;
+    }
+
     /// @dev Delay for a specific account.
     function getDelay(
         address account
@@ -236,16 +247,6 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
         return keccak256(abi.encode(account, mode, executionCalldata, salt));
     }
 
-    /// @dev Default delay for account operations. Set if not provided during {onInstall}.
-    function defaultDelay() public view virtual returns (uint32) {
-        return 5 days;
-    }
-
-    /// @dev Default expiration for account operations. Set if not provided during {onInstall}.
-    function defaultExpiration() public view virtual returns (uint32) {
-        return 60 days;
-    }
-
     /**
      * @dev Sets up the module's initial configuration when installed by an account.
      * The account calling this function becomes registered with the module.
@@ -274,6 +275,26 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
         }
     }
 
+    /**
+     * @dev Cleans up the {getDelay} and {getExpiration} values by scheduling them to `0`
+     * and respecting the previous delay and expiration values.
+     *
+     * IMPORTANT: This function does not clean up scheduled operations. This means operations
+     * could potentially be re-executed if the module is reinstalled later. This is a deliberate
+     * design choice for efficiency, but module implementations may want to override this behavior
+     * to clear scheduled operations during uninstallation for their specific use cases.
+     *
+     * WARNING: The account's delay will be removed if the account calls this function, allowing
+     * immediate scheduling of operations. As an account operator, make sure to uninstall to a
+     * predefined path in your account that properly handles the side effects of uninstallation.
+     * See {AccountERC7579-uninstallModule}.
+     */
+    function onUninstall(bytes calldata) public virtual {
+        _config[msg.sender].installed = false;
+        _setDelay(msg.sender, 0, minSetback()); // Avoids immediate downgrades
+        _setExpiration(msg.sender, 0);
+    }
+
     /**
      * @dev Allows an account to update its execution delay (see {getDelay}).
      *
@@ -293,42 +314,22 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
     /**
      * @dev Schedules an operation to be executed after the account's delay period (see {getDelay}).
      * Operations are uniquely identified by the combination of `mode`, `executionCalldata`, and `salt`.
-     * See {canSchedule} for authorization checks.
+     * See {canSchedule} for caller authorization and {_validateScheduleRequest} for extra
+     * validation checks.
      */
     function schedule(address account, Mode mode, bytes calldata executionCalldata, bytes32 salt) public virtual {
-        bool allowed = canSchedule(account, mode, executionCalldata, salt);
-        _schedule(account, mode, executionCalldata, salt); // Prioritize errors thrown in _schedule
-        require(allowed, ERC7579ExecutorUnauthorizedSchedule());
+        _validateScheduleRequest(account, mode, executionCalldata, salt);
+        _schedule(account, mode, executionCalldata, salt);
     }
 
     /**
-     * @dev Cancels a previously scheduled operation. Can only be called by the account that
-     * scheduled the operation. See {_cancel}.
+     * @dev Cancels a previously scheduled operation.
+     * See {canCancel} for caller authorization and {_validateCancelRequest} for extra
+     * validation checks.
      */
     function cancel(address account, Mode mode, bytes calldata executionCalldata, bytes32 salt) public virtual {
-        bool allowed = canCancel(account, mode, executionCalldata, salt);
-        _cancel(account, mode, executionCalldata, salt); // Prioritize errors thrown in _cancel
-        require(allowed, ERC7579ExecutorUnauthorizedCancellation());
-    }
-
-    /**
-     * @dev Cleans up the {getDelay} and {getExpiration} values by scheduling them to `0`
-     * and respecting the previous delay and expiration values.
-     *
-     * IMPORTANT: This function does not clean up scheduled operations. This means operations
-     * could potentially be re-executed if the module is reinstalled later. This is a deliberate
-     * design choice for efficiency, but module implementations may want to override this behavior
-     * to clear scheduled operations during uninstallation for their specific use cases.
-     *
-     * WARNING: The account's delay will be removed if the account calls this function, allowing
-     * immediate scheduling of operations. As an account operator, make sure to uninstall to a
-     * predefined path in your account that properly handles the side effects of uninstallation.
-     * See {AccountERC7579-uninstallModule}.
-     */
-    function onUninstall(bytes calldata) public virtual {
-        _config[msg.sender].installed = false;
-        _setDelay(msg.sender, 0, minSetback()); // Avoids immediate downgrades
-        _setExpiration(msg.sender, 0);
+        _validateCancelRequest(account, mode, executionCalldata, salt);
+        _cancel(account, mode, executionCalldata, salt);
     }
 
     /**
@@ -354,42 +355,111 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
     }
 
     /**
-     * @dev Internal version of {schedule} that takes an `account` address as an argument.
+     * @dev Validates a schedule request. This base implementation validates
+     * the caller is authorized to schedule, the module is installed, and the
+     * operation was not scheduled before.
      *
-     * Requirements:
+     * Can be overridden by derived contracts to implement custom validation logic.
      *
-     * * The operation must be `Unknown`.
+     * Example extension:
      *
-     * Emits an {ERC7579ExecutorOperationScheduled} event.
+     * ```solidity
+     *  function _validateScheduleRequest(
+     *     address account,
+     *     Mode mode,
+     *     bytes calldata executionCalldata,
+     *     bytes32 salt
+     *  ) internal view virtual override {
+     *    bool conditionMet = ...; // custom logic to check condition
+     *    require(conditionMet, ERC7579ExecutorConditionNotMet());
+     *    super._validateScheduleRequest(account, mode, executionCalldata, salt);
+     *  }
+     *```
      */
-    function _schedule(
+    function _validateScheduleRequest(
         address account,
         Mode mode,
         bytes calldata executionCalldata,
         bytes32 salt
-    ) internal virtual returns (bytes32 operationId, Schedule memory schedule_) {
+    ) internal view virtual {
+        require(_config[account].installed, ERC7579ExecutorModuleNotInstalled());
+
         bytes32 id = hashOperation(account, mode, executionCalldata, salt);
         _validateStateBitmap(id, _encodeStateBitmap(OperationState.Unknown));
 
-        (uint32 executableAfter, , ) = getDelay(account);
+        require(canSchedule(account, mode, executionCalldata, salt), ERC7579ExecutorUnauthorizedSchedule());
+    }
+
+    /**
+     * @dev See {ERC7579Executor-_validateExecutionRequest}.
+     *
+     * Validates the caller is authorized to execute and the operation is in a valid state.
+     */
+    function _validateExecutionRequest(
+        address account,
+        Mode mode,
+        bytes calldata executionCalldata,
+        bytes32 salt
+    ) internal view virtual override {
+        bytes32 id = hashOperation(account, mode, executionCalldata, salt);
+        _validateStateBitmap(id, _encodeStateBitmap(OperationState.Ready));
+
+        super._validateExecutionRequest(account, mode, executionCalldata, salt);
+    }
+
+    /**
+     * @dev Validates a cancellation request against necessary conditions. This base implementation
+     * validates the caller is authorized to cancel and that the operation is in a valid state.
+     *
+     * Derived contracts can override this function to add additional validation logic.
+     *
+     * Example extension:
+     *
+     * ```solidity
+     *  function _validateCancelRequest(
+     *     address account,
+     *     Mode mode,
+     *     bytes calldata executionCalldata,
+     *     bytes32 salt
+     *  ) internal view virtual override {
+     *    bool conditionMet = ...; // custom logic to check additional condition
+     *    require(conditionMet, ERC7579ExecutorConditionNotMet());
+     *    super._validateCancelRequest(account, mode, executionCalldata, salt);
+     *  }
+     *```
+     */
+    function _validateCancelRequest(
+        address account,
+        Mode mode,
+        bytes calldata executionCalldata,
+        bytes32 salt
+    ) internal view virtual {
+        bytes32 id = hashOperation(account, mode, executionCalldata, salt);
+        bytes32 allowedStates = _encodeStateBitmap(OperationState.Scheduled) | _encodeStateBitmap(OperationState.Ready);
+        _validateStateBitmap(id, allowedStates);
+
+        require(canCancel(account, mode, executionCalldata, salt), ERC7579ExecutorUnauthorizedCancellation());
+    }
 
+    /**
+     * @dev Low-level internal function to schedule an operation. Does not perform any validation checks.
+     *
+     * Emits an {ERC7579ExecutorOperationScheduled} event.
+     */
+    function _schedule(address account, Mode mode, bytes calldata executionCalldata, bytes32 salt) internal virtual {
+        bytes32 id = hashOperation(account, mode, executionCalldata, salt);
+        (uint32 executableAfter, , ) = getDelay(account);
         uint48 timepoint = Time.timestamp();
+
         _schedules[id].scheduledAt = timepoint;
         _schedules[id].executableAfter = executableAfter;
         _schedules[id].expiresAfter = getExpiration(account);
 
         emit ERC7579ExecutorOperationScheduled(account, id, mode, executionCalldata, salt, timepoint);
-        return (id, schedule_);
     }
 
     /**
      * @dev See {ERC7579Executor-_execute}.
-     *
-     * Requirements:
-     *
-     * * The operation must be `Ready`.
-     *
-     * NOTE: Anyone can trigger execution once the operation is `Ready`. See {canExecute}.
      */
     function _execute(
         address account,
@@ -398,26 +468,21 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
         bytes32 salt
     ) internal virtual override returns (bytes[] memory returnData) {
         bytes32 id = hashOperation(account, mode, executionCalldata, salt);
-        _validateStateBitmap(id, _encodeStateBitmap(OperationState.Ready));
 
         _schedules[id].executed = true;
 
         return super._execute(account, mode, executionCalldata, salt);
     }
 
     /**
-     * @dev Internal version of {cancel} that takes an `account` address as an argument.
+     * @dev Low-level internal function to cancel an operation. Does not perform any validation checks.
      *
-     * Requirements:
-     *
-     * * The operation must be `Scheduled` or `Ready`.
+     * Canceled operations can't be rescheduled.
      *
-     * Canceled operations can't be rescheduled. Emits an {ERC7579ExecutorOperationCanceled} event.
+     * Emits an {ERC7579ExecutorOperationCanceled} event.
      */
     function _cancel(address account, Mode mode, bytes calldata executionCalldata, bytes32 salt) internal virtual {
         bytes32 id = hashOperation(account, mode, executionCalldata, salt);
-        bytes32 allowedStates = _encodeStateBitmap(OperationState.Scheduled) | _encodeStateBitmap(OperationState.Ready);
-        _validateStateBitmap(id, allowedStates);
 
         _schedules[id].canceled = true;