feat(swift-sdk): add async off-main createWallet(mnemonic:) overload - #4483
Conversation
The synchronous createWallet blocks the calling thread for the whole native create - key derivation for every account plus a synchronous persistence flush, seconds of work - and every production caller is @mainactor, so wallet creation froze the UI for its full duration. Add an additive async overload with identical semantics that runs the blocking FFI on the shared destroyQueue (same rationale as the async shutdown(): park a plain GCD thread, never the main thread or a cooperative-pool thread), with: - a MainActor prologue (ensureConfigured + handle/call-table snapshot) and a direct continuation (no Task wrapper), so an admitted create is enqueued FIFO-before any later shutdown's teardown block; - a MainActor epilogue that re-checks the handle: a shutdown that landed during the off-main window discards the created wallet (its wrapper destroy is a registry no-op after manager teardown) instead of publishing into a torn-down manager; - performCreateWallet timing + offMain logging in parity with performNativeTeardown, behind a new PlatformWalletNativeCreateCalls test seam; - tests covering off-main execution, error mapping, create-after- shutdown, the shutdown-during-create race (FIFO proven via a shared event log), and concurrent creates. The sync overload stays for sync contexts (loadFromPersistor recovery, MainActor.run test bodies). Async call sites resolve to the new overload per SE-0296: SpvLateWalletBackfillIntegrationTests:56 switches silently (behavior equivalent, now off-main); ContentView.recoverWallet and WalletDetailView.enableNetwork needed try await - which also retires enableNetwork's 50ms paint-a-frame sleep hack. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Swift SDK now tracks wallet-creation admission during shutdown. Async creation completes native work and publication before teardown. Example app creation and recovery flows now await asynchronous wallet creation. ChangesAsync wallet creation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change moves wallet creation off the main thread while preserving shutdown ordering and adds coverage for the relevant concurrency cases; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CreateWalletView
participant PlatformWalletManager
participant NativeWalletCalls
participant WalletCollection
CreateWalletView->>PlatformWalletManager: await createWallet(...)
PlatformWalletManager->>NativeWalletCalls: create mnemonic wallet
NativeWalletCalls-->>PlatformWalletManager: wallet ID or native error
PlatformWalletManager->>WalletCollection: publish wallet
PlatformWalletManager-->>CreateWalletView: return wallet or mapped error
PlatformWalletManager->>NativeWalletCalls: await admitted creates before teardown
🚥 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 |
|
✅ Final review complete — no blockers (commit 5b80766) |
Review follow-up (P1): the epilogue-throw design could fail an admitted create RETROACTIVELY - the native create had already persisted wallet data through the persister when a concurrent shutdown() took the handle, so the invalidHandle throw made the app-side caller roll back its mnemonic and orphan the persisted rows (watch-only wallet without a mnemonic; retry hits WalletAlreadyExists). The destroy-queue FIFO only ordered the native calls, not the MainActor epilogue. shutdown() now closes admission first (shutdownRequested - new creates throw invalidHandle up front, before any native work) and then waits for every in-flight create's FULL transaction (native create + publish epilogue) to finish before the take-once. The idempotency/no-op checks re-run after each drain await. The epilogue handle re-check stays as defense in depth (assertionFailure) but is unreachable from the production shutdown path. Tests reworked to prove the new properties: the shutdown-during-create race asserts the handle stays live until the gated create finishes and that create:end precedes the first teardown step in a shared event log; a new case pins create-during-drain rejection; the concurrent-creates test now measures maxInFlight == 1 through the seam. Also adopts the async overload in the example app's CreateWalletView (its per-network create loop ran synchronously inside MainActor.run, freezing the UI - the second reviewer note). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The async path correctly moves native wallet creation off the main thread and drains admitted async transactions before teardown, but shutdown still admits both legacy synchronous creation APIs while the MainActor is suspended. The tests also fabricate live-looking wallet handles whose deinits call the real Rust destructor, creating a test-isolation issue. Source: Codex general, security-auditor, and FFI-engineer reviewer lanes (exact backend model ID was not supplied); Claude final verifier (exact backend model ID was not supplied); openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:546-550: Shutdown admission remains open for synchronous wallet creates
After setting `shutdownRequested`, `shutdown()` suspends while an admitted async create finishes, making the MainActor reentrant while `isConfigured` and `handle` remain live. During that interval, callers can enter the synchronous mnemonic overload at line 828 or the synchronous seed overload at line 982 because both only call `ensureConfigured()`, which does not inspect `shutdownRequested`. Those calls can perform native creation and publish another wallet after shutdown has begun; they are also absent from `activeCreateCount`. This behavior was introduced by moving the handle take after the new drain and contradicts the PR's closed-admission guarantee. Reject shutdown-in-progress from both synchronous creation overloads, preferably through a common creation-specific guard shared with the async overload.
In `packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletCreateWalletTests.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKTests/PlatformWalletCreateWalletTests.swift:62-68: Fake wallet handles can destroy real Rust registry entries
The injected success result is wrapped in `ManagedPlatformWallet`, whose `deinit` unconditionally calls the live `platform_wallet_destroy` FFI. Rust removes that numeric key from the process-global `PLATFORM_WALLET_STORAGE`, and real handles come from a process-global monotonic counter, so fake handles 101 and 102 can collide with live wallets owned by another test and remove their registry entries. This also contradicts the test's claim that the seam does not call FFI. Use `NULL_HANDLE`; destroying handle zero is a harmless registry miss, and the distinct wallet IDs already provide the differentiation these assertions require.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head 5b80766, the supplied Codex evidence is clean and no remaining in-scope defects were found. Both prior findings are fixed: every wallet-creation overload now uses the shutdown-aware admission guard, and injected successful creates use NULL_HANDLE rather than collision-prone fake registry handles. Validation passed with the iOS example build succeeding under warnings-as-errors and all 6 targeted PlatformWalletCreateWalletTests passing; git diff checks were also clean.
Source: Codex reviewer backend gpt-5.6-sol (general, FFI-engineer, and security-auditor); Claude verifier backend exact model ID was not supplied; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
3 participants
Issue being fixed or feature implemented
The synchronous
createWalletblocks the calling thread for the entire native create — per-account key derivation plus a synchronous persistence flush, seconds of work — and every production caller is@MainActor, so wallet creation freezes the UI for its full duration (measured ~1.1–2.2s per network in dashwallet).What was done?
Additive async overload with identical semantics that runs the blocking FFI on the shared
destroyQueue(same rationale as asyncshutdown()from #4469: park a plain GCD thread, never the main thread or a Swift Concurrency cooperative-pool thread):ensureConfigured+ shutdown-in-progress admission check + handle/call-table snapshot) and a direct continuation (deliberately noTask {}wrapper — the dispatch happens synchronously at the suspension point, so an admitted create is enqueued FIFO-before any latershutdown()'s teardown block; teardown can never overtake an in-flight create).shutdown()drains admitted creates before taking the handle: it closes admission (shutdownRequested, new creates throw up front before any native work), then awaits every in-flight create's FULL transaction — native create and MainActor epilogue (publish) — before the take-once. An admitted create whose FFI already persisted wallet data is therefore never failed retroactively by a concurrent teardown (that would make the caller roll back its mnemonic and orphan the persisted rows). The epilogue handle re-check remains as defense in depth (withassertionFailure) but is unreachable from the production shutdown path.setWalletNameand thewalletspublish live in the MainActor epilogue.performCreateWallettelemetry in parity withperformNativeTeardown: one log linenative create finished in Xms offMain=Y network=Z(verified on-device:1512ms offMain=true network=1,1076ms offMain=true network=0). The measured time includes the create's trailingidentity().sync()inside the Rust call — per-stage splits would need Rust changes and are deliberately out of scope.PlatformWalletNativeCreateCallstest seam (the teardown table can't express create) +PlatformWalletCreateWalletTests: off-main execution, error mapping, create-after-shutdown, shutdown-during-create drain (handle stays live until the gated create finishes; shared event log provescreate:endprecedes the first teardown step), create-during-drain rejection, and concurrent creates with a measuredmaxInFlight == 1(the queue provably serializes).destroyQueuedoc updated: it now serializes create+destroy; its job is thread-parking + deterministic FIFO (memory safety comes from the Rust registry — create holds a read guard for its whole call, destroy's map removal takes the write lock andblock_on(shutdown())runs after releasing it).CreateWalletViewnow uses the async overload too (its per-network create loop ran synchronously insideMainActor.run, freezing the UI);WalletDetailView.enableNetworkadopted it and dropped its 50ms paint-a-frame sleep hack;ContentView.recoverWalletadopted it.No Rust changes.
How Has This Been Tested?
bash build_ios.sh --target tests --profile dev && swift test: full suite green (6 create tests).Breaking Changes
Source-compatibility caveat (no ABI/behavior break for existing binaries): adding a same-name async overload changes overload resolution — per SE-0296,
try await manager.createWallet(...)call sites silently re-resolve to the new overload (one such site in this repo:SpvLateWalletBackfillIntegrationTests.swift, behavior equivalent), andtry manager.createWallet(...)written inside an async context stops compiling until markedtry await(two such sites existed in SwiftExampleApp and are fixed here). External Swift consumers with async-context call sites may need the same one-wordawaitfix. Sync contexts are untouched.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests