ERC7579DelayedExecutor - Refactored validation & executing logic#152
ERC7579DelayedExecutor - Refactored validation & executing logic#152gonzaotc wants to merge 10 commits into
Conversation
DelayedExecutor.canExecute| ) internal view virtual { | ||
| require(msg.sender == account, ERC7579UnauthorizedExecution()); |
There was a problem hiding this comment.
I would insist the issue with require statements is that developers can't remove them! The original canExecute intentionally returns bool to follow the pattern of condition || super.canExecute(), which allows to workaround a require(canSchedule(...)) error. I would rather keep that pattern in the new _validateExecutionRequest
| ) internal view virtual { | |
| require(msg.sender == account, ERC7579UnauthorizedExecution()); | |
| ) internal view virtual returns (bool) { | |
| return msg.sender == account; |
| bytes calldata executionCalldata, | ||
| bytes32 salt | ||
| ) internal view virtual { | ||
| require(_config[account].installed, ERC7579ExecutorModuleNotInstalled()); |
There was a problem hiding this comment.
I would insist on keeping this case as a no-op. It's a valid use case to extend and allow scheduling on contracts that haven't installed the module. For example, a factory contract could deploy accounts with create2 and install this module with pre-scheduled payments (ie. subscriptions). In that case it becomes impossible to build that case on top of this contract
Refactor to #121 with some objetives:
Validations were previously divided and duplicated across
canExecuteand_execute, which were causing inconsistencies if they didn't match. See Add ERC7579 Executor modules #121 (comment)The functions
_execute,_schedule,_cancelwere performing validations, while in this approach they're low-level functions that assume prior validation. I think is good to divide validation from the actual executing logic.More specific error throwing, where
_validateExecutionRequestcan determine the priority order of errors to throw for different requirement conditions. See Add ERC7579 Executor modules #121 (comment). Additionally, this approach allows to revert with appropriate errors before executing, which results in gas savings for unauthorized callers. See Add ERC7579 Executor modules #121 (comment)Scheduling was available even if the module wasn't installed. See Add ERC7579 Executor modules #121 (comment)
In this approach, the purposes of each function are very specific and any of them can be overrided for customization:
public execute: allows external calling, calls internal validation and execution.internal _validateExecutionRequest: validates execution requirements.internal _execute: low-level execution.