Add Safe 4337 module migration helpers + fallback-handler reader#191
Open
Sednaoui wants to merge 11 commits into
Open
Add Safe 4337 module migration helpers + fallback-handler reader#191Sednaoui wants to merge 11 commits into
Sednaoui wants to merge 11 commits into
Conversation
Migrating a deployed Safe between EntryPoint versions means swapping its 4337 module and fallback handler. The only built-in helper covered v0.6 -> v0.7; there was no reusable path to v0.9 and no way to read a Safe's current fallback handler. - SafeAccount.createModuleMigrationMetaTransactions(node, oldModule, newModule, overrides): generic [disableOld, enableNew, setFallbackHandler] builder. Both Safe4337Module and Safe4337MultiChainSignatureModule are stateless, so no storage clearing is needed. - SafeAccountV0_3_0.createMigrateToSafeMultiChainSigAccountV1MetaTransactions: convenience wrapper for the v0.7 -> v0.9 path. - SafeAccountV0_2_0.createMigrateToSafeAccountV0_3_0MetaTransactions now delegates to the generic helper (removes duplicated body). - SafeAccount.getFallbackHandler(node): reads the fallback handler (the 4337 module) so callers can confirm which EntryPoint version an account is on. - JsonRpcNode.getStorageAt(address, slot, blockTag): eth_getStorageAt. - Export SAFE_FALLBACK_HANDLER_STORAGE_SLOT constant.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SAFE_FALLBACK_HANDLER_STORAGE_SLOT and JsonRpcNode.getStorageAt; implements SafeAccount.getFallbackHandler and createModuleMigrationMetaTransactions; updates V0_2_0 to delegate to the helper; adds V0_3_0 migration helper; re-exports the storage slot; and adds unit and e2e tests. ChangesSafe Module Migration Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SafeAccount
participant JsonRpcNode
Caller->>SafeAccount: createModuleMigrationMetaTransactions(nodeRpcUrl, oldModule, newModule, overrides)
SafeAccount->>JsonRpcNode: getStorageAt(safeAddress, SAFE_FALLBACK_HANDLER_STORAGE_SLOT)
JsonRpcNode-->>SafeAccount: fallback handler storage word
SafeAccount->>SafeAccount: build disable-old-module MetaTx
SafeAccount->>SafeAccount: build enable-new-module MetaTx
SafeAccount->>SafeAccount: build setFallbackHandler(newModule) MetaTx
SafeAccount-->>Caller: [disableTx, enableTx, setFallbackTx]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/account/Safe/SafeAccountV0_2_0.ts`:
- Around line 382-390: The prevModuleAddress argument passed into
createModuleMigrationMetaTransactions is incorrectly wired to
overrides.safeV06ModuleAddress (the module being disabled) instead of the
linked-list predecessor; update the call so prevModuleAddress is set to the
predecessor value (e.g. overrides.prevModuleAddress or the explicit predecessor
field provided by overrides) rather than overrides.safeV06ModuleAddress,
ensuring createModuleMigrationMetaTransactions and any downstream disableModule
call receive the actual previous-module address.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 233c1326-568f-4648-a4f7-8070eade1b52
📒 Files selected for processing (6)
src/abstractionkit.tssrc/account/Safe/SafeAccount.tssrc/account/Safe/SafeAccountV0_2_0.tssrc/account/Safe/SafeAccountV0_3_0.tssrc/constants.tssrc/transport/JsonRpcNode.ts
createMigrateToSafeAccountV0_3_0MetaTransactions passed overrides.safeV06ModuleAddress (the module being disabled) as prevModuleAddress (the linked-list predecessor). When a caller set safeV06ModuleAddress explicitly, this produced disableModule(prev=module, module) — claiming the module precedes itself. Default callers were unaffected (undefined -> on-chain predecessor lookup). Add a proper prevModuleAddress override and stop mis-wiring it; the predecessor is still looked up on-chain when not provided.
…geAt Unit (offline, deterministic): - test/safe/moduleMigration.test.js: createModuleMigrationMetaTransactions (selectors/targets/order, explicit predecessor, and on-chain predecessor lookup via mock transport), createMigrateToSafeMultiChainSigAccountV1- MetaTransactions (defaults + overrides), getFallbackHandler, and a regression for the v0.6->v0.7 prevModuleAddress wiring (prev != module). - test/transport/JsonRpcNode.test.js: getStorageAt params, block tag, and BAD_DATA on non-string. Integration (e2e, cross-EntryPoint): - test/integration/e2e/migrate-safe-v07-to-v09/migrate.test.js: deploy a SafeAccountV0_3_0 on EP v0.7, migrate to the v0.9 multi-chain module, assert the on-chain module/fallback-handler swap, then execute a userop through SafeMultiChainSigAccountV1 on EP v0.9.
sherifahmed990
requested changes
May 31, 2026
| * @param overrides - overrides for finding the previous module | ||
| * @returns a promise of [disableOld, enableNew, setFallbackHandler] MetaTransactions | ||
| */ | ||
| public async createModuleMigrationMetaTransactions( |
Member
There was a problem hiding this comment.
I suggest making this function protected
| modulesStart?: string; | ||
| modulesPageSize?: bigint; | ||
| } = {}, | ||
| ): Promise<MetaTransaction[]> { |
Member
There was a problem hiding this comment.
I suggest verifying the safe version is compatible with the target Class
Member
Author
There was a problem hiding this comment.
I will add a check for minimum version
Before building a module-migration batch, verify on-chain that the account
is actually a Safe running the old 4337 module — the module is enabled AND
is the current fallback handler — and that its Safe version meets the module
minimum (>= 1.4.1). This turns a cryptic on-chain AA23/AA24 into a clear
up-front error. Opt out with { skipPreflight: true }.
- SafeAccount.getSafeVersion(node): reads VERSION().
- SafeAccount.createModuleMigrationMetaTransactions: runs the preflight unless
skipped; threaded through the v0.6->v0.7 and v0.7->v0.9 wrappers.
- Note: an equality check against the target class's singleton would be wrong
(it would reject valid 1.5.0 Safes); the modules only need >= 1.4.1, so the
check is a minimum, not a match.
Tests cover the preflight pass/fail cases, skipPreflight bypass, getSafeVersion,
and getFallbackHandler.
dev's ethers-minimization dropped the AbiCoder import from SafeAccount.ts; getSafeVersion now decodes VERSION() via the local decodeAbiParameters helper (matching isModuleEnabled/getModules), fixing a runtime ReferenceError that the bundler build did not catch.
…s' into add-safe-module-migration-helpers
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/account/Safe/SafeAccount.ts`:
- Around line 3158-3174: The preflight checks can leak raw RPC/ABI errors from
isModuleEnabled(node, oldModuleAddress) and getSafeVersion(node) or pass through
empty/invalid VERSION() results; catch any thrown errors or invalid/empty
versions around those calls (references: isModuleEnabled, getSafeVersion,
SafeAccount.isVersionAtLeast, MIN_SAFE_4337_VERSION) and rethrow a normalized
AbstractionKitError("BAD_DATA", ...) that includes a clear message referencing
the Safe (this.accountAddress), the module (oldModuleAddress) and the
skipPreflight hint; ensure you wrap both the module-enabled check and the
version fetch/compare in try/catch and treat empty/invalid version responses as
failing the preflight so the same BAD_DATA error is emitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e324101a-4a0f-4d1a-8a0c-7c4ddaf487da
📒 Files selected for processing (4)
src/account/Safe/SafeAccount.tssrc/account/Safe/SafeAccountV0_2_0.tssrc/account/Safe/SafeAccountV0_3_0.tstest/safe/moduleMigration.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/account/Safe/SafeAccountV0_3_0.ts
- src/account/Safe/SafeAccountV0_2_0.ts
- test/safe/moduleMigration.test.js
The migration preflight already wrapped getFallbackHandler, but a raw RPC/ABI
error from isModuleEnabled or getSafeVersion (e.g. a reverting VERSION() on a
non-Safe), or an empty/invalid version string, could leak through unnormalized.
Wrap both calls in try/catch and treat empty/invalid versions as a failed
preflight, rethrowing a clear AbstractionKitError("BAD_DATA", ...) that names
the Safe, the module, and the skipPreflight hint.
It's the shared implementation behind the version-specific migration helpers, which pin the correct module addresses. Marking it protected steers developers to those wrappers instead of supplying raw old/new module addresses directly (easy to get wrong). Subclass wrappers still call it via `this`. Tests now exercise the shape and predecessor lookup through the public wrapper.
…7 fix Document under [UNRELEASED]: the Safe v0.7->v0.9 migration helper with opt-out preflight, the getFallbackHandler/getSafeVersion/getStorageAt readers and the SAFE_FALLBACK_HANDLER_STORAGE_SLOT export, and the prevModuleAddress fix in the v0.6->v0.7 migration helper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds first-class support for migrating a deployed Safe between ERC-4337 module/EntryPoint versions, plus a way to read a Safe's current fallback handler. Surfaced while building a v0.7→v0.9 Safe migration example — the only existing helper covered v0.6→v0.7 (on
SafeAccountV0_2_0), there was no reusable path to v0.9, and no reader for the fallback handler.Changes
SafeAccount.createModuleMigrationMetaTransactions(nodeRpcUrl, oldModule, newModule, overrides)— generic builder returning[disableOld, enableNew, setFallbackHandler]. For Safe 4337 accounts the module is both the enabled module and the fallback handler, so that's the whole migration. Documented that no storage clearing is required: bothSafe4337ModuleandSafe4337MultiChainSignatureModuleare stateless.SafeAccountV0_3_0.createMigrateToSafeMultiChainSigAccountV1MetaTransactions(nodeRpcUrl, overrides)— convenience wrapper for the common v0.7 → v0.9 path.SafeAccountV0_2_0.createMigrateToSafeAccountV0_3_0MetaTransactionsnow delegates to the generic helper (removes the duplicated disable/enable/setFallback body; behavior preserved).SafeAccount.getFallbackHandler(nodeRpcUrl)— reads the fallback handler (i.e. the active 4337 module) so callers can confirm which EntryPoint version an account is on after a migration.JsonRpcNode.getStorageAt(address, slot, blockTag)—eth_getStorageAt.SAFE_FALLBACK_HANDLER_STORAGE_SLOT(keccak256("fallback_manager.handler.address")).Verification
npm run buildclean;184/184unit tests pass.getFallbackHandler()returns the v0.9 module, and the helper produces the correct 3-tx batch (disableModule/enableModule/setFallbackHandler).Notes
The existing v0.6→v0.7 migration is now a thin wrapper over the generic helper — no public API removed or renamed; this is purely additive plus an internal de-duplication.
Summary by CodeRabbit
New Features
Tests