feat(swift-sdk): make wallet deletion asynchronous#4170
Conversation
📝 WalkthroughWalkthroughWallet deletion APIs now run asynchronously through the persistence serial queue, sequence Keychain and SwiftData cleanup phases, and block wallet creation or loading during deletion. The example app and deletion tests now await the updated APIs. ChangesWallet deletion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WalletDetailView
participant PlatformWalletManager
participant PlatformWalletPersistenceHandler
participant Keychain
WalletDetailView->>PlatformWalletManager: await deleteWallet(walletId)
PlatformWalletManager->>PlatformWalletPersistenceHandler: await identityIdsForWallet(walletId)
PlatformWalletManager->>Keychain: delete wallet and identity keys
PlatformWalletManager->>PlatformWalletPersistenceHandler: await deleteWalletData(walletId)
PlatformWalletManager->>Keychain: delete mnemonic and metadata if no rows remain
PlatformWalletManager-->>WalletDetailView: deletion result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
⛔ Blockers found — Sonnet deferred (commit ed72039) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift (1)
1012-1042: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the Rust wallet before the Keychain sweep.
identityIdsis captured beforeplatform_wallet_manager_remove_wallet, so any identity/key material written while deletion is awaiting the Keychain task can miss the purge and remain in Keychain after the wipe completes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift` around lines 1012 - 1042, Reorder the deletion flow so platform_wallet_manager_remove_wallet executes before capturing identityIds and starting the detached Keychain cleanup task. Keep the existing wallet removal and in-memory cleanup behavior, then perform the Keychain sweep using identityIds obtained after the Rust wallet removal completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- Around line 1012-1042: Reorder the deletion flow so
platform_wallet_manager_remove_wallet executes before capturing identityIds and
starting the detached Keychain cleanup task. Keep the existing wallet removal
and in-memory cleanup behavior, then perform the Keychain sweep using
identityIds obtained after the Rust wallet removal completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6aa2d6b9-d07f-4db7-934a-225c9221425e
📒 Files selected for processing (4)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The async persistence bridge preserves serial SwiftData access and keeps the main actor responsive, but wallet deletion now suspends while independently retained Rust wallet handles remain operational. Those handles can persist new state after the final sweep, so the deletion lifecycle requires an FFI-level quiescence barrier before this PR is safe. The manager-level reentrancy gate is also not exercised by the added tests.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (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:1013-1021: Quiesce child FFI handles before awaiting deletion
Deletion now yields before removing the wallet from `wallets`, allowing another main-actor turn to obtain its `ManagedPlatformWallet`; previously retained wrappers are also unaffected by the manager gate. Each wrapper owns an independent handle whose Rust `Arc<PlatformWallet>` remains in `PLATFORM_WALLET_STORAGE`, and `platform_wallet_manager_remove_wallet` removes only the manager's references. Because `ManagedPlatformWallet` operations run through these handles and several async operations persist changes through the same callback-backed persister, an operation started from a stale handle can reach the synchronous persistence callbacks after `deleteWalletData` has completed and recreate wallet rows after deletion reports success. Add a shared FFI deletion/quiescence state that rejects new child-handle operations and waits for already-running operations before the final persistence sweep; removing the published Swift wrapper before the first await reduces exposure but does not protect wrappers already retained elsewhere.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:210-214: Add coverage for the manager-level deletion gate
The new `walletDeletionInProgress` flag and its checks in both `createWallet` overloads, `loadFromPersistor`, and `deleteWallet` are the only protection against manager actor reentrancy while deletion is suspended. The added tests call `PlatformWalletPersistenceHandler.deleteWalletData` directly, and the idempotence test performs two completed deletions sequentially, so removing `beginWalletDeletion` or any create/load guard would not fail the suite. Add a manager-level test that blocks the first deletion on the persistence queue, verifies that an overlapping delete and create/load attempts are rejected, then releases the first deletion and verifies the gate is cleared.
| let identityIds = try await persistenceHandler.identityIdsForWallet( | ||
| walletId: walletId) | ||
|
|
||
| try await Task.detached(priority: .userInitiated) { | ||
| for identityId in identityIds { | ||
| try KeychainManager.shared.deleteAllKeychainItems(forIdentityId: identityId) | ||
| } | ||
| try KeychainManager.shared.deleteAllIdentityPrivateKeys(forWalletId: walletId) | ||
| }.value |
There was a problem hiding this comment.
🔴 Blocking: Quiesce child FFI handles before awaiting deletion
Deletion now yields before removing the wallet from wallets, allowing another main-actor turn to obtain its ManagedPlatformWallet; previously retained wrappers are also unaffected by the manager gate. Each wrapper owns an independent handle whose Rust Arc<PlatformWallet> remains in PLATFORM_WALLET_STORAGE, and platform_wallet_manager_remove_wallet removes only the manager's references. Because ManagedPlatformWallet operations run through these handles and several async operations persist changes through the same callback-backed persister, an operation started from a stale handle can reach the synchronous persistence callbacks after deleteWalletData has completed and recreate wallet rows after deletion reports success. Add a shared FFI deletion/quiescence state that rejects new child-handle operations and waits for already-running operations before the final persistence sweep; removing the published Swift wrapper before the first await reduces exposure but does not protect wrappers already retained elsewhere.
source: ['codex']
| /// Async deletion yields while the persistence handler sweeps SwiftData. | ||
| /// Reject create/load/another delete during that window so actor | ||
| /// reentrancy cannot recreate a deterministic wallet id that the in-flight | ||
| /// deletion is about to remove from persistence. | ||
| private var walletDeletionInProgress = false |
There was a problem hiding this comment.
🟡 Suggestion: Add coverage for the manager-level deletion gate
The new walletDeletionInProgress flag and its checks in both createWallet overloads, loadFromPersistor, and deleteWallet are the only protection against manager actor reentrancy while deletion is suspended. The added tests call PlatformWalletPersistenceHandler.deleteWalletData directly, and the idempotence test performs two completed deletions sequentially, so removing beginWalletDeletion or any create/load guard would not fail the suite. Add a manager-level test that blocks the first deletion on the persistence queue, verifies that an overlapping delete and create/load attempts are rejected, then releases the first deletion and verifies the gate is cleared.
source: ['codex']
2 participants
Summary
PlatformWalletManager.deleteWalletasync so SwiftData and Keychain deletion no longer block the main actorTesting
WalletDeletionTestscoverage for the async persistence barrier and overlapping deletiondashpayarm64 Simulator build with the async APIdashwallettest scheme remains blocked by the local watchOS runtime mismatch (23S303runtime vs23T570SDK)Summary by CodeRabbit