feat(wallet): run Add Wallet's create FFI off-main and serialize it on the lifecycle queue - #1068
Merged
Merged
Conversation
…n the lifecycle queue Etap B of the Add Wallet freeze fix (a PARTIAL fix - see scope note below). Adopts the SDK's new async createWallet(mnemonic:) overload (platform feat/swift-sdk-async-create-wallet) so the blocking native create - both networks' key derivation + persistence flush, the bulk of the ~5s MainActor freeze - runs on the SDK's dedicated queue instead of the main thread. The overlay's spinner now animates through the create windows. - MnemonicFirstWalletCreation.run takes an async createWallet closure (single version - no sync/async pair); rollback-after-await semantics unchanged and now covered by a suspension-path test. - createAndPersist is async; its three call sites await it. The loadFromPersistor recovery path (recoverPersistedWallet) deliberately stays on the sync SDK overload. - SerialAsyncLifecycleQueue gains a value-returning enqueueAwaitable<T> (same chain, full barrier semantics), and the runtime gains performAddWallet: the interactive add now runs as ONE link of the serial lifecycle chain, so a queued refresh/fullReset can no longer interleave with the multi-network provisioning. The post-add switch stays OUTSIDE the chain (sequential op - nesting would self-await- deadlock the queue). Onboarding's createOrImportWallet deliberately stays off the chain (its launch-time race-by-design is unchanged). - The 100ms overlay-commit sleep STAYS: on the mirror-repair path the first provisioning work is still the other network's synchronous SDK build on the MainActor with no suspension point before it. Scope note: the mirror-network leg's SDK(network:) + configure + loadFromPersistor (~1.6-2s) still blocks the MainActor under the visible overlay - known limitation, candidate for etap C after the new create telemetry (offMain + duration logs) is in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughWallet creation now awaits asynchronous SDK operations. Additive provisioning runs as one serialized lifecycle operation. Tests cover queue results, error propagation, ordering, and mnemonic rollback. ChangesWallet lifecycle updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change moves wallet creation off the main thread and serializes interactive lifecycle work; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant WalletsViewModel
participant SwiftDashSDKWalletRuntime
participant SerialAsyncLifecycleQueue
participant SwiftDashSDKHost
WalletsViewModel->>SwiftDashSDKWalletRuntime: performAddWallet
SwiftDashSDKWalletRuntime->>SerialAsyncLifecycleQueue: enqueueAwaitable
SerialAsyncLifecycleQueue->>SwiftDashSDKHost: addWallet
SwiftDashSDKHost-->>SerialAsyncLifecycleQueue: AddWalletResult or error
SerialAsyncLifecycleQueue-->>WalletsViewModel: provisioning result or error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review follow-up (P1): adopting the async SDK create everywhere put a REAL suspension point in the middle of createOrImportWallet's transaction - between the mnemonic persist and the wallet rows appearing. During that window the MainActor could run a concurrently scheduled startIfReady/refresh, which would see the mnemonic (hasSDKWallet) with no wallet rows yet, build a competing runtime, and the two flows could overwrite each other's publish or leak a manager to the deinit fallback teardown. The old sync FFI blocked the MainActor exactly there, so the ordering was NOT unchanged as previously claimed. createAndPersist gains offMainCreate: the interactive add keeps the async overload (safe - performAddWallet serializes it on the lifecycle queue, so refreshes queue behind it), while the onboarding/migration path (createOrImportWallet) forces the SYNC SDK overload via an explicitly typed non-async closure - no suspension between persist and create, restoring the atomic critical section. Onboarding cannot go on the lifecycle queue instead: refresh awaits the key migrator, and the migrator calls createOrImportWallet - enqueueing would self-deadlock. Onboarding therefore still blocks main for the create's duration (as before this PR); its off-main adoption needs its own serialization design and stays out of this etap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Issue being fixed or feature implemented
Etap B of the Add Wallet freeze fix — a partial fix, by design. Etap A (#1064) put the blocking overlay up from the first moment, but the MainActor was still genuinely frozen underneath: the synchronous SDK
createWalletFFI ran on the main thread for both networks (~1.5s + ~1.1s measured), so the spinner stalled and the app was dead to the touch.What was done?
Adopts the SDK's new async
createWallet(mnemonic:)overload (dashpay/platform#4483) for the interactive Add Wallet flow and serializes that flow on the runtime's lifecycle chain:MnemonicFirstWalletCreation.runtakes an asynccreateWalletclosure (single version — no sync/async pair, to avoid the overload-resolution hazard inside app code). Rollback-after-await semantics unchanged, pinned by a suspension-path test.createAndPersist→async throwswith anoffMainCreateswitch:true— interactive add: the async overload; the blocking FFI leaves the MainActor. Safe becauseperformAddWalletruns the whole add as one link of the serial lifecycle chain, so a queuedrefresh/fullResetcan never interleave with the suspension the off-main create introduces.false— onboarding/migration (createOrImportWallet): the sync SDK overload, forced by an explicitly typed non-async closure. An await between the mnemonic persist and the wallet rows appearing would let a concurrently scheduledstartIfReady/refresh observe the half-state (mnemonic present, no rows), build a competing runtime, and the two flows could overwrite each other's publish or leak a manager to the deinit-fallback teardown. Onboarding can't ride the lifecycle queue instead (refresh awaits the key migrator, and the migrator callscreateOrImportWallet— enqueueing would self-deadlock), so it keeps the MainActor-atomic critical section and still blocks main during onboarding create, as before this PR. Its off-main adoption needs its own serialization design (etap C candidate).recoverPersistedWallet(launch recovery, sync context) stays on the sync SDK overload.SerialAsyncLifecycleQueuegains a value-returningenqueueAwaitable<T>(same chain, full barrier semantics); the runtime gainsperformAddWallet. The post-add switch stays outside the chain (sequential op — nesting would self-await-deadlock the queue, same hazard documented atswitchNetwork).SDK-side, #4483's
shutdown()drains admitted creates before taking the handle, so an add whose FFI already persisted wallet data can never be failed retroactively into a mnemonic rollback that orphans the persisted rows.Honest scope — what still blocks main: the mirror-network leg's
SDK(network:)+configure+loadFromPersistor(~1.6–2s, one continuous stretch) during Add Wallet, and the whole onboarding create (unchanged from before). Both are etap C candidates once the new create telemetry accumulates. The Add Wallet loader shows for a similar total time as before — the work is the same, it just no longer freezes the UI during the create phases.How Has This Been Tested?
dashpaybuild (arm64 sim). Testnet simulator smoke of Wallets → Add Wallet → Create New Wallet:native create finished in 1512ms offMain=true network=1and1076ms offMain=true network=0— both legs off-main;startIfReadyracing): wallet created and bound, single runtime.MnemonicFirstWalletCreationTests+ newenqueueAwaitabletest are compile-ready per the repo's current posture.Build requirement: the local
../platformcheckout must include dashpay/platform#4483 (local SPM path dependency — no pin to bump; no FFI rebuild needed, Rust untouched).🤖 Generated with Claude Code