feat(migration): track fire-and-forget userOp hashes and harden the retry loop - #408
Conversation
…etry loop The fire-and-forget migration flow (ProcessorResult::Pending) previously discarded the submitted userOp hash, which made a reverting transaction invisible (record stayed InProgress with no error) and allowed duplicate submissions when the app reopened before the transaction mined. - ProcessorResult::Pending now carries the userOp hash, persisted on MigrationRecord (serde-default for backward compatibility) and exposed via MigrationRecordEntry for triage - New MigrationProcessor::check_pending_work resolves the previous submission before re-executing: StillPending skips the run (no duplicate submission), Reverted records MINED_REVERT and retries, Mined falls through to the is_applicable end-state recheck - Reverts are capped: after MAX_MINED_REVERTS (3) the migration goes FailedTerminal instead of resubmitting on every app open - TTL-expired Succeeded migrations whose is_applicable errors renew recheck_at with a short retry TTL (1 day) instead of rechecking on every app open during an RPC outage - Documented the strengthened is_applicable contract for previously attempted migrations; extracted mark_succeeded so the execute() and recheck promotion paths stay in lockstep - migration_run.completed appends pending= after pre-existing fields so positionally-keyed log parsers keep working - Integration test drives the real controller through submit -> dropped transaction (anvil evm_revert) -> re-execute -> promote; requires simple nonce management and widening InMemoryDeviceKeyValueStore to the test_utils feature Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9bdecd6 to
6fb94ff
Compare
Removes the userOp receipt check from the migration flow. A migration completes when is_applicable() reports the end state holds; an unmet end state is simply re-submitted. This fixes a permanent stall: a userOp that was dropped rather than mined or reverted returned StillPending forever, skipping re-execution on every launch. Deletes check_pending_work, PendingWorkStatus and revert_count. Collapses the not-applicable branch to two arms, and caches never-needed migrations as Succeeded so recheck_at is the only trigger rather than an RPC read on every launch. Giving up is now split. The controller counts only submissions confirmed mined that left the end state unmet, so a dropped userOp cannot push a working migration terminal. Safe4337ModuleProcessor relays a Safe tx and gets an internal transaction id back, which no receipt lookup can resolve, so it keeps its own attempt cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
run_single_processor was 334 lines doing four jobs. No behaviour change; the existing 66 tests were the safety net. - The four execute-outcome arms were near-identical: each computed a duration, logged, set status + error fields + hash, and built a one-field summary. They now yield (status, error, hash) and share one block, which also collapses the repeated hash clearing to one line. - MigrationRunSummary gains one-field constructors, replacing 12 struct literals. - The storage-error block was copy-pasted three times; now one helper. - Extracted should_attempt, record_end_state_met and count_failed_landing, leaving the main function a flat sequence of guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Renames MAX_FAILED_LANDINGS to MAX_REVERTS and lowers the cap from 5 to 3. The counter field goes back to its original revert_count, which drops the serde alias since the persisted key is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR refactors the migration controller to treat on-chain state (is_applicable()) as the only oracle of completion, switching migrations to a fire-and-forget submission model (no receipt polling in the decision path) while adding bounded retry caps and better diagnostics via persisted submission references.
Changes:
- Introduces
ProcessorResult::Pending { user_op_hash }and updates processors/controller to persist (but not gate on) submission hashes/ids and re-run untilis_applicable()reports the end state holds. - Hardens retry/terminal behavior by counting only mined-but-still-unmet submissions toward a controller-level
MAX_REVERTS, and adds a separateMAX_ATTEMPTScap for the Safe 4337 module migration. - Expands tests and documentation to cover the new lifecycle, including resubmission when a previously-submitted transaction is dropped.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| bedrock/src/migration/controller.rs | Implements fire-and-forget lifecycle, pending summaries, mined-only revert counting, and helper refactors. |
| bedrock/src/migration/processor.rs | Adds Pending to ProcessorResult and documents the “completion via is_applicable” contract. |
| bedrock/src/migration/state.rs | Extends MigrationRecord with pending_user_op_hash and mined-failure revert_count. |
| bedrock/src/migration/processors/permit2_approval_processor.rs | Switches Permit2 approvals to submit-and-return-Pending (no receipt polling). |
| bedrock/src/migration/processors/safe_4337_module_processor.rs | Adds KV-backed attempt cap, real on-chain applicability check, and returns Pending with relay id. |
| bedrock/src/primitives/key_value_store.rs | Makes InMemoryDeviceKeyValueStore available under feature = "test_utils" for integration tests. |
| bedrock/src/migration/README.md | Updates documentation/state diagram to match the on-chain-oracle + fire-and-forget model. |
| bedrock/tests/test_permit2_approval_processor.rs | Updates test to validate Pending behavior and controller-driven resubmission after a dropped tx. |
| bedrock/tests/test_safe_4337_module_processor.rs | Updates constructor usage to pass an in-memory KV store. |
Suppressed comments (3)
bedrock/src/migration/controller.rs:496
- Severity P2: Same as above: avoid
error=in log message key/value text; useerror_message=(or structured fields) so log pipelines don’t treat this as a reserved error attribute.
MigrationStatus::FailedRetryable => crate::warn!(
"migration.failed_retryable id={} attempt={} duration_ms={} error={:?} timestamp={}",
migration_id, record.attempts, duration_ms, error, Utc::now().to_rfc3339()
),
bedrock/src/migration/controller.rs:728
- Severity P2:
storage_failurelogs useerror=in the message text. Rename toerror_message=(or emit structured fields) to avoid reserved/ambiguous log attributes and improve queryability.
crate::error!(
"migration.storage_error error={:?} timestamp={}",
e,
Utc::now().to_rfc3339()
);
bedrock/src/migration/controller.rs:585
- Severity P2: When the revert cap is hit in
count_failed_landing, the record is markedFailedTerminalbutpending_user_op_hashis left populated. That field is described as a reference to outstanding work, so keeping it on a terminal record is inconsistent and can confuse diagnostics/UIs.
record.status = MigrationStatus::FailedTerminal;
record.last_error_code = Some("LANDING_FAILED".to_string());
record.last_error_message = Some(format!(
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1e433e5bc
ℹ️ 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".
Success marks a migration done on the processor's word alone, before anything confirmed it on-chain, which is the opposite of the rule the rest of this PR establishes. Processors now return Pending in every case and let is_applicable prove completion on the next run. The variant stays on ProcessorResult (removing it is a uniffi change) and is documented as unused. Also corrects two integration assertions that still expected Success/Retryable from the Safe 4337 processor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A Pending{None} result leaves the record InProgress without a hash, so
the next run logged migration.submission_did_not_land about a submission
that was never made. Nothing was miscounted (submission_mined is never
called without a hash), but the log was wrong. Return early instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
P1 —
Either preserve the prior state for non-completion skips, or coordinate consumer changes so errors propagate rather than returning P1 — This cap counts submissions, not confirmed failures. If the app opens five times before the relayer mines any request, the sixth run observes work still needed and permanently records Keep the migration retryable/recheckable after the cap, or terminalize only confirmed failures. P2 —
Provide a compatible API path or treat this as a breaking SDK release. Discussed in person too - I think the cleanest thing would just to be discard this PR change and update |
The FFI `MigrationProcessor` trait was serving two populations with
different notions of completion. A foreign processor reports its own
success and is believed; an on-chain migration cannot, since the only
proof its work landed is a later observation of the chain. Squeezing the
second into the first is what forced `ProcessorResult::Success` to be
documented as "do not use", grew `MigrationRecord` two fields no foreign
processor reads, and made `Safe4337ModuleProcessor` keep a private
attempt counter because it did not fit the shared cap.
Bedrock's own migrations now have their own Rust-only framework beside
the FFI one. `MigrationController` runs both under the existing lock and
merges the summaries; the FFI surface is byte-identical to main apart
from one additive `pending` field on `MigrationRunSummary`.
A wallet migration defines two phases and nothing else:
async fn end_state_holds(&self) -> Result<bool, MigrationError>;
async fn submit(&self) -> Result<Reconciled, MigrationError>;
The controller wires them together, so the check that decides "is there
work to do" exists in exactly one place and cannot be reordered or
restated. `Reconciled` has no success variant, so "completion is proven
by observation, never by a submission" is a type rather than a comment.
This replaces `is_applicable` + `execute`, where the first stashed its
result on `self` for the second to read. `Permit2ApprovalMigration` loses
its `Mutex<Vec<(Address, &str)>>` and `Safe4337ModuleMigration` loses the
duplicate chain read it did because it distrusted that hand-off.
Ordering: the 4337 repair relays an owner-signed `execTransaction`, so it
is the only migration that works on a Safe which cannot yet validate a
userOp. It runs first and alone; dependents run only once it has
converged — an observation, never a submission — so the cold start that
relays the repair does not also run them.
Giving up: MAX_ATTEMPTS counts submissions observed to have failed, per
gap. Once spent, the next pass calls `end_state_holds` alone and never
submits, so a migration is never written off on the strength of a
transaction nobody watched. Converging resets the count. A one-hour
cooldown keeps repeated cold starts from burning the cap before the
first submission can mine — there was no such cooldown before.
The on-chain execution path is unchanged: `build_signed_transaction` is
byte-identical, as are the allowance read, `sign_and_execute` and
`relay_safe_transaction` calls.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the wallet migration split. `WalletMigration` keeps two methods, but they are alternatives rather than halves. `reconcile` observes once and reuses the value, and is the normal path. `end_state_holds` is a pure read used only when the give-up cap is spent — the one case where the controller must know without acting. The two call sites are mutually exclusive, so no pass ever reads the chain twice, and no migration needs to cache an observation on `self`. Both migrations now name their private observe method `observe` and read identically: observe once, check the gap, submit. `build_signed_transaction` returns the request rather than an Option. Its `None` arm was dead — `reconcile` returns Converged before it can be reached — and reading it as convergence claimed the migration was done without having observed anything. An empty bundle is now a caller error. Deletes `migration/processors/`. `ExampleProcessor` was unreachable (private module inside a public one), referenced nowhere, alive only via `#[allow(dead_code)]`, and documented a `ProcessorResult` variant that does not exist. Its audience writes Swift and Kotlin, so the trait's own docs are the template. The module doc pointed at that folder and at `default_processors()`, neither of which still exists. It is now a short pointer, and the README explains fire-and-forget, why on-chain state is the only oracle, and that wallet migrations cover on-chain wallet operations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three changes, all shrinking the surface. **Always read the chain.** A converged migration no longer waits out a 30-day TTL before looking again, so `SUCCESS_TTL_DAYS`, `recheck_at` and the RPC-outage backoff all go with it. What gets held back is *submitting*, never looking: `still_settling` gates only the resubmit, and a pass that may not submit calls `end_state_holds` instead of `reconcile`. Skipping the observation was how the resubmit used to be throttled, which is why those three concepts existed at all. The result is strictly more responsive — a submission that lands is seen on the next launch rather than up to an hour later, and a state that drifts back is caught immediately rather than up to 30 days later — for one chain read per migration per launch. A migration that has given up is the only one that reads nothing. `settle_capped` becomes `observe_only`, covering both reasons to look without acting: the cap is spent, or a submission has not had its hour. **Shared `RecordStore`.** Both controllers had the same namespaced load/save/delete over `DeviceKeyValueStore`, including the same corrupt-data-is-a-reset handling, differing only in record type and key prefix. Now one generic store; the prefix is what keeps the two frameworks' records apart. **Integration coverage for `WalletMigrationController`.** The ordering rule had only unit coverage with doubles. `test_wallet_migration_controller` drives the real controller against anvil with a Safe deployed without the 4337 module, and asserts that launch 1 relays the repair while Permit2 does not run or submit, that launch 2 observes the repair and unblocks it in the same pass, and that launch 3 settles both with nothing submitted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
resolved the older comments, comments aren't relevant anymore since the controller refactor |
The wallet framework had its own copies of the record and status types that
mapped 1:1 onto the native ones, so both are gone: `MigrationStatus` and
`MigrationRecord` are now shared, and `record_store.rs` owns them alongside
`MigrationRecordEntry` and the store itself. `state.rs` is deleted.
`MigrationRecord::into_entry` replaces the entry construction that was
duplicated verbatim in both controllers.
Wallet logs move to structured attributes and drop 11 hand-rolled
`timestamp={}` fields — `timestamp` is reserved and already supplied by the
backend.
Drops `WalletMigrationResult::GiveUp`, which no migration ever returned, and
renames `Reconciled` to `WalletMigrationResult` to match `ProcessorResult`.
`MigrationProcessor` is untouched: it is FFI-exported. Generated bindings are
unchanged — same uniffi type names, same variants, same fields.
Inline comments capped at 3 lines throughout; the README keeps the long form.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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 d9b19d5. Configure here.
Neither framework bounds how long a migration may take: the only migration timeout in the crate is Turnkey's, an unrelated system. The run is off the app-start path but holds the migration lock, so a slow pass stalls the rest. Also renames the bare `error` log key, which collides with the backend's reserved `error.*` namespace. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9b19d5d3d
ℹ️ 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".
Two ways a wallet migration could stall silently, both from measuring the
resubmit cooldown off `last_attempted_at`, which every pass bumps — including
passes that only observed:
- A user opening the app more than once an hour pushed the deadline forward
forever, so a dropped userOp was never resubmitted and the cap was never
spent. The migration sat `InProgress` indefinitely, blocking dependents.
- The submission that spent the cap was written off on the very next launch,
seconds after going out. For the 4337 repair that blocked every dependent
permanently.
`last_submission` becomes a `Submission { reference, at }`, so the cooldown is
measured from when the work actually went out. Giving up now also requires the
last submission to be past its grace period.
Both paths get a regression test; each fails against the old logic.
Also converts the 4337 relay failure to `Retry` instead of `?`, matching the
Permit2 migration: an outer `Err` read as a failed observation, so the run
reported `skipped` and the record never explained why the repair was stuck.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c37c7aefca
ℹ️ 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".
`WalletMigrationController::run` was re-exported at the crate root, where it bypasses the process-wide lock `MigrationController::run_migrations` holds: two direct callers could load the same record and submit the same work twice. It is now `pub(crate)`, re-exported as `TestWalletMigrationController` only under the `test_utils` feature so the integration tests keep their entry point. Also fixes the Permit2 integration test's cooldown helper, which still rewound `last_attempted_at` and so no longer released the cooldown — the test asserted attempt 2 and got 1. And makes the 4337 relay log structured, matching the rest of the wallet logs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The type shipped in v0.6.1 as a `uniffi::Object` with an exported constructor and a root re-export, so deleting it would have broken any Swift, Kotlin, or Rust consumer referencing it. It comes back as an inert, deprecated shim with the identical exported surface — same `new`, same `as_migration_processor`, same `processors::safe_4337_module_processor` path. Registering it does nothing: `is_applicable` returns false and warns. The repair is a wallet migration the controller runs itself. Its `migration_id` is deliberately suffixed so a consumer that still registers it cannot collide with the record the controller owns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0288beff53
ℹ️ 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".

What
Splits migrations into two frameworks and makes on-chain state the only oracle for whether Bedrock's own migrations completed.
MigrationProcessor, implemented in Swift/Kotlin by the platform teams. Logically unchanged by this PR.WalletMigration, Rust-only, for on-chain work on the user's Safe. New in this PR.A wallet migration submits and returns immediately. There is no receipt lookup, no polling, and nothing waited on. It is complete when a later pass observes the end state.
Why
The receipt-based flow could stall permanently.
check_pending_workreturnedStillPendingfor a userOp the bundler evicted — no receipt ever appears for one that was dropped. Every launch re-read the same hash, gotStillPending, and returned before re-executing. The migration satInProgressforever, silently.Tracing the four receipt outcomes shows the check never changed the decision:
Every branch ends at the same recheck, so the receipt only decorated a log line.
The wallet framework
Two methods, and the gap is always a local — never stashed on
self:WalletMigrationResulthas no success variant —Converged,Submitted,Retry. The rule is a type, not a doc comment.A healthy launch calls
reconcileonly, so no pass ever reads the chain twice.end_state_holdsruns only when submitting is held back: the cap is spent, or a submission is still settling.Ordering
Safe4337ModuleMigrationis a prerequisite, not a peer. It relays an owner-signedexecTransaction, so it is the only migration that works on a Safe that cannot yet validate a userOp. It runs first and alone; dependents run only once it has been observed converged.The two frameworks run concurrently with
futures::join!; each fans out internally withjoin_all.Giving up, and the cooldown
MAX_ATTEMPTS = 5, counted per gap. Only accepted submissions count, so no number of offline launches exhausts it. Converging resets the count.RESUBMIT_COOLDOWN_HOURS = 1, measured from when the submission actually went out (Submission.at), never from the last pass. Giving up also requires the last submission to be past that window.Every launch reads the chain except one that has already given up. What gets held back is submitting, never looking — so work that landed is noticed on the next launch, and a state that drifts back is caught immediately.
Native migrations: what changed
No logic. The native path —
run_migrations,run_single_processor,load_record/save_record, therecheck_atTTL, the process-wide lock, and theMigrationProcessortrait signatures — is untouched. What did change:MigrationRecord,MigrationStatus, andMigrationRecordEntrymoved intorecord_store.rs, which now owns the record, its FFI view, and its persistence.state.rsis deleted.list_all_recordsuses the newMigrationRecord::into_entry, replacing a 9-line construction that was duplicated in both controllers.Compatibility
FFI surface:
MigrationRecordEntryis byte-identical tomain— same eight fields, same order.MigrationStatuskeeps the same five variants in the same order.MigrationProcessorandProcessorResultare untouched.Three consumer-visible changes, all in the Rust API rather than the generated bindings except the first:
MigrationRunSummarygains apendingfield. It is auniffi::Record, so the generated Swift/Kotlin struct gains a field — additive for readers, breaking only for code that constructs one.Safe4337ModuleProcessorand theprocessorsmodule are removed; that work is nowSafe4337ModuleMigration, registered automatically. It shipped in v0.6.1 but the controller was the only constructor.WalletMigrationController,WalletMigration,WalletMigrationResult,Submission.Persistence: old records still deserialize.
recheck_atandlast_submissionare#[serde(default)], and a corrupt or missing record reads as a reset — which costs one reconcile pass, since the chain is the oracle.Corrected claims
errorlog key collided with the backend's reservederror.*namespace; wallet logs now use structured attributes throughout and no longer hand-rolltimestamp, which the backend supplies.Tests
71 unit tests in the migration module, plus three integration tests against an Anvil fork of WorldChain with real userOps:
test_repair_runs_alone_then_unblocks_the_rest— on a Safe missing the 4337 module: launch 1 relays the repair and holds Permit2 back (asserting the allowance is still zero); launch 2 observes the repair, converges it, and submits the approvals; launch 3 settles both.test_safe_4337_module_migration_full_flowtest_permit2_approval_migration_full_flowTwo regression tests cover the stall modes fixed in review — each fails against the previous logic:
test_observe_only_passes_do_not_extend_the_cooldowntest_the_capping_submission_gets_its_grace_periodNotes
src/migration/README.mddocuments the model, the state table, and a mermaid flow diagram.Note
High Risk
Changes how wallet setup runs at app start (on-chain submissions, ordering, and failure/terminal behavior) and alters
MigrationRunSummaryfor UniFFI consumers; mistakes could stall or duplicate userOps.Overview
Introduces a Rust-only wallet migration framework alongside unchanged FFI native
MigrationProcessorflows, both orchestrated byMigrationController::run_migrationsunder one lock with merged summaries (including a newpendingcount for in-flight submissions).Wallet migrations (
WalletMigration:reconcile+end_state_holds) submit Permit2 approvals and Safe 4337 repairs without waiting on receipts; completion is decided only when a later launch re-reads chain state.WalletMigrationControllerenforces prerequisite ordering (4337 repair first, then parallel dependents), resubmit cooldown, attempt caps, andmigration:wallet:record keys via sharedRecordStore.Moves the real on-chain work from
Permit2ApprovalProcessor/Safe4337ModuleProcessorintowallet/migrations registered automatically whensafe_accountis set; default processors are removed fromMigrationController::new. Legacy processor types remain as inert FFI shims where needed. Docs and tests expand around the new state machine and Anvil integration coverage.Reviewed by Cursor Bugbot for commit 0288bef. Bugbot is set up for automated code reviews on this repo. Configure here.