feat(swift-sdk): deleteWallet wipes full wallet footprint#3653
Open
llbartekll wants to merge 4 commits into
Open
feat(swift-sdk): deleteWallet wipes full wallet footprint#3653llbartekll wants to merge 4 commits into
llbartekll wants to merge 4 commits into
Conversation
`PlatformWalletManager.deleteWallet(walletId:)` wipes the wallet's Rust manager state, SwiftData rows (including orphans the `@Relationship` graph misses: `PersistentTransaction`, `PersistentPendingInput`, `PersistentTokenBalance`), identity-keyed and wallet-keyed Keychain entries, and the network sync state when no sibling wallets remain. Closes the data-leakage path where `modelContext.delete(wallet)` alone left orphan rows on the same network for the next wallet to see — the bug that prompted this on dashwallet-ios and was present in the SDK's own example app. Rust-side adds: - `platform_wallet_manager_remove_wallet` FFI (idempotent on missing). - `PlatformWalletPersistence::retire_wallet` trait method (default no-op). - `FFIPersister` retired set: drops non-registration `store`/`flush` for retired wallets and auto-clears on a `wallet_metadata`-bearing changeset so a same-session reimport recovers. - `PlatformWalletManager::remove_wallet` snapshots the wallet's identity ids and `IdentitySyncManager::unregister_identity`s each before dropping the wallet, so per-identity token sync stops. - `IdentitySyncManager::apply_fresh_balances` re-checks the live state under its write lock before persisting, closing the race where a mid-sync unregister could still emit a token balance. Swift-side adds: - `PlatformWalletPersistenceHandler.deleteWalletData` + companion `identityIdsForWallet` for the orchestration. - `KeychainManager.deleteAllKeychainItems(forIdentityId:)` (sweeps both `privkey_*` and `specialkey_*` schemes) and `deleteAllIdentityPrivateKeysForWallet(walletId:)` throwing variants so partial-failure surfaces instead of silently passing. Tests: - `WalletDeletionTests` (Swift, in-memory `ModelContainer`): cascade + orphan-tx sweep, idempotency, network-sync conditional cleanup (both branches), and per-scheme Keychain sweeps. - `retired_wallet_drops_non_registration_store_and_flush` and `registration_store_clears_retirement` (Rust): persister retirement semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the `do { try X; return true } catch { return false }` bridge
pattern around `deleteAllPrivateKeys(for:)` and
`deleteAllIdentityPrivateKeys(forWalletId:)`. The throwing variants
take over the canonical names; callers `try` directly. Drops the
parallel `*Throwing` / `*ForWallet` names.
Replaces seven inline `map { String(format: "%02x", $0) }.joined()`
hex-format duplications with the existing `Data.toHexString()`
extension, and removes the same-shape `hex(_:)` helper from
`WalletDeletionTests` in favor of the same extension.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis pull request implements complete wallet deletion across the platform. The Rust backend introduces a retirement mechanism to gate stale persistence writes, coordinates identity unregistration with wallet removal, and robustifies identity syncing to handle unregistered identities. The Swift layer adds deletion APIs for SwiftData and Keychain, and orchestrates the full cleanup workflow including FFI calls and in-memory state updates. ChangesWallet Deletion Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "6a7d38f3ff70b54502868ce2a3de260b42bce1c295697faebb4d671f9942302c"
)Xcode manual integration:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/manager/identity_sync.rs (1)
570-587:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid calling external persistence while holding
statewrite lock.
self.persister.store(...)runs underself.state.write(). Because this is an external call, it can stall or re-enter paths that needstate, blockingregister/unregister/update_watched_tokensand creating deadlock risk.Take a snapshot under lock, release lock before
store, then reacquire and re-check identity before applying cache updates.🤖 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/rs-platform-wallet/src/manager/identity_sync.rs` around lines 570 - 587, Currently persister.store(...) is called while holding the self.state.write() lock which can deadlock; instead, inside identity_sync.rs (around the code using state.get(&identity_id), existing_row, sentinel, and cs) clone or take a snapshot of the changeset (cs) and any needed existing_row data while holding the write lock, then drop the lock before calling self.persister.store(sentinel, cs_snapshot), and after store returns reacquire the write lock and re-check that the identity still exists (state.get(&identity_id)) before applying any cache updates or mutations; ensure you handle and log persister.store errors the same way but performed outside the lock.
🧹 Nitpick comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift (2)
149-191: ⚡ Quick winAdd a non-matching wallet control for the wallet-scoped key sweep.
This only proves that the targeted
walletIdrow is removed. BecausedeleteAllIdentityPrivateKeys(forWalletId:)enumerates the entire service, a broken filter that deletes everyidentity_privkey.*item would still satisfy this test. Seed a second row with a differentwalletIdand assert it survives.🤖 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/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift` around lines 149 - 191, The test testThrowingKeychainSweepsUseIsolatedService currently only inserts one identity private key so deleteAllIdentityPrivateKeys(forWalletId:) could accidentally delete all identity_privkey.* rows; insert a second identity private key via KeychainManager.storeIdentityPrivateKey with a different walletId (e.g., walletId2 and publicKey2) before calling manager.deleteAllIdentityPrivateKeys(forWalletId: walletId) and after the deletion assert that retrieveIdentityPrivateKey(publicKeyHex: publicKey2) still returns non-nil while the original publicKey is nil, ensuring the sweep is scoped to the targeted walletId.
38-120: ⚡ Quick winExercise a transaction that becomes orphaned because of the wallet cascade.
orphanTxstarts out orphaned before deletion, so this test still passes ifdeleteWalletData(walletId:)misses transactions that only become orphaned after wallet/account/TXO rows are removed. Please add one wallet-owned TXO/transaction pair and assert that the transaction is swept after the delete.🤖 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/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift` around lines 38 - 120, Add a transaction+TXO pair that is initially owned by the wallet and therefore removed by the wallet cascade: create a new PersistentTransaction named walletOwnedTx and a matching PersistentTxo named walletOwnedTxo that is associated with the wallet (set whichever field ties a txo to a wallet in your model—e.g., walletId or an address/ownership field) and append walletOwnedTxo to walletOwnedTx.outputs, insert both into the context alongside the existing objects before saving, then after calling PlatformWalletPersistenceHandler.deleteWalletData(walletId:) assert that the fetched PersistentTransaction list no longer contains walletOwnedTx (i.e., transactions count remains 1 and walletOwnedTx is gone). This ensures transactions that only become orphaned by the wallet cascade are swept.
🤖 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.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/persistence.rs`:
- Around line 416-422: store() currently only checks the retired set once and
can persist changes if retire_wallet() runs concurrently, allowing stale writes
for just-retired wallets; modify the persistence path to coordinate retirement
and writes atomically by introducing a per-wallet lifecycle lock or state
machine (e.g., a per-wallet mutex/atomic state) and use it in both store() and
retire_wallet() so that store() acquires the wallet lock/state before running
the callback and verifies retirement state again (or aborts) before committing
pending merges; ensure you reference and protect the same retired set and the
code paths that examine changeset.wallet_metadata and perform pending merges so
no post-retire writes can be committed.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- Around line 349-353: The deleteWallet(walletId:) flow calls
persistenceHandler?.identityIdsForWallet(...) and proceeds to call
KeychainManager.shared.deleteAllIdentityPrivateKeys(forWalletId:) even when
persistenceHandler is nil (e.g., configure(..., modelContainer: nil)), which can
leave private keys in Keychain; update deleteWallet(walletId:) to guard that
persistenceHandler is present and identityIds were resolved before performing
any destructive Keychain operations—either make persistenceHandler mandatory for
this path (throw an error if nil) or explicitly fail early when identityIds
cannot be retrieved, using the existing persistenceHandler and
identityIdsForWallet(...) check to decide whether to proceed with
KeychainManager.shared.deleteAllKeychainItems(...) and
deleteAllIdentityPrivateKeys(...).
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/manager/identity_sync.rs`:
- Around line 570-587: Currently persister.store(...) is called while holding
the self.state.write() lock which can deadlock; instead, inside identity_sync.rs
(around the code using state.get(&identity_id), existing_row, sentinel, and cs)
clone or take a snapshot of the changeset (cs) and any needed existing_row data
while holding the write lock, then drop the lock before calling
self.persister.store(sentinel, cs_snapshot), and after store returns reacquire
the write lock and re-check that the identity still exists
(state.get(&identity_id)) before applying any cache updates or mutations; ensure
you handle and log persister.store errors the same way but performed outside the
lock.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift`:
- Around line 149-191: The test testThrowingKeychainSweepsUseIsolatedService
currently only inserts one identity private key so
deleteAllIdentityPrivateKeys(forWalletId:) could accidentally delete all
identity_privkey.* rows; insert a second identity private key via
KeychainManager.storeIdentityPrivateKey with a different walletId (e.g.,
walletId2 and publicKey2) before calling
manager.deleteAllIdentityPrivateKeys(forWalletId: walletId) and after the
deletion assert that retrieveIdentityPrivateKey(publicKeyHex: publicKey2) still
returns non-nil while the original publicKey is nil, ensuring the sweep is
scoped to the targeted walletId.
- Around line 38-120: Add a transaction+TXO pair that is initially owned by the
wallet and therefore removed by the wallet cascade: create a new
PersistentTransaction named walletOwnedTx and a matching PersistentTxo named
walletOwnedTxo that is associated with the wallet (set whichever field ties a
txo to a wallet in your model—e.g., walletId or an address/ownership field) and
append walletOwnedTxo to walletOwnedTx.outputs, insert both into the context
alongside the existing objects before saving, then after calling
PlatformWalletPersistenceHandler.deleteWalletData(walletId:) assert that the
fetched PersistentTransaction list no longer contains walletOwnedTx (i.e.,
transactions count remains 1 and walletOwnedTx is gone). This ensures
transactions that only become orphaned by the wallet cascade are swept.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 755ba89b-4903-4bcd-a4b0-33c76ed16508
📒 Files selected for processing (11)
packages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/traits.rspackages/rs-platform-wallet/src/manager/identity_sync.rspackages/rs-platform-wallet/src/manager/wallet_lifecycle.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/KeyManagerTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift
Comment on lines
+416
to
+422
| if wallet_id != WalletId::default() { | ||
| if changeset.wallet_metadata.is_some() { | ||
| self.retired.write().remove(&wallet_id); | ||
| } else if self.retired.read().contains(&wallet_id) { | ||
| return Ok(()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Retirement gate has a race window against concurrent retire_wallet.
store() only checks retired once up front, then runs callback persistence and pending merge later. If retire_wallet() runs between those points, stale writes can still be persisted for a just-retired wallet. This undermines the “drop post-delete writes” guarantee.
Consider a per-wallet lifecycle lock/state machine so retirement check + persistence path are coordinated atomically for non-registration writes.
🤖 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/rs-platform-wallet-ffi/src/persistence.rs` around lines 416 - 422,
store() currently only checks the retired set once and can persist changes if
retire_wallet() runs concurrently, allowing stale writes for just-retired
wallets; modify the persistence path to coordinate retirement and writes
atomically by introducing a per-wallet lifecycle lock or state machine (e.g., a
per-wallet mutex/atomic state) and use it in both store() and retire_wallet() so
that store() acquires the wallet lock/state before running the callback and
verifies retirement state again (or aborts) before committing pending merges;
ensure you reference and protect the same retired set and the code paths that
examine changeset.wallet_metadata and perform pending merges so no post-retire
writes can be committed.
Without a `ModelContainer`-backed persistence handler, the per-identity Keychain sweep can't resolve the wallet's identity ids and the loop silently skips, leaving `privkey_<identityHex>_*` and `specialkey_<identityHex>_*` rows behind in the Keychain. Guard at the top of `deleteWallet`, before the destructive FFI call, so the caller gets a clear `invalidHandle` error instead of a partially completed wipe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Review GateCommit:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- Around line 350-361: The PlatformWalletManager is removing the wallet via
platform_wallet_manager_remove_wallet and updating wallets before retrieving
per-identity IDs, which can leave a partial state if
persistenceHandler.identityIdsForWallet(walletId:) later throws; move the try
persistenceHandler.identityIdsForWallet(walletId: walletId) call to occur before
calling platform_wallet_manager_remove_wallet and before
wallets.removeValue(forKey:), so identityIds are resolved (and any errors
surface) prior to the FFI removal and in-memory mutation in the removeWallet
flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3bbda40-bf4e-4618-8b7b-b6176b0cf78a
📒 Files selected for processing (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
Comment on lines
+350
to
+361
| try walletId.withUnsafeBytes { raw in | ||
| guard let base = raw.baseAddress?.assumingMemoryBound(to: FFIByteTuple32.self) else { | ||
| throw PlatformWalletError.nullPointer( | ||
| "wallet_id buffer base address was nil" | ||
| ) | ||
| } | ||
| try platform_wallet_manager_remove_wallet(handle, base).check() | ||
| } | ||
|
|
||
| wallets.removeValue(forKey: walletId) | ||
|
|
||
| let identityIds = try persistenceHandler.identityIdsForWallet(walletId: walletId) |
Contributor
There was a problem hiding this comment.
Resolve identityIds before the destructive FFI removal.
If Line 361 throws after Line 356 succeeds, the wallet is already gone from Rust and wallets, but the per-identity keychain sweep never runs. Fetch the identity ids first so SwiftData read failures fail fast instead of turning into a partial wipe.
Suggested change
+ let identityIds = try persistenceHandler.identityIdsForWallet(walletId: walletId)
+
try walletId.withUnsafeBytes { raw in
guard let base = raw.baseAddress?.assumingMemoryBound(to: FFIByteTuple32.self) else {
throw PlatformWalletError.nullPointer(
"wallet_id buffer base address was nil"
)
}
try platform_wallet_manager_remove_wallet(handle, base).check()
}
wallets.removeValue(forKey: walletId)
-
- let identityIds = try persistenceHandler.identityIdsForWallet(walletId: walletId)
for identityId in identityIds {
try KeychainManager.shared.deleteAllKeychainItems(forIdentityId: identityId)
}🤖 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 350 - 361, The PlatformWalletManager is removing the wallet via
platform_wallet_manager_remove_wallet and updating wallets before retrieving
per-identity IDs, which can leave a partial state if
persistenceHandler.identityIdsForWallet(walletId:) later throws; move the try
persistenceHandler.identityIdsForWallet(walletId: walletId) call to occur before
calling platform_wallet_manager_remove_wallet and before
wallets.removeValue(forKey:), so identityIds are resolved (and any errors
surface) prior to the FFI removal and in-memory mutation in the removeWallet
flow.
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
2 participants
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
A wallet deletion through the SDK left orphan data behind: SwiftData rows the
@Relationshipcascade graph doesn't reach (PersistentTransaction,PersistentPendingInput,PersistentTokenBalance), in-memory Rust manager state, per-identity Keychain entries, and the shared network sync checkpoint. The next wallet created on the same network observed ghost rows from the deleted one. The same gap was present in the SDK's own example app — itsWalletDetailView.deleteWallet()carried aTODOacknowledging the missingPlatformWalletManagerremoval API.What was done?
Adds
PlatformWalletManager.deleteWallet(walletId:) throws, a single SDK call that wipes a wallet's complete footprint:platform_wallet_manager_remove_walletFFI drops the wallet from the in-memory map, snapshots its identities, and unregisters them fromIdentitySyncManagerso per-identity token-balance polling stops.retire_walletconcept — non-registration writes for retired wallet ids are dropped at the Rust→Swift boundary. Registration changesets auto-clear the retired flag, so re-importing the same seed works. This closes the race where Rust tasks already in flight when the delete fired could resurrect rows in SwiftData viaensureWalletRecord.IdentitySyncManager::apply_fresh_balancesre-checks the live state under its write lock before persisting, so a balance fetched mid-sync for a just-unregistered identity is dropped on the floor..nullify; this explicit path takes them down), sweepsPersistentPendingInputby walletId denorm, sweepsPersistentTransactionrows now reachable by nothing, sweepsPersistentTokenBalancefor the cascaded identity ids, and deletes the network sync state row only when no sibling wallet remains on that network.privkey_*+specialkey_*rows and wallet-derivedidentity_privkey.<path>rows, thenWalletStoragemetadata then mnemonic last (mnemonic-as-retry-anchor — if any earlier step fails, the pre-flight on retry still sees it).The example app's
WalletDetailViewbecomes the first consumer;dashwallet-iosfollows.How Has This Been Tested?
WalletDeletionTests(Swift, against in-memoryModelContainer): cascade + orphan-tx sweep, idempotency under double-call, network-sync conditional cleanup (both last-wallet and sibling-remains branches), and per-scheme Keychain sweeps with an isolated keychain service.rs-platform-wallet-ffi:retired_wallet_drops_non_registration_store_and_flushandregistration_store_clears_retirementpin the retire/auto-unretire semantics.cargo check -p platform-wallet,cargo clippy -p platform-wallet-fficlean.build_ios.sh --target sim --profile devsucceeds with-warnings-as-errors.WalletDeletionTestspass underxcodebuild test.Worth a simulator smoke pass before merging: create a wallet, sync a bit, delete it, create another on the same network, confirm StorageExplorerView shows zero ghosts for the deleted walletId across every
Persistent*table.Breaking Changes
KeychainManager.deleteAllPrivateKeys(for:)andKeychainManager.deleteAllIdentityPrivateKeys(forWalletId:)are nowthrows(were-> Boolpreviously, swallowing keychain errors). External consumers — includingdashwallet-ios— will need totrythe calls.Conventional-commit type is
feat, notfeat!, because the breaking surface is limited to two helper methods whose previousBoolreturn value didn't distinguish "nothing to delete" from "delete failed" — call sites that relied on the boolean were already silently wrong. Happy to retitle tofeat!if reviewers prefer.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Behavior Changes
Tests