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 on the account.
+    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,72 +125,106 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
         return OperationState.Ready;
     }
 
-    /// @dev See {ERC7579Executor-canExecute}. Allows anyone to execute an operation if it's `Ready`.
-    function canExecute(
+    /**
+     * @dev See {ERC7579Executor-_validateExecution}. Validates that the operation is in `Ready` state.
+     *
+     * Derived contracts can override this function to add additional validation logic.
+     *
+     * Example extension:
+     *
+     * ```solidity
+     *  function _validateExecution(
+     *     address account,
+     *     Mode mode,
+     *     bytes calldata executionCalldata,
+     *     bytes32 salt
+     *  ) internal view virtual override {
+     *    bool conditionMet = ...; // custom logic to check condition
+     *    require(conditionMet, ERC7579ExecutorConditionNotMet());
+     *    super._validateExecution(account, mode, executionCalldata, salt);
+     *  }
+     *```
+     */
+    function _validateExecution(
         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);
+    ) internal view virtual override {
+        bytes32 id = hashOperation(account, mode, executionCalldata, salt);
+        _validateStateBitmap(id, _encodeStateBitmap(OperationState.Ready));
+
+        super._validateExecution(account, mode, executionCalldata, salt);
     }
 
     /**
-     * @dev Whether the caller is authorized to cancel operations.
-     * By default, this checks if the caller is the account itself. Derived contracts can
-     * override this to implement custom authorization logic.
+     * @dev Validates cancellation. 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 canCancel(
+     *  function _validateCancel(
      *     address account,
      *     Mode mode,
      *     bytes calldata executionCalldata,
      *     bytes32 salt
-     *  ) public view virtual returns (bool) {
-     *    bool isAuthorized = ...; // custom logic to check authorization
-     *    return isAuthorized || super.canCancel(account, mode, executionCalldata, salt);
+     *  ) internal view virtual override {
+     *    bool conditionMet = ...; // custom logic to check additional condition
+     *    require(conditionMet, ERC7579ExecutorConditionNotMet());
+     *    super._validateCancel(account, mode, executionCalldata, salt);
      *  }
      *```
      */
-    function canCancel(
+    function _validateCancel(
         address account,
-        Mode /* mode */,
-        bytes calldata /* executionCalldata */,
-        bytes32 /* salt */
-    ) public view virtual returns (bool) {
-        return account == msg.sender;
+        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(msg.sender == account, ERC7579ExecutorUnauthorizedCancellation());
     }
 
     /**
-     * @dev Whether the caller is authorized to schedule operations.
-     * By default, this checks if the caller is the account itself. Derived contracts can
-     * override this to implement custom authorization logic.
+     * @dev Validates scheduling. This base implementation validates the caller
+     * is authorized to schedule, the module is installed, and the operation was
+     * not scheduled before.
+     *
+     * Can be overridden by derived contracts to implement custom validation logic.
      *
      * Example extension:
      *
      * ```solidity
-     *  function canSchedule(
+     *  function _validateSchedule(
      *     address account,
      *     Mode mode,
      *     bytes calldata executionCalldata,
      *     bytes32 salt
-     *  ) public view virtual returns (bool) {
-     *    bool isAuthorized = ...; // custom logic to check authorization
-     *    return isAuthorized || super.canSchedule(account, mode, executionCalldata, salt);
+     *  ) internal view virtual override {
+     *    bool conditionMet = ...; // custom logic to check condition
+     *    require(conditionMet, ERC7579ExecutorConditionNotMet());
+     *    super._validateSchedule(account, mode, executionCalldata, salt);
      *  }
      *```
      */
-    function canSchedule(
+    function _validateSchedule(
         address account,
-        Mode /* mode */,
-        bytes calldata /* executionCalldata */,
-        bytes32 /* salt */
-    ) public view virtual returns (bool) {
-        return account == msg.sender;
+        Mode mode,
+        bytes calldata executionCalldata,
+        bytes32 salt
+    ) internal view virtual {
+        require(_config[account].installed, ERC7579ExecutorModuleNotInstalled());
+
+        bytes32 id = hashOperation(account, mode, executionCalldata, salt);
+        _validateStateBitmap(id, _encodeStateBitmap(OperationState.Unknown));
+
+        require(msg.sender == account, ERC7579ExecutorUnauthorizedSchedule());
     }
 
     /// @dev Minimum delay after which {setDelay} takes effect.
@@ -293,22 +330,19 @@ 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 {_validateSchedule} for validation.
      */
     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());
+        _validateSchedule(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 scheduled operation. See {_validateCancel} for validation.
      */
     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());
+        _validateCancel(account, mode, executionCalldata, salt);
+        _cancel(account, mode, executionCalldata, salt);
     }
 
     /**
@@ -354,42 +388,27 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor {
     }
 
     /**
-     * @dev Internal version of {schedule} that takes an `account` address as an argument.
-     *
-     * Requirements:
-     *
-     * * The operation must be `Unknown`.
+     * @dev Schedules an operation. Does not perform any validation checks.
      *
      * Emits an {ERC7579ExecutorOperationScheduled} event.
+     *
+     * NOTE: Calling this function directly will NOT perform any validation checks.
+     * Scheduling a operation should be done using {schedule}.
      */
-    function _schedule(
-        address account,
-        Mode mode,
-        bytes calldata executionCalldata,
-        bytes32 salt
-    ) internal virtual returns (bytes32 operationId, Schedule memory schedule_) {
+    function _schedule(address account, Mode mode, bytes calldata executionCalldata, bytes32 salt) internal virtual {
         bytes32 id = hashOperation(account, mode, executionCalldata, salt);
-        _validateStateBitmap(id, _encodeStateBitmap(OperationState.Unknown));
-
         (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}.
+     * @inheritdoc ERC7579Executor
      */
     function _execute(
         address account,
@@ -398,26 +417,24 @@ 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 Cancels an operation.
      *
-     * Requirements:
+     * Emits an {ERC7579ExecutorOperationCanceled} event.
      *
-     * * The operation must be `Scheduled` or `Ready`.
+     * NOTE: Calling this function directly will NOT perform any validation checks.
+     * Cancelling a operation should be done using {cancel}.
      *
-     * Canceled operations can't be rescheduled. Emits an {ERC7579ExecutorOperationCanceled} event.
+     * NOTE: Canceled operations can't be rescheduled.
      */
     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;
 

contracts/account/modules/ERC7579Executor.sol

@@ -11,7 +11,7 @@ import {Mode} from "@openzeppelin/contracts/account/utils/draft-ERC7579Utils.sol
  * The module enables accounts to execute arbitrary operations, leveraging the execution
  * capabilities defined in the ERC-7579 standard. By default, the executor is restricted to
  * operations initiated by the account itself, but this can be customized in derived contracts
- * by overriding the {canExecute} function.
+ * by overriding the {_validateExecution} function.
  *
  * TIP: This is a simplified executor that directly executes operations without delay or expiration
  * mechanisms. For a more advanced implementation with time-delayed execution patterns and
@@ -30,56 +30,56 @@ abstract contract ERC7579Executor is IERC7579Module {
     }
 
     /**
-     * @dev Check if the caller is authorized to execute operations.
-     * By default, this checks if the caller is the account itself. Derived contracts can
-     * override this to implement custom authorization logic.
+     * @dev Validates execution. By default, only validates the caller is the account itself.
+     * Derived contracts can override this function to add additional validation logic.
      *
      * Example extension:
      *
      * ```solidity
-     *  function canExecute(
+     *  function _validateExecution(
      *     address account,
      *     Mode mode,
      *     bytes calldata executionCalldata,
      *     bytes32 salt
-     *  ) public view virtual returns (bool) {
-     *    bool isAuthorized = ...; // custom logic to check authorization
-     *    return isAuthorized || super.canExecute(account, mode, executionCalldata, salt);
+     *  ) internal view virtual override {
+     *    bool conditionMet = ...; // custom logic to check condition
+     *    require(conditionMet, ERC7579ExecutorConditionNotMet());
+     *    super._validateExecution(account, mode, executionCalldata, salt);
      *  }
      *```
      */
-    function canExecute(
+    function _validateExecution(
         address account,
         Mode /* mode */,
         bytes calldata /* executionCalldata */,
         bytes32 /* salt */
-    ) public view virtual returns (bool) {
-        return msg.sender == account;
-    }
+    ) internal view virtual {}
 
     /**
      * @dev Executes an operation and returns the result data from the executed operation.
-     * Restricted to the account itself by default. See {_execute} for requirements and
-     * {canExecute} for authorization checks.
+     * See {_execute} for requirements and {_validateExecution} for validation.
      */
     function execute(
         address account,
         Mode mode,
         bytes calldata executionCalldata,
         bytes32 salt
     ) public virtual returns (bytes[] memory returnData) {
-        bool allowed = canExecute(account, mode, executionCalldata, salt);
-        returnData = _execute(account, mode, executionCalldata, salt); // Prioritize errors thrown in _execute
-        require(allowed, ERC7579UnauthorizedExecution());
-        return returnData;
+        _validateExecution(account, mode, executionCalldata, salt);
+        return _execute(account, mode, executionCalldata, salt);
     }
 
     /**
-     * @dev Internal version of {execute}. Emits {ERC7579ExecutorOperationExecuted} event.
+     * @dev Executes an operation.
+     *
+     * Emits {ERC7579ExecutorOperationExecuted} event.
      *
      * Requirements:
      *
      * * The `account` must implement the {IERC7579Execution-executeFromExecutor} function.
+     *
+     * NOTE: Calling this function directly will NOT perform any validation checks.
+     * Executing a operation should be done using {execute}.
      */
     function _execute(
         address account,

contracts/mocks/account/modules/ERC7579ExecutorMock.sol

@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.27;
+
+import {Mode} from "@openzeppelin/contracts/account/utils/draft-ERC7579Utils.sol";
+import {ERC7579Executor} from "../../../account/modules/ERC7579Executor.sol";
+
+contract ERC7579ExecutorMock is ERC7579Executor {
+    function onInstall(bytes calldata) public virtual {}
+
+    function onUninstall(bytes calldata) public virtual {}
+
+    function _validateExecution(
+        address account,
+        Mode /* mode */,
+        bytes calldata /* executionCalldata */,
+        bytes32 /* salt */
+    ) internal view override {
+        if (account != msg.sender) revert ERC7579UnauthorizedExecution();
+    }
+}