Add ERC7579 Executors #5890
Add ERC7579 Executors #5890
Conversation
🦋 Changeset detectedLatest commit: 59870af The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Reviewer's GuideThis PR introduces two new ERC-7579 executor modules: a basic immediate executor and an advanced delayed executor with configurable scheduling, expiration, and cancellation. The delayed executor tracks operations in a Schedule struct, enforces time-based state transitions via a state bitmap, and provides account-specific delay/expiration settings. Comprehensive tests, mocks, and changesets are included to cover installation, scheduling, execution, expiration, and cancellation workflows. Sequence diagram for operation cancellation in ERC7579DelayedExecutorsequenceDiagram
participant Account
participant ERC7579DelayedExecutor
Account->>ERC7579DelayedExecutor: cancel(account, salt, mode, data)
ERC7579DelayedExecutor->>ERC7579DelayedExecutor: _validateCancel(account, salt, mode, data)
ERC7579DelayedExecutor->>ERC7579DelayedExecutor: _cancel(account, mode, data, salt)
Note over ERC7579DelayedExecutor: Operation marked as Canceled
Class diagram for ERC7579Executor and ERC7579DelayedExecutor modulesclassDiagram
class ERC7579Executor {
+execute(account, salt, mode, data)
+isModuleType(moduleTypeId)
#_validateExecution(account, salt, mode, data)
#_execute(account, mode, salt, executionCalldata)
<<abstract>>
}
class ERC7579DelayedExecutor {
+schedule(account, salt, mode, data)
+cancel(account, salt, mode, data)
+setDelay(newDelay)
+setExpiration(newExpiration)
+onInstall(initData)
+onUninstall(data)
+state(account, salt, mode, executionCalldata)
+getDelay(account)
+getExpiration(account)
+getSchedule(account, salt, mode, executionCalldata)
+hashOperation(account, salt, mode, executionCalldata)
#_validateSchedule(account, salt, mode, data)
#_validateCancel(account, salt, mode, data)
#_scheduleAt(account, salt, mode, executionCalldata, timepoint, delay)
#_setDelay(account, newDelay, minimumSetback)
#_setExpiration(account, newExpiration)
#_execute(account, salt, mode, executionCalldata)
#_cancel(account, mode, executionCalldata, salt)
#_validateStateBitmap(operationId, allowedStates)
#_encodeStateBitmap(operationState)
<<abstract>>
Schedule
ExecutionConfig
OperationState
}
ERC7579DelayedExecutor --|> ERC7579Executor
class Schedule {
uint48 scheduledAt
uint32 executableAfter
uint32 expiresAfter
bool executed
bool canceled
}
class ExecutionConfig {
Time.Delay delay
uint32 expiration
bool installed
}
class OperationState {
Unknown
Scheduled
Ready
Expired
Executed
Canceled
}
ERC7579DelayedExecutor o-- Schedule
ERC7579DelayedExecutor o-- ExecutionConfig
ERC7579DelayedExecutor o-- OperationState
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughAdds two new executor modules for ERC-7579 accounts: a base ERC7579Executor and an extended ERC7579DelayedExecutor with scheduling, delay, and expiration. Introduces corresponding mocks, tests, and documentation. Updates changesets for a minor release. No modifications to existing contracts; all additions are new files and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Exec as ERC7579Executor
participant Account as IERC7579Execution (Account)
Caller->>Exec: execute(account, salt, mode, data)
activate Exec
Exec->>Exec: _validateExecution(...)\n(return executionCalldata)
Exec->>Account: executeFromExecutor(mode, executionCalldata)
activate Account
Account-->>Exec: return bytes[]
deactivate Account
Exec-->>Caller: return bytes[]
deactivate Exec
Note over Exec: Emits ERC7579ExecutorOperationExecuted
sequenceDiagram
autonumber
actor Account
participant DExec as ERC7579DelayedExecutor
participant Time as Blockchain Time
participant Target as IERC7579Execution (Account)
Account->>DExec: onInstall(initData)\n(set delay/expiration)
Note over DExec: Emits Delay/Expiration Updated
Account->>DExec: schedule(account, salt, mode, data)
Note over DExec: State: Unknown → Scheduled\nEmits OperationScheduled
Time-->>DExec: advance past delay
alt before expiration
Account->>DExec: execute(account, salt, mode, data)
DExec->>DExec: validate state (Ready)
DExec->>Target: executeFromExecutor(mode, calldata)
Note over DExec: State: Scheduled/Ready → Executed
else after expiration
Account->>DExec: execute(...)
DExec-->>Account: revert UnexpectedOperationState\n(State = Expired)
end
opt cancellation path
Account->>DExec: cancel(account, salt, mode, data)
Note over DExec: State: Scheduled/Ready → Canceled\nEmits OperationCanceled
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
_executeoverride in ERC7579DelayedExecutor has swappedsaltandmodeparameters compared to the base implementation, causing the super call to receive them in the wrong order—please align the parameter order to match the parent signature. - The
_scheduleAtfunction returns an uninitializedSchedule memory schedule_that is never used—consider removing this unused return value or properly populating it if needed. - The
minSetbackcomment “Up to ~136 years” is confusing since the function returns a constant 5 days; please clarify or correct the remark to avoid misunderstandings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_execute` override in ERC7579DelayedExecutor has swapped `salt` and `mode` parameters compared to the base implementation, causing the super call to receive them in the wrong order—please align the parameter order to match the parent signature.
- The `_scheduleAt` function returns an uninitialized `Schedule memory schedule_` that is never used—consider removing this unused return value or properly populating it if needed.
- The `minSetback` comment “Up to ~136 years” is confusing since the function returns a constant 5 days; please clarify or correct the remark to avoid misunderstandings.
## Individual Comments
### Comment 1
<location> `contracts/account/modules/ERC7579DelayedExecutor.sol:199` </location>
<code_context>
+ * * The account (i.e `msg.sender`) must implement the {IERC7579ModuleConfig} interface.
+ * * `initData` must be empty or decode correctly to `(uint32, uint32)`.
+ */
+ function onInstall(bytes calldata initData) public virtual {
+ if (!_config[msg.sender].installed) {
+ _config[msg.sender].installed = true;
+ (uint32 initialDelay, uint32 initialExpiration) = initData.length > 0
+ ? abi.decode(initData, (uint32, uint32))
+ : (minSetback(), defaultExpiration());
+ // An old delay might be still present
+ // So we set 0 for the minimum setback relying on any old value as the minimum delay
+ _setDelay(msg.sender, initialDelay, 0);
+ _setExpiration(msg.sender, initialExpiration);
+ }
+ }
</code_context>
<issue_to_address>
Potential issue with decoding initData if length is nonzero but not valid.
If initData is non-empty but incorrectly formatted, abi.decode will revert. Consider adding validation or custom error handling to improve robustness.
</issue_to_address>
### Comment 2
<location> `contracts/account/modules/ERC7579DelayedExecutor.sol:244` </location>
<code_context>
+ * @dev Cancels a previously scheduled operation. Can only be called by the account that
+ * scheduled the operation. See {_cancel}.
+ */
+ function cancel(address account, bytes32 salt, bytes32 mode, bytes calldata data) public virtual {
+ _validateCancel(account, salt, mode, data);
+ _cancel(account, mode, data, salt); // Prioritize errors thrown in _cancel
</code_context>
<issue_to_address>
Parameter order in _cancel differs from schedule/execute.
Standardizing the parameter order across these functions will improve consistency and maintainability.
Suggested implementation:
```
_cancel(account, salt, mode, data); // Prioritize errors thrown in _cancel
```
You must also update the definition of the `_cancel` function and all its internal calls/usages to use the standardized parameter order: `(address account, bytes32 salt, bytes32 mode, bytes calldata data)`. This ensures consistency throughout the codebase.
</issue_to_address>
### Comment 3
<location> `contracts/account/modules/ERC7579DelayedExecutor.sol:352` </location>
<code_context>
+ *
+ * Emits an {ERC7579ExecutorOperationScheduled} event.
+ */
+ function _scheduleAt(
+ address account,
+ bytes32 salt,
+ bytes32 mode,
+ bytes calldata executionCalldata,
+ uint48 timepoint,
+ uint32 delay
+ ) internal virtual returns (bytes32 operationId, Schedule memory schedule_) {
+ bytes32 id = hashOperation(account, salt, mode, executionCalldata);
+ _validateStateBitmap(id, _encodeStateBitmap(OperationState.Unknown));
+
+ _schedules[id].scheduledAt = timepoint;
+ _schedules[id].executableAfter = delay;
+ _schedules[id].expiresAfter = getExpiration(account);
+
+ emit ERC7579ExecutorOperationScheduled(account, id, salt, mode, executionCalldata, timepoint + delay);
+ return (id, schedule_);
+ }
</code_context>
<issue_to_address>
schedule_ is returned but never assigned.
Assign schedule_ to _schedules[id] before returning to ensure the function returns the correct Schedule value.
</issue_to_address>
### Comment 4
<location> `contracts/account/modules/ERC7579Executor.sol:38` </location>
<code_context>
+ * Restricted to the account itself by default. See {_execute} for requirements and
+ * {_validateExecution} for authorization checks.
+ */
+ function execute(
+ address account,
+ bytes32 salt,
+ bytes32 mode,
+ bytes calldata data
+ ) public virtual returns (bytes[] memory returnData) {
+ bytes calldata executionCalldata = _validateExecution(account, salt, mode, data);
+ returnData = _execute(account, mode, salt, executionCalldata); // Prioritize errors thrown in _execute
+ return returnData;
+ }
+
</code_context>
<issue_to_address>
Parameter order in _execute differs from execute.
Standardize the parameter order between execute and _execute to improve consistency and maintainability.
Suggested implementation:
```
function execute(
address account,
bytes32 salt,
bytes32 mode,
bytes calldata data
) public virtual returns (bytes[] memory returnData) {
bytes calldata executionCalldata = _validateExecution(account, salt, mode, data);
returnData = _execute(account, salt, mode, executionCalldata); // Prioritize errors thrown in _execute
return returnData;
}
```
```
returnData = _execute(account, salt, mode, executionCalldata); // Prioritize errors thrown in _execute
```
You must also update the definition of `_execute` and all its usages throughout the codebase to use the standardized parameter order: `(address account, bytes32 salt, bytes32 mode, bytes calldata executionCalldata)`.
</issue_to_address>
### Comment 5
<location> `contracts/mocks/account/modules/ERC7579ExecutorMocks.sol:9` </location>
<code_context>
+import {ERC7579DelayedExecutor} from "../../../account/modules/ERC7579DelayedExecutor.sol";
+
+abstract contract ERC7579ExecutorMock is ERC7579Executor {
+ function onInstall(bytes calldata data) external {}
+
+ function onUninstall(bytes calldata data) external {}
+
+ function _validateExecution(
</code_context>
<issue_to_address>
Empty onInstall and onUninstall implementations may be misleading.
If these are intentionally no-ops for mocks, please add a comment clarifying this to prevent confusion for future maintainers.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
function onInstall(bytes calldata data) external {}
function onUninstall(bytes calldata data) external {}
=======
/// @dev No-op implementation for mocks. Intentionally left empty.
function onInstall(bytes calldata data) external {}
/// @dev No-op implementation for mocks. Intentionally left empty.
function onUninstall(bytes calldata data) external {}
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `.changeset/weak-chefs-open.md:5` </location>
<code_context>
+'openzeppelin-solidity': minor
+---
+
+`ERC7579Executor`: Add an executor module that enables executing calls from accounts where the it's installed.
</code_context>
<issue_to_address>
Correct the grammatical error in 'where the it's installed'.
Change to 'where it's installed' or 'where the module is installed' for correct grammar.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| function onInstall(bytes calldata initData) public virtual { | ||
| if (!_config[msg.sender].installed) { | ||
| _config[msg.sender].installed = true; | ||
| (uint32 initialDelay, uint32 initialExpiration) = initData.length > 0 | ||
| ? abi.decode(initData, (uint32, uint32)) | ||
| : (minSetback(), defaultExpiration()); | ||
| // An old delay might be still present | ||
| // So we set 0 for the minimum setback relying on any old value as the minimum delay | ||
| _setDelay(msg.sender, initialDelay, 0); | ||
| _setExpiration(msg.sender, initialExpiration); |
There was a problem hiding this comment.
issue: Potential issue with decoding initData if length is nonzero but not valid.
If initData is non-empty but incorrectly formatted, abi.decode will revert. Consider adding validation or custom error handling to improve robustness.
| * @dev Cancels a previously scheduled operation. Can only be called by the account that | ||
| * scheduled the operation. See {_cancel}. | ||
| */ | ||
| function cancel(address account, bytes32 salt, bytes32 mode, bytes calldata data) public virtual { |
There was a problem hiding this comment.
suggestion: Parameter order in _cancel differs from schedule/execute.
Standardizing the parameter order across these functions will improve consistency and maintainability.
Suggested implementation:
_cancel(account, salt, mode, data); // Prioritize errors thrown in _cancel
You must also update the definition of the _cancel function and all its internal calls/usages to use the standardized parameter order: (address account, bytes32 salt, bytes32 mode, bytes calldata data). This ensures consistency throughout the codebase.
| function _scheduleAt( | ||
| address account, | ||
| bytes32 salt, | ||
| bytes32 mode, | ||
| bytes calldata executionCalldata, | ||
| uint48 timepoint, | ||
| uint32 delay | ||
| ) internal virtual returns (bytes32 operationId, Schedule memory schedule_) { | ||
| bytes32 id = hashOperation(account, salt, mode, executionCalldata); | ||
| _validateStateBitmap(id, _encodeStateBitmap(OperationState.Unknown)); |
There was a problem hiding this comment.
issue (bug_risk): schedule_ is returned but never assigned.
Assign schedule_ to _schedules[id] before returning to ensure the function returns the correct Schedule value.
| function execute( | ||
| address account, | ||
| bytes32 salt, | ||
| bytes32 mode, | ||
| bytes calldata data | ||
| ) public virtual returns (bytes[] memory returnData) { | ||
| bytes calldata executionCalldata = _validateExecution(account, salt, mode, data); | ||
| returnData = _execute(account, mode, salt, executionCalldata); // Prioritize errors thrown in _execute | ||
| return returnData; |
There was a problem hiding this comment.
suggestion: Parameter order in _execute differs from execute.
Standardize the parameter order between execute and _execute to improve consistency and maintainability.
Suggested implementation:
function execute(
address account,
bytes32 salt,
bytes32 mode,
bytes calldata data
) public virtual returns (bytes[] memory returnData) {
bytes calldata executionCalldata = _validateExecution(account, salt, mode, data);
returnData = _execute(account, salt, mode, executionCalldata); // Prioritize errors thrown in _execute
return returnData;
}
returnData = _execute(account, salt, mode, executionCalldata); // Prioritize errors thrown in _execute
You must also update the definition of _execute and all its usages throughout the codebase to use the standardized parameter order: (address account, bytes32 salt, bytes32 mode, bytes calldata executionCalldata).
| function onInstall(bytes calldata data) external {} | ||
|
|
||
| function onUninstall(bytes calldata data) external {} |
There was a problem hiding this comment.
suggestion: Empty onInstall and onUninstall implementations may be misleading.
If these are intentionally no-ops for mocks, please add a comment clarifying this to prevent confusion for future maintainers.
| function onInstall(bytes calldata data) external {} | |
| function onUninstall(bytes calldata data) external {} | |
| /// @dev No-op implementation for mocks. Intentionally left empty. | |
| function onInstall(bytes calldata data) external {} | |
| /// @dev No-op implementation for mocks. Intentionally left empty. | |
| function onUninstall(bytes calldata data) external {} |
| 'openzeppelin-solidity': minor | ||
| --- | ||
|
|
||
| `ERC7579Executor`: Add an executor module that enables executing calls from accounts where the it's installed. |
There was a problem hiding this comment.
issue (typo): Correct the grammatical error in 'where the it's installed'.
Change to 'where it's installed' or 'where the module is installed' for correct grammar.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (19)
.changeset/weak-chefs-open.md (1)
5-5: Fix grammar in summary line.Replace “where the it's” with a grammatically correct phrase.
-`ERC7579Executor`: Add an executor module that enables executing calls from accounts where the it's installed. +`ERC7579Executor`: Add an executor module that enables executing calls from the account where it's installed..changeset/free-waves-draw.md (1)
5-5: Tighten wording for clarity.Minor style improvement; current sentence is redundant.
-`ERC7579DelayedExecutor`: Add executor module that adds a delay before executing an account operation. +`ERC7579DelayedExecutor`: Add an executor module that delays execution of account operations.contracts/account/README.adoc (1)
10-11: Fix grammar in module descriptions.Correct the phrasing in the first bullet; the second is fine but can be tightened if desired.
-* {ERC7579Executor}: An executor module that enables executing calls from accounts where the it's installed. +* {ERC7579Executor}: An executor module that enables executing calls from the account where it's installed.Optional:
-* {ERC7579DelayedExecutor}: An executor module that adds a delay before executing an account operation. +* {ERC7579DelayedExecutor}: An executor module that delays execution of account operations.test/account/modules/ERC7579Executor.test.js (2)
18-21: Fix comment: this deploys an executor, not a validator.Keep comments accurate to avoid confusion with validator modules.
- // Deploy ERC-7579 validator module + // Deploy ERC-7579 executor module
40-44: Use bigint literal for uint256 value.Prefer 0n to match the encodeSingle signature and avoid BigNumberish coercion edge cases.
- const calldata = encodeSingle(target, 0, data); + const calldata = encodeSingle(target, 0n, data);contracts/mocks/account/modules/ERC7579ExecutorMocks.sol (2)
9-12: Silence unused-parameter warnings in no-op hooks.Mark these as pure and drop param names to avoid warnings and tighten intent.
- function onInstall(bytes calldata data) external {} + function onInstall(bytes calldata) external pure {} @@ - function onUninstall(bytes calldata data) external {} + function onUninstall(bytes calldata) external pure {}
24-30: Add explicit revert reason or custom error for clearer failures.Bare require makes debugging test failures harder. Add a short reason.
- function _validateSchedule(address account, bytes32, bytes32, bytes calldata) internal view override { - require(msg.sender == account); - } + function _validateSchedule(address account, bytes32, bytes32, bytes calldata) internal view override { + require(msg.sender == account, "ERC7579DelayedExecutorMock: only account"); + } @@ - function _validateCancel(address account, bytes32, bytes32, bytes calldata) internal view override { - require(msg.sender == account); - } + function _validateCancel(address account, bytes32, bytes32, bytes calldata) internal view override { + require(msg.sender == account, "ERC7579DelayedExecutorMock: only account"); + }test/account/modules/ERC7579Module.behavior.js (3)
7-10: Avoideventuallyfor simple view returns; await the call directly.Hardhat tests in this repo typically use
awaited values; this also avoids reliance on chai-as-promised.- await expect(this.mock.isModuleType(this.moduleType)).to.eventually.be.true; - await expect(this.mock.isModuleType(999)).to.eventually.be.false; // Using random unassigned module type + expect(await this.mock.isModuleType(this.moduleType)).to.equal(true); + expect(await this.mock.isModuleType(999)).to.equal(false); // Using random unassigned module type
47-54: Consistently call from the account wrapper; dropeventually.Keep caller consistent between valid/invalid branches and avoid
eventually.- await expect(this.mockFromAccount.isValidSignatureWithSender(this.other, hash, signature)).to.eventually.equal( - MAGIC_VALUE, - ); + expect( + await this.mockFromAccount.isValidSignatureWithSender(this.other, hash, signature) + ).to.equal(MAGIC_VALUE);
56-62: Use the same contract instance as above (mockFromAccount) and avoideventually.Minor consistency fix.
- const signature = this.invalidSignature || '0x00'; - await expect(this.mock.isValidSignatureWithSender(this.other, hash, signature)).to.eventually.equal( - INVALID_VALUE, - ); + const signature = this.invalidSignature || '0x00'; + expect( + await this.mockFromAccount.isValidSignatureWithSender(this.other, hash, signature) + ).to.equal(INVALID_VALUE);test/account/modules/ERC7579DelayedExecutor.test.js (4)
77-94: Add assertion for OperationExecuted event to catch parameter-order regressions.Validates we emit with the right (account, salt, mode, calldata).
- await this.mock.execute(this.mockAccount.address, salt, this.mode, this.calldata); + await expect( + this.mock.execute(this.mockAccount.address, salt, this.mode, this.calldata) + ) + .to.emit(this.mock, 'ERC7579ExecutorOperationExecuted') + .withArgs(this.mockAccount.address, salt, this.mode, this.calldata);
247-255: Also assert OperationExecuted during execution path (any caller).- await expect(this.mock.execute(this.mockAccount.address, salt, this.mode, this.calldata)) + await expect(this.mock.execute(this.mockAccount.address, salt, this.mode, this.calldata)) .to.emit(this.target, 'MockFunctionCalledWithArgs') .withArgs(...this.args); + await expect(this.mock.execute(this.mockAccount.address, salt, this.mode, this.calldata)) + .to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState');
257-265: Mirror the event assertion for account-caller path.- await expect(this.mockFromAccount.execute(this.mockAccount.address, salt, this.mode, this.calldata)) + await expect(this.mockFromAccount.execute(this.mockAccount.address, salt, this.mode, this.calldata)) .to.emit(this.target, 'MockFunctionCalledWithArgs') .withArgs(...this.args);
134-149: Minor wording: test title typo.“schedule delay unset and unsets expiration on uninstallation” → “schedules delay reset and unsets expiration on uninstallation”.
contracts/account/modules/ERC7579Executor.sol (2)
33-47: Doc says “Restricted to the account itself by default” but code does not enforce it.Clarify that access control is left to
_validateExecutionand/or the account’sexecuteFromExecutor.- * @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 - * {_validateExecution} for authorization checks. + * @dev Executes an operation and returns the result data from the executed operation. + * Access control is delegated to {_validateExecution} and/or the account’s + * {IERC7579Execution-executeFromExecutor} implementation.
84-92: Parameter order increases footguns across overrides. Consider aligning with publicexecute(account, salt, mode, …).Not a blocker for this PR, but reordering to
(account, salt, mode, executionCalldata)would reduce mistakes in derived contracts.contracts/account/modules/ERC7579DelayedExecutor.sol (3)
170-177:hashOperationcan bepure.No state read; makes intent explicit and can slightly reduce gas.
- function hashOperation( + function hashOperation( address account, bytes32 salt, bytes32 mode, bytes calldata executionCalldata - ) public view virtual returns (bytes32) { + ) public pure virtual returns (bytes32) { return keccak256(abi.encode(account, salt, mode, executionCalldata)); }
352-369: Remove unusedSchedulereturn to simplify API.The second return value is never populated/used; trimming avoids confusion.
- function _scheduleAt( + function _scheduleAt( address account, bytes32 salt, bytes32 mode, bytes calldata executionCalldata, uint48 timepoint, uint32 delay - ) internal virtual returns (bytes32 operationId, Schedule memory schedule_) { + ) internal virtual returns (bytes32 operationId) { bytes32 id = hashOperation(account, salt, mode, executionCalldata); _validateStateBitmap(id, _encodeStateBitmap(OperationState.Unknown)); _schedules[id].scheduledAt = timepoint; _schedules[id].executableAfter = delay; _schedules[id].expiresAfter = getExpiration(account); emit ERC7579ExecutorOperationScheduled(account, id, salt, mode, executionCalldata, timepoint + delay); - return (id, schedule_); + return id; }
235-238: Name nit:executableAfterlocal reads the configured “delay”.Minor: consider renaming the local to
delayfor clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
.changeset/free-waves-draw.md(1 hunks).changeset/weak-chefs-open.md(1 hunks)contracts/account/README.adoc(2 hunks)contracts/account/modules/ERC7579DelayedExecutor.sol(1 hunks)contracts/account/modules/ERC7579Executor.sol(1 hunks)contracts/mocks/account/modules/ERC7579ExecutorMocks.sol(1 hunks)test/account/modules/ERC7579DelayedExecutor.test.js(1 hunks)test/account/modules/ERC7579Executor.test.js(1 hunks)test/account/modules/ERC7579Module.behavior.js(1 hunks)test/helpers/enums.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/account/modules/ERC7579Executor.test.js (2)
test/helpers/account.js (1)
impersonate(7-10)test/helpers/erc7579.js (5)
MODULE_TYPE_EXECUTOR(4-4)encodeSingle(26-27)encodeMode(15-24)CALL_TYPE_CALL(11-11)EXEC_TYPE_DEFAULT(8-8)
test/account/modules/ERC7579Module.behavior.js (1)
test/helpers/erc4337.js (1)
SIG_VALIDATION_FAILURE(4-4)
test/account/modules/ERC7579DelayedExecutor.test.js (2)
test/helpers/account.js (1)
impersonate(7-10)test/helpers/erc7579.js (5)
MODULE_TYPE_EXECUTOR(4-4)encodeSingle(26-27)encodeMode(15-24)CALL_TYPE_CALL(11-11)EXEC_TYPE_DEFAULT(8-8)
🪛 LanguageTool
.changeset/weak-chefs-open.md
[grammar] ~5-~5: There might be a mistake here.
Context: ... from accounts where the it's installed.
(QB_NEW_EN)
.changeset/free-waves-draw.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...': minor --- ERC7579DelayedExecutor: Add executor module that adds a delay befor...
(QB_NEW_EN)
[grammar] ~5-~5: There might be a mistake here.
Context: ...y before executing an account operation.
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: slither
- GitHub Check: tests-foundry
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: tests
- GitHub Check: halmos
🔇 Additional comments (5)
.changeset/weak-chefs-open.md (1)
2-2: No action needed – changeset key matches package.json name
The package.jsonnamefield is “openzeppelin-solidity” and all changesets target'openzeppelin-solidity', so the original concern is outdated..changeset/free-waves-draw.md (1)
2-2: No changes needed—package name in changeset is correctThe changeset key
'openzeppelin-solidity'exactly matches thenamefield in package.json, so no updates are required.test/helpers/enums.js (1)
14-15: ✅ ERC7579OperationState enum matches Solidity definitionThe JS enum in test/helpers/enums.js exactly mirrors the Solidity
enum OperationStatein contracts/account/modules/ERC7579DelayedExecutor.sol (both defineUnknown,Scheduled,Ready,Expired,Executed,Canceledin the same order). No further changes are needed.contracts/mocks/account/modules/ERC7579ExecutorMocks.sol (1)
13-20: LGTM: pass-through validation for tests.Returning calldata verbatim is appropriate for the mock.
test/account/modules/ERC7579DelayedExecutor.test.js (1)
200-226: Ignore missing_scheduleAtwrapper check
The externalschedule(...)function in ERC7579DelayedExecutor.sol (line 233) invokes the internal_scheduleAtmethod (line 237), so tests callingschedulealready exercise_scheduleAt. No separate public wrapper for_scheduleAtis required and the existing tests will not fail at runtime.Likely an incorrect or invalid review comment.
| function schedule(address account, bytes32 salt, bytes32 mode, bytes calldata data) public virtual { | ||
| require(_config[account].installed, ERC7579ExecutorModuleNotInstalled()); | ||
| _validateSchedule(account, salt, mode, data); | ||
| (uint32 executableAfter, , ) = getDelay(account); | ||
| _scheduleAt(account, salt, mode, data, Time.timestamp(), executableAfter); | ||
| } |
There was a problem hiding this comment.
Use custom error style instead of require(..., CustomError()) (won’t compile).
Switch to explicit revert with the custom error.
- function schedule(address account, bytes32 salt, bytes32 mode, bytes calldata data) public virtual {
- require(_config[account].installed, ERC7579ExecutorModuleNotInstalled());
+ function schedule(address account, bytes32 salt, bytes32 mode, bytes calldata data) public virtual {
+ if (!_config[account].installed) revert ERC7579ExecutorModuleNotInstalled();
_validateSchedule(account, salt, mode, data);
(uint32 executableAfter, , ) = getDelay(account);
_scheduleAt(account, salt, mode, data, Time.timestamp(), executableAfter);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function schedule(address account, bytes32 salt, bytes32 mode, bytes calldata data) public virtual { | |
| require(_config[account].installed, ERC7579ExecutorModuleNotInstalled()); | |
| _validateSchedule(account, salt, mode, data); | |
| (uint32 executableAfter, , ) = getDelay(account); | |
| _scheduleAt(account, salt, mode, data, Time.timestamp(), executableAfter); | |
| } | |
| function schedule(address account, bytes32 salt, bytes32 mode, bytes calldata data) public virtual { | |
| if (!_config[account].installed) revert ERC7579ExecutorModuleNotInstalled(); | |
| _validateSchedule(account, salt, mode, data); | |
| (uint32 executableAfter, , ) = getDelay(account); | |
| _scheduleAt(account, salt, mode, data, Time.timestamp(), executableAfter); | |
| } |
🤖 Prompt for AI Agents
In contracts/account/modules/ERC7579DelayedExecutor.sol around lines 233 to 238,
the require call using a custom error (require(_config[account].installed,
ERC7579ExecutorModuleNotInstalled());) is invalid; replace it with an explicit
revert: check the condition and if false revert
ERC7579ExecutorModuleNotInstalled(); keeping the rest of the function logic
intact so _validateSchedule and subsequent calls execute only when installed is
true.
| function _execute( | ||
| address account, | ||
| bytes32 salt, | ||
| bytes32 mode, | ||
| bytes calldata executionCalldata | ||
| ) internal virtual override returns (bytes[] memory returnData) { | ||
| bytes32 id = hashOperation(account, salt, mode, executionCalldata); | ||
| _validateStateBitmap(id, _encodeStateBitmap(OperationState.Ready)); | ||
|
|
||
| _schedules[id].executed = true; | ||
|
|
||
| return super._execute(account, salt, mode, executionCalldata); | ||
| } |
There was a problem hiding this comment.
Parameter-order mismatch in _execute override breaks scheduling/execution IDs and emitted events.
The override flips salt and mode. This will cause false UnexpectedOperationState reverts and wrong event fields. Align with the base signature (account, mode, salt, executionCalldata).
- function _execute(
- address account,
- bytes32 salt,
- bytes32 mode,
- bytes calldata executionCalldata
- ) internal virtual override returns (bytes[] memory returnData) {
- bytes32 id = hashOperation(account, salt, mode, executionCalldata);
- _validateStateBitmap(id, _encodeStateBitmap(OperationState.Ready));
-
- _schedules[id].executed = true;
-
- return super._execute(account, salt, mode, executionCalldata);
- }
+ function _execute(
+ address account,
+ bytes32 mode,
+ bytes32 salt,
+ bytes calldata executionCalldata
+ ) internal virtual override returns (bytes[] memory returnData) {
+ bytes32 id = hashOperation(account, salt, mode, executionCalldata);
+ _validateStateBitmap(id, _encodeStateBitmap(OperationState.Ready));
+ _schedules[id].executed = true;
+ return super._execute(account, mode, salt, executionCalldata);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _execute( | |
| address account, | |
| bytes32 salt, | |
| bytes32 mode, | |
| bytes calldata executionCalldata | |
| ) internal virtual override returns (bytes[] memory returnData) { | |
| bytes32 id = hashOperation(account, salt, mode, executionCalldata); | |
| _validateStateBitmap(id, _encodeStateBitmap(OperationState.Ready)); | |
| _schedules[id].executed = true; | |
| return super._execute(account, salt, mode, executionCalldata); | |
| } | |
| function _execute( | |
| address account, | |
| bytes32 mode, | |
| bytes32 salt, | |
| bytes calldata executionCalldata | |
| ) internal virtual override returns (bytes[] memory returnData) { | |
| bytes32 id = hashOperation(account, salt, mode, executionCalldata); | |
| _validateStateBitmap(id, _encodeStateBitmap(OperationState.Ready)); | |
| _schedules[id].executed = true; | |
| return super._execute(account, mode, salt, executionCalldata); | |
| } |
🤖 Prompt for AI Agents
In contracts/account/modules/ERC7579DelayedExecutor.sol around lines 378 to 390,
the overridden _execute flips the order of `salt` and `mode`, causing wrong
operation IDs and events; change the function signature to match the base
signature (account, mode, salt, executionCalldata), update all uses inside
(_hashOperation/validateStateBitmap/_schedules keying and the super._execute
call) to use the corrected parameter order so the computed id, state checks,
schedule flagging, and emitted event fields align with the base implementation.
| function _validateStateBitmap(bytes32 operationId, bytes32 allowedStates) internal view returns (OperationState) { | ||
| OperationState currentState = state(operationId); | ||
| require( | ||
| _encodeStateBitmap(currentState) & allowedStates != bytes32(0), | ||
| ERC7579ExecutorUnexpectedOperationState(operationId, currentState, allowedStates) | ||
| ); | ||
| return currentState; |
There was a problem hiding this comment.
Custom error with require is invalid; revert explicitly.
Fixes a hard compile error and preserves the rich error data.
- require(
- _encodeStateBitmap(currentState) & allowedStates != bytes32(0),
- ERC7579ExecutorUnexpectedOperationState(operationId, currentState, allowedStates)
- );
+ if (_encodeStateBitmap(currentState) & allowedStates == bytes32(0)) {
+ revert ERC7579ExecutorUnexpectedOperationState(operationId, currentState, allowedStates);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _validateStateBitmap(bytes32 operationId, bytes32 allowedStates) internal view returns (OperationState) { | |
| OperationState currentState = state(operationId); | |
| require( | |
| _encodeStateBitmap(currentState) & allowedStates != bytes32(0), | |
| ERC7579ExecutorUnexpectedOperationState(operationId, currentState, allowedStates) | |
| ); | |
| return currentState; | |
| function _validateStateBitmap(bytes32 operationId, bytes32 allowedStates) internal view returns (OperationState) { | |
| OperationState currentState = state(operationId); | |
| if (_encodeStateBitmap(currentState) & allowedStates == bytes32(0)) { | |
| revert ERC7579ExecutorUnexpectedOperationState(operationId, currentState, allowedStates); | |
| } | |
| return currentState; | |
| } |
🤖 Prompt for AI Agents
In contracts/account/modules/ERC7579DelayedExecutor.sol around lines 417 to 423,
the use of a custom error as the third argument to require is invalid; replace
the require(...) call with an explicit if-check that reverts using the custom
error (e.g., if ((_encodeStateBitmap(currentState) & allowedStates) ==
bytes32(0)) revert ERC7579ExecutorUnexpectedOperationState(operationId,
currentState, allowedStates);) so the rich error data is preserved and the
contract compiles.
| await expect(this.mockFromAccount.$_execute(this.mockAccount.address, ethers.ZeroHash, this.mode, this.calldata)) | ||
| .to.emit(this.mock, 'ERC7579ExecutorOperationExecuted') | ||
| .to.emit(this.target, 'MockFunctionCalledWithArgs') | ||
| .withArgs(...this.args); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm $_execute parameter order matches the exposed signature.
Depending on the exposed harness, $_execute may take (account, mode, salt, calldata). If so, swap salt and mode.
If needed:
-await expect(this.mockFromAccount.$_execute(this.mockAccount.address, ethers.ZeroHash, this.mode, this.calldata))
+await expect(this.mockFromAccount.$_execute(this.mockAccount.address, this.mode, ethers.ZeroHash, this.calldata))🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exposed function signature
rg -nP "(function\\s+\\$?_execute\\s*\\(|\\$?_execute\\()"
rg -nP "_execute\\s*\\(" contracts/account/modules/ERC7579Executor.sol -n -C2 || trueLength of output: 72
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List available Solidity files in the module directory
find contracts/account/modules -maxdepth 1 -type f -name '*.sol'
# Search for the _execute function declaration with and without the dollar prefix
rg -nP 'function\s+\$?_execute\s*\(' -g 'contracts/account/modules/ERC7579Executor.sol' -n -C3 || true
rg -nP 'function\s+_execute\s*\(' -g 'contracts/account/modules/ERC7579Executor.sol' -n -C3 || trueLength of output: 1456
Swap mode and salt arguments in the test to match _execute signature
The _execute function in contracts/account/modules/ERC7579Executor.sol is declared as:
function _execute(
address account,
bytes32 mode,
bytes32 salt,
bytes calldata data
) …(lines 84–87) – it expects mode as the second argument and salt as the third. The test currently passes them in the opposite order, which will cause the emitted events check to run against the wrong values.
• File: test/account/modules/ERC7579Executor.test.js
• Lines: 66–70
Required change:
- await expect(this.mockFromAccount.$_execute(
- this.mockAccount.address,
- ethers.ZeroHash, // ← salt
- this.mode, // ← mode
- this.calldata
- ))
+ await expect(this.mockFromAccount.$_execute(
+ this.mockAccount.address,
+ this.mode, // ← mode
+ ethers.ZeroHash, // ← salt
+ this.calldata
+ ))
.to.emit(this.mock, 'ERC7579ExecutorOperationExecuted')
.to.emit(this.target, 'MockFunctionCalledWithArgs')
.withArgs(...this.args);Please swap the mode and salt parameters so the test aligns with the on-chain signature.
🤖 Prompt for AI Agents
In test/account/modules/ERC7579Executor.test.js around lines 66 to 70 the test
calls this.mockFromAccount.$_execute(this.mockAccount.address, ethers.ZeroHash,
this.mode, this.calldata) but the contract signature expects (address account,
bytes32 mode, bytes32 salt, bytes calldata data); swap the second and third
arguments so you pass mode then salt (i.e., this.mode then ethers.ZeroHash) to
align the test with the on-chain `_execute` signature and ensure the emitted
events/assertions check the correct values.
Smaller version of #5888 for executor modules
PR Checklist
npx changeset add)Summary by Sourcery
Introduce two new ERC-7579 executor modules for smart accounts: a basic executor for immediate calls and an advanced delayed executor that supports scheduling, delay periods, expiration windows, and cancellation of operations per the ERC-7579 specification.
New Features:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores