feat(wallet): support recovery phrases for multiple wallets - #1018
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
romchornyi
left a comment
There was a problem hiding this comment.
Reviewed the full diff, and traced a couple of the riskier-looking paths into the SDK and the setup flow.
What checks out
- The core fix is real:
strictlyPersistedMnemonic(for:)reads exactly the selected wallet id and never substitutes the active/first/sole entry, and the post-readmaterial.canonicalWalletId == descriptor.idguard (RecoveryPhraseFlow.swift:437,:521) closes the TOCTOU window. - Grouping mirrored mainnet/testnet entries under the canonical mainnet id is correct, and
walletChangedis thrown when one group holds conflicting phrases. The tests cover both directions. SwiftDashSDKStoredWalletNetworkResolveris a faithful extraction ofclassifyStoredWalletIdsByNetwork— no behavior change.- I checked whether reusing
DWPreviewSeedPhraseViewControllerfor an existing wallet arms the screenshot wipe. It does not: the destructivescreenshotAlertOKActionoverride lives only inBackupSeedPhraseViewController; the preview controller's is a NOP. Good. ResetDelegate's emptysecureWalletRoutineDidVerify/DidFinishare unreachable here —shouldVerifyPassphraseis only read byDWSetupViewController, so the preview screen never enters the verify routine. It mirrors the existingDelegateInternal.
Main concern: one bad entry disables the whole backup path
RecoveryPhraseInventory.load() does try storedEntries.map(classify), and classify throws when Mnemonic.validate rejects a phrase or the network can't be resolved. So a single unreadable or non-BIP39 Keychain entry produces "Couldn't Read Recovery Phrases" for every wallet, with a Retry that is guaranteed to fail again.
Fail-closed is the right posture for the destructive paths in #1014/#1017. Here it's inverted: this is the only in-app route a user has to write down their seed, and a neighbouring bad entry locks them out of all of it. It should degrade gracefully — skip or flag the entry that failed to classify, and still list the wallets that did.
The per-wallet route (beginWallet from a Wallets row) isn't affected, since mnemonic(for:) reads a single id. It's the global Security-menu entry point that breaks.
Copy Seed (DEBUG) has no auth gate
copyWalletMnemonic (RecoveryPhraseFlow.swift:416) is the only entry point that doesn't go through authenticate(...), unlike beginWallet. It writes the plaintext mnemonic to UIPasteboard.general with no expirationDate and no localOnly, so it propagates to other devices via Universal Clipboard.
It's #if DEBUG, but the existing debug copy affordance (debugContentViewLongPressAction) at least sits behind the already-authenticated phrase screen. Worth routing this through the same gate and constraining the pasteboard item.
Main-thread FFI with no visible progress
classify → resolve performs up to two SwiftDashSDK.Wallet(mnemonic:network:) calls, and that's wallet_create_from_mnemonic — a full Rust wallet construction (BIP39 seed plus default account derivation), not just an id hash. load() is @MainActor and loadGlobalRoute calls it synchronously, so an N-wallet device does up to 2N of those on the main thread. Previously this classification only ran on the wiper's background queue.
isBusy can't cover it either: it's set and cleared by defer within the same synchronous scope, so it never renders. Same pattern in select, loadWallet, and copyWalletMnemonic.
Minor
shortIdentifieruses the canonical mainnet id prefix whilefallbackName(for: source.walletId)uses the real wallet id, so an unnamed testnet wallet renders as "Wallet a1b2… / Testnet • 9f3e…" — two different short ids on one row.handleRecoveryPhraseNavigationis duplicated verbatim inSecurityMenuScreenandWalletsScreen.- The pbxproj UUIDs are hand-written (
A1C0D001…0001) rather than Xcode-generated. Unique and valid, just out of order with the rest of the file. - Picker selection is routed through the parent SwiftUI screen's
.onReceivewhile the picker is already pushed on top as aUIHostingController. It should hold, but please confirm it on device during the smoke — if that parent view is ever detached, a tap in the picker would silently do nothing.
Process
This is stacked on #1017, which I've just requested changes on, so the base will still move. Tests weren't run and the manual smoke is still outstanding.
Nothing here is as severe as the lockout in #1017, so I'm leaving this as a comment rather than a block — but I'd like the graceful-degradation fix and the Copy Seed gate before this merges.
|
@romchornyi Thanks for flagging this. We're intentionally leaving |
|
@romchornyi Thanks. We are intentionally keeping the global recovery-phrase inventory strict: silently omitting an unreadable or unclassifiable entry could present a partial list as complete, while the exact per-row path remains available for healthy wallets. Copy Seed (Debug) is also intentionally unchanged as a DEBUG-only, low-friction testing aid, including Universal Clipboard; production paths remain authenticated. We will smoke-test picker navigation and multi-wallet latency, but without a reproduced navigation failure or measurable stall we are not adding a wrapper or moving the classification architecture in this PR. The remaining ID presentation, duplication, and project-file ordering notes are non-functional and deferred. |
|
Fair on On the strict inventory, you're right about the per-row path and that narrows what I flagged: The part I'd still push on is the framing, because it isn't a choice between "strict" and "silently partial". A third option shows the wallets that did classify and states explicitly that N entries couldn't be read — the list never claims to be complete, and access isn't lost. That gets the honesty you're protecting without the all-or-nothing failure. There's also a residual case the per-row path doesn't cover: a wallet whose Keychain entry exists but isn't loaded in the current manager (the other network) has no row at all, so the global entry point was its only route to the phrase. For that wallet, one unrelated bad entry means no way to reach it. Not blocking — leaving it as a comment either way. |
1fb32d3 to
b8b85a2
Compare
There was a problem hiding this comment.
Graceful degradation for the recovery-phrase list is still not in place
RecoveryPhraseInventory.load() (RecoveryPhraseFlow.swift:~112) is unchanged in the latest push:
let entries = try storedEntries.map(classify)
One unreadable or non-BIP39 Keychain entry still throws and takes down the entire list, with a Retry that's guaranteed to fail again. This is the only in-app route to write down a seed for wallets not reachable via a per-row action — a single bad neighbor shouldn't lock users out of every phrase they could otherwise read.
Main-thread FFI blocking is still unaddressed
classify() (RecoveryPhraseFlow.swift:~199) still calls SwiftDashSDKStoredWalletNetworkResolver.resolve(...) synchronously inside @MainActor static func load() — up to 2N full wallet_create_from_mnemonic calls on the main thread for N wallets, with no offload and no working busy-indicator (still set/cleared within the same synchronous scope via defer). Worth at least confirming on-device latency for a multi-wallet case before merge, since this wasn't part of the smoke-test scope mentioned.
The residual "unreachable wallet" case is still open
WalletsViewModel.displayName(for:) was made reusable, but the Wallets row list is still built only from the current network's manager — a wallet whose Keychain entry exists for the other network but isn't loaded there still has no row, and no way to reach it if the global inventory throws. That wallet's only path to its phrase is the one that's still fail-closed above. Flagging again since the last push didn't touch this.
|
Implemented the requested graceful degradation in eb8d367. RecoveryPhraseInventory now strictly enumerates stored SDK wallet IDs, but reads and classifies each entry independently. An unreadable, empty, invalid-BIP39, or unresolvable neighboring entry is skipped and logged using only a count and short wallet-ID prefix; no mnemonic is logged. Readable wallets continue through the existing direct/chooser routing. If stored IDs exist but none are readable, the flow still shows the read-error/Retry state; a genuinely empty inventory still shows No Recovery Phrase Found. The final reveal remains unchanged and strict: it re-reads the exact selected sourceWalletId and verifies the canonical wallet ID before presenting anything, with no active/first/sole fallback. Conflicting valid material for one canonical wallet also remains fail-closed. Added focused regression coverage for readable plus unreadable, readable plus invalid, all-unreadable, and empty inventories. git diff --check passes. No migration code was touched: SwiftDashSDKKeyMigrator, wallet recovery, the wiper, runtime, and legacy serialization are unchanged. I also manually smoked the picker with four wallets; it appeared immediately with no visible UI stall, so I did not add concurrency churn for the main-thread FFI concern. No Xcode build/test run, per the agreed workflow. |
Summary
Dependency
Stacked on #1017. After #1017 merges, retarget this PR to its merged base branch.
Verification
git diff --checkpasses