fix(services): implement 3-phase write-ahead log (WAL) for key rotation

[justjoolz]

Key Rotation: 3-Phase Expand → Verify → Collapse (with WAL recovery)

Description

This PR restructures the key rotation lifecycle to remove a class of race condition that can leave an account with a revoked old key but no usable new key.

The existing atomic addAndRevokeKeys flow adds the new key and revokes the old one in a single transaction. If anything goes wrong with the new key after that transaction succeeds — storage write failure, an interrupted process, an unusable key — there is no fallback, and the account can lose signing access.

Approach

The rotation is decomposed into three phases so the old key is never revoked until the new key is proven to work:

1. Expand — add the new key on-chain. The old key stays active; the account temporarily holds two valid keys.
2. Verify — sign a test payload with the new key and confirm the returned public key and signature match. This proves the new key is stored correctly and can produce valid signatures.
3. Collapse — only after verification succeeds, revoke the old key(s).

If any phase fails, the old key remains active and the account is never locked out.

Write-Ahead Log

A durable pending-rotation marker is written through the platform storage bridge before each state transition. On cold boot, reconcilePendingRotation cross-references the marker against on-chain key state and either resumes, finalizes, or (after a staleness window) clears it — so an interrupted rotation self-heals rather than orphaning local state. The marker methods are optional on the bridge, so platforms that don't implement them degrade to a no-op.

Testing

Eight unit tests (packages/services/tests/key-rotation-3phase.test.ts) cover:

• Happy path Expand → Verify → Collapse
• Phase 2 verification failure (wrong key / no signature) — old keys remain active
• Phase 1 and Phase 3 on-chain failures — no unsafe revocation
• WAL phase progression ordering
• Post-revoke local cleanup failure not masking a completed rotation
• Reconciliation keeping pending state when revoke is still required

Developed independently against the public source.

[github-actions]

PR Summary

Restructured the key rotation lifecycle to eliminate race conditions that could leave accounts locked out. The previous atomic addAndRevokeKeys flow revoked the old key before verifying the new key worked, risking permanent loss of signing access.

The new architecture decomposes rotation into three distinct phases:

1. Expand — Add the new key on-chain while keeping the old key active
2. Verify — Sign a test payload with the new key to prove it works
3. Collapse — Only revoke old keys after mathematical proof of new key functionality

A Write-Ahead Log (WAL) persists pending rotation state through the platform storage bridge before each phase transition. On cold boot, reconcilePendingRotation cross-references the marker against on-chain key state to either resume, finalize, or clear stale entries after a 5-minute staleness window. The WAL methods are optional on the bridge for backward compatibility.

Added new Cadence transactions add_keys.cdc and revoke_keys.cdc to support the separated phases, along with comprehensive unit tests covering happy path, phase failures, WAL progression, and reconciliation scenarios.

Changes

File	Summary
PLAN.md	New file documenting the Expand & Collapse architecture proposal. Explains the problem with atomic rotation, details the 3-phase approach (Expand → Verify → Collapse), provides Cadence transaction examples, and includes a failure safety comparison table.
apps/extension/src/background/controller/wallet.ts	Modified signRotationRequest to support signing with the pending new key during Phase 2 verification. Detects key-rotation-verify: prefixed payloads and derives keys from pendingNewKeyInfo.seedphrase instead of the current keyring. Supports both P256 and secp256k1 algorithms.
apps/extension/src/bridge/PlatformImpl.ts	Added keyRotationPendingKey field to temporarily store NewKeyInfo during 3-phase rotation. Updated saveNewKey to persist this value and signRotationRequest to pass it for verification payloads. Clears the pending key after verification attempts.
apps/react-native/src/bridge/NativeFRWBridge.ts	Added PendingRotationState interface and three new bridge methods: savePendingRotation, getPendingRotation, and clearPendingRotation to the TurboModule spec for WAL persistence on React Native.
apps/react-native/src/bridge/PlatformImpl.ts	Added implementations for savePendingRotation, getPendingRotation, and clearPendingRotation that delegate to NativeFRWBridge. Imported PendingRotationState type from @onflow/frw-types.
packages/cadence/src/cadence.generated.ts	Added auto-generated addKeys(publicKeys: string[]) and revokeKeys(revokeKeyIndexs: number[]) methods to support the separated Phase 1 (Expand) and Phase 3 (Collapse) transactions.
packages/cadence/src/cadence/Base/add_keys.cdc	New Cadence transaction that adds public keys to an account using ECDSA_secp256k1 signature algorithm, SHA2_256 hash algorithm, and weight 1000. Supports multiple keys via array parameter.
packages/cadence/src/cadence/Base/revoke_keys.cdc	New Cadence transaction that revokes keys by their indexes. Iterates through revokeKeyIndexs array and calls signer.keys.revoke() for each index.
packages/screens/src/keyrotation/KeyRotationMnemonicScreen.query.tsx	Added InfoDialog confirmation step before starting rotation. User must explicitly confirm via showRotationConfirmDialog state. Dialog explains the process and reassures that old keys remain until new key is verified.
packages/services/src/KeyRotationService.ts	Major refactor implementing Expand → Verify → Collapse phases. Added WAL persistence via savePendingRotation/clearPendingRotation before each state transition. Added reconcilePendingRotation for crash recovery with 5-minute staleness window. Phase 2 verifies signature matches expected public key. Added KEY_VERIFICATION_FAILED error type.
packages/services/tests/key-rotation-3phase.test.ts	New test file with 8 tests covering: happy path completion, Phase 2 verification failures (wrong key, no signature), Phase 1/3 on-chain failures, WAL phase progression ordering, post-revoke cleanup failure handling, and reconciliation keeping pending state when revoke is still required.
packages/services/vitest.config.ts	New Vitest configuration file enabling globals, V8 coverage provider with text/json/html reporters, 50-second test timeout, and standard exclusion patterns for node_modules and build artifacts.
packages/types/src/KeyRotation.ts	Added PendingRotationState interface with phase tracking (pre-tx, key-added, key-verified, api-registered, tx-confirmed). Extended KeyRotationDependencies with optional WAL methods. Added verificationPassed to KeyRotationServiceResult and KEY_VERIFICATION_FAILED to RotationErrorType.
packages/workflow/src/keyRotation/keyRotation.ts	Added addKeysOnChain (Phase 1 Expand) and revokeKeysOnChain (Phase 3 Collapse) methods to the KeyRotation class. Both methods submit transactions, wait for execution with status polling, and throw RotationError on failure.

---

autogenerated by presubmit.ai

[github-actions]

✅ LGTM!

Review Summary

Commits Considered (1)

• f24af2a: fix(services): implement 3-phase write-ahead log (WAL) for key rotation

Files Processed (14)

• PLAN.md (1 hunk)
• apps/extension/src/background/controller/wallet.ts (2 hunks)
• apps/extension/src/bridge/PlatformImpl.ts (2 hunks)
• apps/react-native/src/bridge/NativeFRWBridge.ts (2 hunks)
• apps/react-native/src/bridge/PlatformImpl.ts (2 hunks)
• packages/cadence/src/cadence.generated.ts (2 hunks)
• packages/cadence/src/cadence/Base/add_keys.cdc (1 hunk)
• packages/cadence/src/cadence/Base/revoke_keys.cdc (1 hunk)
• packages/screens/src/keyrotation/KeyRotationMnemonicScreen.query.tsx (6 hunks)
• packages/services/src/KeyRotationService.ts (8 hunks)
• packages/services/tests/key-rotation-3phase.test.ts (1 hunk)
• packages/services/vitest.config.ts (1 hunk)
• packages/types/src/KeyRotation.ts (3 hunks)
• packages/workflow/src/keyRotation/keyRotation.ts (1 hunk)

Actionable Comments (0)

Skipped Comments (7)

• packages/cadence/src/cadence/Base/add_keys.cdc [7-9]

  enhancement: "Hardcoded cryptographic parameters reduce flexibility."

• apps/extension/src/background/controller/wallet.ts [1346-1364]

  possible issue: "Missing validation for flowKey existence before accessing its properties."

• packages/services/src/KeyRotationService.ts [203-208]

  security: "Seedphrase is persisted in WAL state, increasing security exposure."

• packages/services/src/KeyRotationService.ts [346-348]

  maintainability: "Redundant null check for revokeTxId after Phase 3 completion."

• packages/services/src/KeyRotationService.ts [406-416]

  possible bug: "Incomplete flowKey object during recovery may cause downstream issues."

• apps/extension/src/bridge/PlatformImpl.ts [919-923]

  possible issue: "Pending key cleared after first verify attempt may break retry scenarios."

• packages/cadence/src/cadence.generated.ts [253-255]

  enhancement: "Hardcoded algorithm in generated Cadence limits key type flexibility."

[Kay-Zee]

Thanks for putting this together — the expand → verify → collapse ordering is the right call, and I like that the old key can't be revoked until the new one has proven it can sign.

That said, I traced the integrated path against FRW-Android (onflow/FRW-Android#3182 on top of current dev) and hit a few things that block this as-is. Details inline, short version:

• The first WAL write (pre-tx) carries no seedphrase, and the Android implementation throws when it's missing — so on Android the rotation dies at step one.
• Android's signRotationRequest (existing code, untouched by onflow/FRW-Android#3182) has no equivalent of the extension's key-rotation-verify: branch, so Phase 2 can never pass there — and by that point the new key is already on-chain.
• The reconciliation path never actually runs: it's only reachable through isBloctoAccount, which nothing calls — that predates this PR, but it means the cold-boot self-healing in the description has no execution path.
• When reconcile finds a rotation stuck between Phase 1 and Phase 3, it returns 'pending' and nothing ever finishes the revoke.

Also worth flagging: the extension doesn't implement any of the WAL methods (allowed, they're optional) — but that means the only platform that can currently complete the flow is also the one with no crash marker.

Small stuff, no back-and-forth needed: the if (!revokeTxId) check after Phase 3 can't fire since revokeKeysOnChain either returns an id or throws; the Phase 2 comment says "deterministic test payload" but the payload has Date.now() in it (fine functionally, just misleading); and PLAN.md probably shouldn't ship in the diff.

On tests — they pass, but bridge and workflow are both mocked, and the mock returns the new key on verify, so none of the cross-platform issues above get exercised. Something that pins down what the service actually expects from savePendingRotation / signRotationRequest implementations would go a long way.

[Kay-Zee]

This is the first thing rotateKey writes, and it has no seedphrase — but the Android savePendingRotation (onflow/FRW-Android#3182) throws when it's missing, and the catch below treats that as fatal. So on Android, rotation aborts right here, every time.

The same mismatch shows up in the types: seedphrase? is optional in frw-types but required in the TurboModule spec, so the passthrough in apps/react-native/src/bridge/PlatformImpl.ts doesn't actually typecheck — it only slips through because the RN app isn't covered by pnpm typecheck.

I'd pick one contract and stick to it — ideally the marker never carries the seedphrase at all (see my comment on the type) and native impls tolerate its absence.

[Kay-Zee]

The extension special-cases key-rotation-verify: and signs with the pending new key, but the Android signRotationRequest predates this flow and doesn't — it always signs with the current provider and returns the old public key (and saveNewKey doesn't invalidate the provider cache, so "current" stays the old key). This comparison can never succeed on Android, and we'd fail here after the new key was already added on-chain in Phase 1.

Needs a Kotlin counterpart to the extension branch — or honestly, this might be a good argument for moving the "sign verify payloads with the pending key" logic somewhere shared so the platforms can't drift apart.

[Kay-Zee]

The comment above says nothing local has been mutated yet, but I don't think that holds on Android: saveNewKey there does Wallet.store().updateMnemonic(seed).store(), which overwrites the stored mnemonic in place, no backup. At this point the new key isn't on-chain or even registered with the API. If the process dies here, a mnemonic-only account comes back up deriving from the new mnemonic, the old key is gone, and if Phase 1 never ran the account is locked out — the exact scenario this PR is trying to prevent.

I'd hold the destructive local switch until Phase 1 confirms on-chain, or make saveNewKey non-destructive until verification passes.

[Kay-Zee]

Is this wired up anywhere? isBloctoAccount looks like the natural entry point, but as far as I can tell nothing calls it (that predates this PR — the extension goes through checkKeyRotationNeeded → detectBloctoKey directly, and RN detection happens natively). So the self-healing described in the PR never actually runs. It'd need explicit hooks on each platform's boot path — plus a test, so it can't silently get unwired again.

[Kay-Zee]

If we get here — new key on-chain, revoke still outstanding — we return 'pending' and… that's it, forever. Nothing re-drives the revoke, so an interruption between Phase 1 and 3 leaves the old key active indefinitely, with the marker (seedphrase included) persisted the whole time. The marker already has phase + txId, so resuming the collapse from here seems doable; at minimum we should surface the stuck state to the user.

[Kay-Zee]

There's no check for whether this pubkey is already active on the account before adding it. Flow happily adds the same key again at a new index, so every retry after a failed Phase 2/3 stacks another duplicate — and with the Android verify issue, retries are guaranteed. Checking the pending marker + on-chain state before Phase 1 would make this idempotent, and would basically be the resume path reconcile is missing anyway. (Related: the API submit happens before Phase 1, so a Phase 1 failure leaves the backend registered with a key that never landed.)

[Kay-Zee]

The later markers (key-added onward) do send the seedphrase through the bridge, and on Android it lands in plain SharedPreferences — which is what this comment warns against. As far as I can tell recovery doesn't need it: reconcile already handles the missing-seedphrase case, and pubkey/phase/txId are enough to finalize or resume. I'd just remove the field — that also fixes the Android contract mismatch in one move.

[justjoolz]

Thanks Kaye, I really appreciate the thorough cross-repo review. This is exactly the level of technical engagement I was hoping for.

I agree with the issues you’ve identified. I’ve been working through them across both repositories and am validating the integrated path carefully before pushing a cohesive update and replying to each thread.

One question before I finalise it: are there any existing product or UX expectations for how interrupted or partially completed rotations should be surfaced to the user? I haven’t made assumptions about UI behaviour yet - particularly around automatic recovery versus an explicit resume/retry prompt. Would rather align with your intended flow before adding anything.

[justjoolz]

Thanks again for the detailed review. The local work has grown into three distinct concerns, so I’m going to split it to make each part safer and reviewable.

I’ll narrow Android PR 3182 to recovering existing orphaned Android Keystore accounts and surfacing them through the existing account UI.
I’ll keep PR 1427 focused on the shared prevention protocol and recovery contract.
I’ll open a separate Android PR implementing the native side of that protocol.

I also have substantial extension and iOS implementation work locally, but integrated review exposed platform-specific cold-start custody assumptions that I don’t want to present as production-ready without the relevant platform expertise. I can share those as separate draft work or continue them with guidance.

Does that split match how you’d prefer to review and land the work?

I’ve preserved the full current work on backup branches and won’t push or discard any of it while we agree the cleanest scope.

[Kay-Zee]

Thanks — and yes, the split matches exactly how we'd prefer to review and land this. #3182's narrowed rebuild re-reviewed well (comment there); the approach works.

On the UX question: we'd lean toward keeping it invisible. If the reconcile step can figure out the right ending on its own, it should just do it — nobody wants a "resume key rotation?" prompt they can't possibly evaluate, and most people would (reasonably) panic or dismiss it. The one time it's worth surfacing something is when the account genuinely can't be recovered on that device — that's a custody event and the user needs to know. Everything else is plumbing.

Two things we'd love to see pinned down in the narrowed contract, since the expand/verify/collapse core feels right but the edges matter. First: compromise-driven rotation — the dual-key window between expand and collapse keeps a leaked key valid for longer, so either an expedited path or an explicit "not for compromise response" note. Second: the multi-device story — when a rotation on one device revokes another device's key, what should that device reconcile into? (That ties straight into the custody-event case above.)

The extension/iOS work is welcome as drafts whenever you want early eyes on it.

[justjoolz]

Thanks @Kay-Zee - that gives me the product direction I needed. I agree that recoverable intermediate states should be detected and reconciled without asking users to interpret internal rotation state, with UI reserved for cases requiring user authorization or an unrecoverable custody event.
Where reconciliation or rotation requires a new on-chain account-key mutation, I think it should retain the wallet’s standard transaction review flow - including displaying the transaction code/details and obtaining distinct explicit approval - rather than initiating the mutation directly from an earlier workflow action.
I’ll incorporate the additional contract concerns into the proposed scope:
explicitly distinguish routine rotation from compromise response, where retaining the old key is itself unsafe;
define multi-device reconciliation when another device’s key has been revoked, including the custody-event boundary;
distinguish invisible local reconciliation from on-chain transitions requiring explicit transaction approval.
These make the complete cross-platform implementation materially broader than the current draft, so I’m going to formalise the remaining work into platform-specific milestones before continuing implementation.