feat: migration to repair ERC-4337 config on Safes - #375
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a migration path to “repair” legacy Gnosis Safes that were deployed without the ERC-4337 module configuration, by detecting missing config (module enablement + fallback handler) and relaying a signed execTransaction through the backend.
Changes:
- Add
Safe4337ModuleProcessormigration that checks on-chain Safe state and relays a signed repairexecTransaction. - Add Safe-module helper utilities (calldata encoding, fallback handler slot constant, MultiSend bundle building).
- Extend the RPC client + test HTTP client to support
eth_getStorageAtandwa_relaySafeTransaction, plus add an end-to-end integration test.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| bedrock/tests/test_safe_4337_module_processor.rs | New E2E test validating detection + relayed repair + post-state checks. |
| bedrock/tests/common.rs | Refactors Safe deployment helper; adds a “without 4337 module” deployment path for tests. |
| bedrock/src/transactions/rpc.rs | Adds EthGetStorageAt + RelaySafeTransaction RPC method support and client helpers. |
| bedrock/src/transactions/contracts/safe_module.rs | New helpers for checking/installing the 4337 module and reading fallback handler slot. |
| bedrock/src/transactions/contracts/mod.rs | Exposes the new safe_module helpers module. |
| bedrock/src/test_utils.rs | Extends AnvilBackedHttpClient to simulate eth_getStorageAt + wa_relaySafeTransaction. |
| bedrock/src/migration/processors/safe_4337_module_processor.rs | New migration processor implementing the Safe 4337 repair flow. |
| bedrock/src/migration/processors/mod.rs | Registers/exports the new migration processor module. |
| bedrock/src/migration/mod.rs | Re-exports Safe4337ModuleProcessor. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 346cd1b713
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
aurel-fr
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3f5484c. Configure here.
| processors | ||
| .push(Arc::new(Permit2ApprovalProcessor::new(account.clone()))); | ||
| processors | ||
| .push(Safe4337ModuleProcessor::new(account).as_migration_processor()); |
There was a problem hiding this comment.
Permit2 runs parallel to repair
Medium Severity
Registering Safe4337ModuleProcessor alongside Permit2ApprovalProcessor while run_migrations_async still runs all processors in parallel means legacy Safes attempt Permit2 user operations before the 4337 repair can finish in the same run, so Permit2 fails even when the repair would succeed moments later.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3f5484c. Configure here.
There was a problem hiding this comment.
Migration processor has been built this way. It will self-heal on next app open and we can consider making the migration sequential in a future PR.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
|
||
| impl Safe4337ModuleProcessor { | ||
| /// Reads the Safe's ERC-4337 configuration on-chain and returns the repairs | ||
| /// needed (module enablement and/or fallback handler). |
There was a problem hiding this comment.
let me make sure the backend checks for this - i think it was assuming both steps
| } | ||
|
|
||
| async fn is_applicable(&self) -> Result<bool, MigrationError> { | ||
| // Always attempt — the on-chain check lives in `execute`, which is the |
There was a problem hiding this comment.
the check logic should be in here not execute.
execute is only called if this function returns true
There was a problem hiding this comment.
This is on purpose. Let's assume a user needs repair. On first try the execute will send the repair and forward to tx-sitter. The transaction may take time to mine or it could fail even. So we don't mark the processor as executed successfully. On next app open the execute will trigger again, and if the previous tx mined it will exit early and mark as successful. The reason we do this dance is because only execute can return a ProcessorResult, is_applicable just return a boolean.
There was a problem hiding this comment.
this is fine - the is_applicable should always be checking the chain state as the source of truth instead of the txId/txHash returned
the migrations only apply once on cold start so we dont really need to worry about them running multiple times
There was a problem hiding this comment.
I'm not following. The migration processor does check chain state let repairs = self.fetch_repairs().await?;. It is the source of truth.
| // configured (e.g. a repair relayed on a previous run has since mined), | ||
| // report success; otherwise relay the repair and stay retryable so the | ||
| // next run confirms it. | ||
| let repairs = self.fetch_repairs().await?; |
Adds a migration that enables the 4337 module and sets it as fallback handler via a relayed execTransaction.