fix(sdk): wallet-flow network fixes for SwiftExampleApp#3772
fix(sdk): wallet-flow network fixes for SwiftExampleApp#3772QuantumExplorer wants to merge 11 commits into
Conversation
Three independent wallet-flow bugs surfaced while testing on non-active networks: - SDK.swift: the devnet-only `platformQuorumURL` override was applied to every network, leaking a non-https http quorum URL into mainnet/testnet SDK builds. Rust's trusted context provider rejects that, so mainnet/testnet wallet creation silently failed. Gate the override behind `useOverrideAddresses` so mainnet/testnet use the SDK's automatic (canonical) quorum endpoints. - CreateWalletView.swift: on create failure the error alert is bound to CreateWalletView, but the pushed SeedBackupView sat on top of it with its submit button stuck disabled — the user got no feedback. Pop the backup screen in the catch block so the alert is visible. - SearchWalletsForIdentitiesView.swift / IdentitiesContentView.swift: the Re-scan for Identities picker used an unfiltered @query and listed wallets from every network. Filter to the active network (query-all + computed-filter, matching WalletsContentView) and re-inject platformState into the sheet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR refactors multi-network wallet support across the Swift SDK and example app. The data model shifts PersistentWallet uniqueness from per- ChangesMulti-network wallet architecture and UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Supporting per-network wallet infrastructure that the create-wallet and rescan fixes build on: - PersistentWallet: make rows unique per (walletId, network) and add `predicate(walletId:)` / `predicate(walletId:network:)` helpers so the same mnemonic can live on multiple networks without colliding. - PlatformWalletPersistenceHandler / PlatformWalletManager: scope persister wallet lookups to the manager's network so a per-network manager only restores its own rows. - WalletDetailView: implement the per-network "+" add-to-network action so an existing wallet can be added to another network. - IdentitiesContentView: take an explicit `network` and scope its identities @query to it; IdentitiesView passes the active network. - ContentView: thread the active network through. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
✅ Review complete (commit 92b6b28) |
|
✅ 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: "f5e7320fb4595c1d59824f08b63b277ae6ec80885693f579cc900d060bc2bbcf"
)Xcode manual integration:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift (1)
746-752: ⚡ Quick winSilent error handling may hide real failures from the user.
The catch block logs all errors but shows no UI feedback. While "already exists" is a valid no-op, other failures (network issues, Rust panics, etc.) will silently fail, leaving the user confused why the "+" button didn't enable the network.
Consider distinguishing "already exists" from other errors to surface actionable feedback:
Proposed improvement
} catch { - // An "already exists" throw means the wallet is already on - // this network — treat as a no-op and just refresh below. - SDKLogger.error( - "enableNetwork(\(network.displayName)) create returned: \(error.localizedDescription)" - ) + let message = error.localizedDescription + // "already exists" is a no-op; other errors should surface + let isAlreadyExists = message.lowercased().contains("already exists") + if !isAlreadyExists { + SDKLogger.error( + "enableNetwork(\(network.displayName)) failed: \(message)" + ) + errorMessage = "Failed to enable \(network.displayName): \(message)" + showError = true + return + } }🤖 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/SwiftExampleApp/Core/Views/WalletDetailView.swift` around lines 746 - 752, The catch in WalletDetailView's enableNetwork call currently logs every error and treats all failures as no-ops; update the catch in the enableNetwork (or the closure where SDKLogger.error is called) to inspect the thrown error (e.g., error.localizedDescription or error code) and only suppress/log silently when it indicates "already exists" (or equivalent SDK-specific error type/enum); for any other error, propagate user-facing feedback by triggering the view's error UI (e.g., set a `@State` showAlert / errorMessage, call presentError, or dispatch to main to update UI) so the user sees a clear failure message while still refreshing on the benign "already exists" case.
🤖 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/PlatformWalletPersistenceHandler.swift`:
- Around line 2643-2645: The deleteWalletData path only scopes the
PersistentWallet lookup via walletRecordPredicate(walletId:) but still deletes
child rows by walletId alone; update deleteWalletData so all deletions
(PersistentTxo, PersistentPendingInput, PersistentAssetLock) include the network
scope as well (e.g., add networkId/network predicate) similar to the wallet
lookup. Locate deleteWalletData and change the predicates used for
FetchDescriptor/PersistentTxo, PersistentPendingInput, and PersistentAssetLock
to combine walletId AND network (or reuse an updated walletRecordPredicate that
accepts network), ensuring child-table deletions are network-scoped and do not
wipe sibling-network state.
- Around line 423-437: walletRecordPredicate currently scopes by network when
self.network is set but walletNetwork(walletId:) still fetches by walletId-only;
update walletNetwork(walletId:) to use walletRecordPredicate(walletId:) for its
fetch so it selects the network-scoped PersistentWallet row when self.network is
non-nil, and audit other fetches used by persistSyncState, identity/token
writes, and deriveAndStoreIdentityKey to replace any walletId-only queries with
walletRecordPredicate to avoid cross-network row leakage.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentitiesView.swift`:
- Around line 26-31: hasAnyPrivateKey currently checks only that identifiers
(identity.votingPrivateKeyIdentifier, identity.ownerPrivateKeyIdentifier,
identity.payoutPrivateKeyIdentifier) are non-nil, which can incorrectly report
keys present even if the keychain item is missing; update hasAnyPrivateKey to
verify actual key presence in the keychain for each non-nil identifier (e.g.,
call your Keychain helper or use SecItemCopyMatching for each identifier) and
return true only if at least one identifier both exists and has a corresponding
key entry; alternatively remove this shortcut and call the existing concrete
key-existence check used elsewhere so the view reflects real keychain state.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- Around line 746-752: The catch in WalletDetailView's enableNetwork call
currently logs every error and treats all failures as no-ops; update the catch
in the enableNetwork (or the closure where SDKLogger.error is called) to inspect
the thrown error (e.g., error.localizedDescription or error code) and only
suppress/log silently when it indicates "already exists" (or equivalent
SDK-specific error type/enum); for any other error, propagate user-facing
feedback by triggering the view's error UI (e.g., set a `@State` showAlert /
errorMessage, call presentError, or dispatch to main to update UI) so the user
sees a clear failure message while still refreshing on the benign "already
exists" case.
🪄 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: 62019d46-f017-434b-846d-1acbfb8ac91c
📒 Files selected for processing (10)
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/Sources/SwiftDashSDK/SDK.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentitiesView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/SearchWalletsForIdentitiesView.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR introduces multi-network wallet support by making PersistentWallet unique on (walletId, networkRaw) and giving each persistence handler a bound network. The wallet-row scoping is correct, but several downstream paths weren't updated to match the new invariant: deriveAndStoreIdentityKey still uses the network-agnostic PersistentWallet.predicate(walletId:) (line 1815) and can derive keys on the wrong network; and persister caches/cleanups keyed only by walletId (PersistentTxo, PersistentAssetLock, PersistentPlatformAddress, PersistentShieldedSyncState) now leak across networks because the same walletId can have rows on more than one. The new multi-network create + add-to-network UX also masks every per-network failure as a benign 'already exists' case, producing silent partial creates and writing keychain metadata claiming networks were enabled when they weren't.
🔴 4 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
3 additional finding(s) omitted (not in diff).
🤖 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/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1815-1824: deriveAndStoreIdentityKey can pick the wrong network row after the (walletId, networkRaw) split
After this PR, `PersistentWallet` is unique on `(walletId, networkRaw)` and every other lookup on this handler routes through `walletRecordPredicate(walletId:)` so it scopes to `self.network` when set. This call site still uses the bare `PersistentWallet.predicate(walletId:)` and takes `.first`, so a per-network handler whose wallet also exists on another network can resolve to the other network's row, read `persistentWallet.network` from it, and feed the wrong `coin_type` into `KeyDerivation.getIdentityAuthenticationPath`. The resulting identity authentication path is built for the wrong chain and the private-key bytes written to Keychain are unusable for the on-chain identity. This is exactly the breakage `walletRecordPredicate` was introduced to prevent — apply it here too.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:2722-2748: Per-walletId caches and cleanups leak across networks now that walletId is no longer unique
`PersistentWallet` was made unique on `(walletId, networkRaw)`, but `PersistentTxo`, `PersistentPendingInput`, `PersistentAssetLock`, `PersistentPlatformAddress`, and `PersistentShieldedSyncState` have no network column and are still fetched/deleted by `walletId` alone. With the new ability to materialize the same mnemonic on multiple networks (`CreateWalletView` loop, `WalletDetailView.enableNetwork`), this contaminates several flows that share one SwiftData store across per-network handlers:
- `deleteWalletData` (this hunk) deletes every `PersistentTxo` / `PersistentPendingInput` / `PersistentAssetLock` with the matching `walletId` — wiping another network's UTXOs, pending inputs, and asset-lock state when the user removes the wallet from one network.
- `loadCachedBalancesOnQueue` (lines 241-258) and `loadCachedAssetLocksOnQueue` (lines 192-210) feed Rust restore state from rows that may belong to a different network's wallet of the same id.
- The unspent-TXO bucket map in `loadWalletList` (lines 2941-2980) buckets by raw `walletId`, so restore can re-seed Rust with the wrong network's UTXOs.
- `PersistentShieldedSyncState` is `#Unique([\.walletId, \.accountIndex])` (no network), so adding the wallet to a second network silently shares the shielded watermark.
Either thread `networkRaw` onto these models and scope every lookup/delete by `(walletId, networkRaw)`, or scope deletion/load by the handler's `self.network` when it is set.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift:343-417: Multi-network create reports success when only some selected networks succeeded
The per-network loop catches *every* error from `createWallet` and treats it as a benign 'already exists'. Real failures (`backgroundManager(for:)` throwing, Rust manager construction failures, persistence errors, endpoint misconfiguration, etc.) on some networks are masked: as long as one network succeeds, `createdWalletId` is non-nil and the flow continues straight into `storage.setMetadata(...)`, writing keychain metadata that claims `networks: selectedNetworks.map { $0.networkName }` were all enabled. The dialog then dismisses as if the operation fully succeeded. The user has no UI signal that some of the ticked networks were skipped, and orphan-recovery later will believe the wallet was added everywhere it was selected. Inspect the error and only swallow a real 'already-exists' variant (preferred), or accumulate the per-network failures, report them in the UI, and only stamp `networks:` with the networks that actually got a row.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:739-755: enableNetwork swallows every create error as 'already exists' with no UI feedback
The catch block treats any throw from `mgr.createWallet(...)` as the benign 'already on this network' case and only logs through `SDKLogger.error`. Genuine failures — Rust manager construction failing, persistence errors, network/quorum config rejected by the trusted-context provider — are silently swallowed: `errorMessage` / `showError` are never set on the create path, `loadNetworkStates()` simply observes the row is missing, and the toggle quietly reverts. The user gets zero signal that ticking the network did nothing. This is also asymmetric with the mnemonic-retrieval branch above (which does surface through `errorMessage`/`showError`). Inspect the error and only treat the actual 'already exists' variant as a no-op; surface everything else through the existing error UI.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:723-752: Add-to-network pulls the mnemonic across the FFI boundary — anti-precedent per swift-sdk/CLAUDE.md
`enableNetwork` retrieves the mnemonic via `WalletStorage().retrieveMnemonic(for:)` and hands it back to `mgr.createWallet(mnemonic:network:name:)` to materialize the wallet on another network. `packages/swift-sdk/CLAUDE.md` calls out this exact pipeline as the anti-precedent: 'Don't fetch the mnemonic from Keychain, hand it back to Rust, wait for derived bytes, and write those to Keychain — that's the same pipeline orchestrated on the wrong side. If you need this pipeline, add a single FFI entry point that does the whole thing.' The `CreateWalletView` loop has the same shape (Swift-side iteration deciding which networks to materialize and reusing `createWallet(mnemonic:)` N times). The original TODO acknowledged that the proper fix is a Rust-side `platform_wallet_add_to_network` (or `platform_wallet_create_on_networks([networks])`) FFI; the stop-gap re-exposes the seed phrase across the boundary on every add and cements the wrong shape. Push this down to `platform-wallet` so Swift only persists what Rust returns.
…l key check CodeRabbit review on #3772: - PlatformWalletPersistenceHandler.walletNetwork(walletId:): use walletRecordPredicate so the network resolved for sync-state / identity / token writes and key derivation is THIS manager's network, not an arbitrary sibling row when the same seed lives on two networks. - deleteWalletData: the txo / pending-input / asset-lock child tables are keyed by the network-independent walletId and carry no network column, so they're shared across networks. Only wipe them when the row being deleted is the wallet's LAST remaining per-network row; otherwise deleting one network erased a sibling network's cached state. The wallet row itself stays network-scoped. - WalletDetailView.enableNetwork: only swallow an "already exists" throw (a genuine no-op); surface any other failure via the error alert instead of silently doing nothing. - IdentitiesView.hasAnyPrivateKey: a stored *PrivateKeyIdentifier only means an identifier string was persisted — the Keychain item can be missing. Confirm actual presence via hasSpecialKey(identifier:) before counting it, so the "No Keys" badge isn't wrongly hidden. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per CodeRabbit's alternative suggestion on #3772: a stored *PrivateKeyIdentifier only means an identifier string was persisted — the backing Keychain item can be missing. Remove the non-nil-identifier shortcut and rely solely on the existing concrete key-presence checks (hasPrivateKey / hasIdentityPrivateKey) so the 'No Keys' badge reflects actual Keychain state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Reconciling the six prior PastaClaw findings at HEAD 207b4f8: #4 is fully fixed (enableNetwork now distinguishes 'already exists' from real errors). #1, #3, #5, and #6 are carried forward unchanged. #2 is partially fixed on the delete path (per-walletId wipes now gated on 'last network row'), but the symmetric read paths (loadCachedAssetLocksOnQueue, loadCachedBalancesOnQueue, loadShieldedSyncStates / loadShieldedNotes, the loadWalletList TXO bucketing) still bucket by network-independent walletId, so a wallet enabled on multiple networks still cross-feeds cached state across managers. No new defects were introduced by the latest delta. Three blocking issues remain in scope for this PR (which is what makes multi-network walletIds load-bearing).
🔴 3 blocking | 💬 1 nitpick(s)
4 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1815-1824: deriveAndStoreIdentityKey still resolves the wallet row by bare walletId — can derive identity keys under the wrong network's coin_type
Carried forward from prior finding #1 — STILL VALID. After this PR made `PersistentWallet` unique on `(walletId, networkRaw)`, every other lookup in this handler was routed through `walletRecordPredicate(walletId:)` so it intersects with `self.network` (most recently the explicit fix to `walletNetwork(walletId:)` at line 4083, with the same rationale). This call site is the last holdout: it uses `PersistentWallet.predicate(walletId: walletId)` and then `.first`, which arbitrarily picks any row matching the id. When the same mnemonic is enabled on more than one network — exactly what this PR enables — the per-network handler's persister callback can resolve to a sibling-network row, read `persistentWallet.network` from it, and feed the wrong `Network` into `KeyDerivation.getIdentityAuthenticationPath` at line 1852. That changes the DIP-9 `coin_type` segment, so `key_wallet_derive_private_key_from_seed` returns a 32-byte scalar that does not correspond to the public key registered for the identity on this manager's network. The derived bytes are then stored in Keychain under the identity id with no public/private match check, so the corruption is silent until a signature later fails. The fix is one token — match every other call site in the file.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:192-260: Read-side caches (asset locks, platform addresses, shielded sync state, restored UTXOs) still bucket by network-independent walletId — cross-network state leaks back across the FFI on load
Carried forward from prior finding #2 — write/delete paths were narrowed by this delta (per-walletId wipes are now gated on `isLastNetworkRow` at lines 2733-2767, good), but the symmetric read paths still match by `walletId` alone. With this PR's invariant that the same walletId can legitimately live on multiple networks, those reads cross-feed state between per-network managers:
• `loadCachedAssetLocksOnQueue` (line 192-211) uses `PersistentAssetLock.predicate(walletId:)`; `unused_asset_locks` for one network can rehydrate into the other.
• `loadCachedBalancesOnQueue` (line 241-260) uses `PersistentPlatformAddress.predicate(walletId:)`; platform-address balances from a foreign network restore into this network's provider.
• `loadWalletList` TXO bucketing (line 2960-3000) groups all unspent `PersistentTxo` by `row.walletId` only, so the per-wallet UTXO buffer Rust receives can include rows that belong to a different network's wallet sharing the id.
• `loadShieldedSyncStates` / `loadShieldedNotes` (lines 2417, 2525) DO call `inNetworkWalletIds()` — but because both networks' `PersistentWallet` rows share the same walletId, both managers' `inNetworkWalletIds()` sets include that id and the filter no longer separates them. Same-mnemonic-on-multiple-networks watermarks/notes are still shared. The fix is structural: add `networkRaw` to `PersistentTxo`, `PersistentPendingInput`, `PersistentAssetLock`, `PersistentPlatformAddress`, and `PersistentShieldedSyncState` (matching what `PersistentWallet` just got), backfill from the owning wallet, and switch these lookups to network-scoped predicates. This is the same hazard the delete-path fix was protecting against, just on the read side — and the per-network-wallets feature is what makes it exploitable.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift:343-417: Multi-network create still reports success when only some selected networks succeeded; keychain metadata records networks that were never created
Carried forward from prior finding #3 — STILL VALID. The companion fix you just landed in `WalletDetailView.enableNetwork` (lines 746-763) already does the right thing: branch on whether the error description contains 'already exists' and surface anything else through the alert. This call site was not migrated. The loop here catches every `createWallet` error and logs it identically to the benign 'already exists' case; as long as any single ticked network succeeds, the flow dismisses the sheet, writes `WalletKeychainMetadata.networks = selectedNetworks.map { $0.networkName }` (line 409), and reports success. So the very class of bug this PR was originally written to fix (devnet quorum URL leaking into mainnet/testnet manager construction) can silently leave the user with no row on the affected network while orphan-recovery metadata claims one exists. Mirror the WalletDetailView idiom: distinguish 'already exists' from real failures, only stamp `networks` for those that actually succeeded, and surface a partial-failure error to the user.
thepastaclaw review on #3772: deriveAndStoreIdentityKey resolved the wallet's network with the bare PersistentWallet.predicate(walletId:), which can pick a sibling network's row now that the same walletId can have one row per network — feeding the wrong coin_type into KeyDerivation.getIdentityAuthenticationPath and writing key bytes unusable for the on-chain identity. Use walletRecordPredicate so the lookup is scoped to this handler's network, matching every other PersistentWallet fetch on the handler (the lone remaining bare lookup, walletRowCountAcrossNetworks, is deliberately cross-network). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the re-review. Responses to the new findings: Fixed (076c0a0):
Already addressed in prior commits (threads resolved):
Acknowledged, proposing follow-up rather than expanding this PR:
Branch tip 076c0a0, build green. |
…iew) thepastaclaw review on #3772: the multi-network create loop caught every createWallet error and treated it like the benign "already exists" case, so a real failure on one network (e.g. the devnet quorum URL leaking into mainnet/testnet manager construction — the very bug this PR fixes) was masked: as long as one network succeeded the sheet dismissed as success and keychain metadata claimed every ticked network was enabled. Mirror the WalletDetailView.enableNetwork idiom: - only "already exists" counts a network as present (benign no-op); - genuine failures are collected and surfaced via the error alert; - keychain metadata records only the networks the wallet actually has a row on (presentNetworks), not every ticked network; - a partial create (wallet exists but not on all ticked networks) throws a descriptive error instead of silently dismissing as success. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/PlatformWalletPersistenceHandler.swift`:
- Around line 1814-1820: The current method performs mnemonic→seed→DIP-9
path→private-key derivation in Swift (around the wallet lookup using
walletRecordPredicate and the FetchDescriptor<PersistentWallet> /
walletDescriptor), which violates the rule to keep derivation inside
platform-wallet; refactor by removing all mnemonic/seed/path/key derivation from
PlatformWalletPersistenceHandler and instead call a new or existing
platform-wallet FFI entry (exposed from Rust) that accepts the walletId (and
network scope) and returns the derived key material or a handle; update the code
that currently reads from Keychain and derives keys to call that FFI function
and persist only the returned key material, leaving walletRecordPredicate and
FetchDescriptor logic intact but delegating all derivation to the Rust
platform-wallet API.
- Around line 2737-2743: The current logic computes isLastNetworkRow solely from
the siblingDescriptor count which can be true even when walletRow is nil; update
the check so you only treat it as the last network row when the handler actually
owns a row. Concretely, ensure walletRow is non-nil before using the
cross-network count (e.g. compute isLastNetworkRow as walletRow != nil && ((try?
backgroundContext.fetchCount(siblingDescriptor)) ?? 0) <= 1) and use that
updated isLastNetworkRow where you decide to wipe shared
TXO/pending-input/asset-lock tables.
🪄 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: 6bc471fe-33d7-472f-862a-f1e7c5725b41
📒 Files selected for processing (3)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentitiesView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentitiesView.swift
| // testnet). Scope to THIS handler's network via | ||
| // `walletRecordPredicate` — the same `walletId` can now have | ||
| // a row per network, and a bare walletId-only fetch could | ||
| // resolve to a sibling network's row and derive the key on | ||
| // the wrong chain (unusable on-chain). | ||
| let walletDescriptor = FetchDescriptor<PersistentWallet>( | ||
| predicate: PersistentWallet.predicate(walletId: walletId) | ||
| predicate: walletRecordPredicate(walletId: walletId) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move identity-key derivation behind a Rust/platform-wallet FFI entry point.
This method still pulls the mnemonic from Keychain, derives the seed, builds the DIP-9 path, and derives the private key in Swift. The new network-scoped lookup helps correctness, but the full mnemonic → seed → path → key pipeline is still implemented on the Swift side instead of persisting only Rust-returned key material.
As per coding guidelines, "Do not build derivation paths in Swift", "Do not orchestrate multi-step derivation pipelines in Swift (mnemonic → seed → path → key → store)", and "Identity key derivation and any action using a derived key must route through platform-wallet, not be re-implemented in Swift".
🤖 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/PlatformWalletPersistenceHandler.swift`
around lines 1814 - 1820, The current method performs mnemonic→seed→DIP-9
path→private-key derivation in Swift (around the wallet lookup using
walletRecordPredicate and the FetchDescriptor<PersistentWallet> /
walletDescriptor), which violates the rule to keep derivation inside
platform-wallet; refactor by removing all mnemonic/seed/path/key derivation from
PlatformWalletPersistenceHandler and instead call a new or existing
platform-wallet FFI entry (exposed from Rust) that accepts the walletId (and
network scope) and returns the derived key material or a handle; update the code
that currently reads from Keychain and derives keys to call that FFI function
and persist only the returned key material, leaving walletRecordPredicate and
FetchDescriptor logic intact but delegating all derivation to the Rust
platform-wallet API.
There was a problem hiding this comment.
Acknowledged, not addressed in this PR. You're right that the mnemonic→seed→DIP-9 path→key pipeline belongs in platform-wallet per the swift-sdk CLAUDE.md rules. This code predates this PR (the PR only changed the wallet-row lookup from a bare predicate to walletRecordPredicate for network-correctness); pushing the whole derivation behind a Rust FFI entry point (platform_wallet_derive_identity_key or similar) is a cross-language refactor that belongs in its own PR with the swift-rust-ffi-engineer, not this Swift-only bugfix. Tracking as a follow-up.
There was a problem hiding this comment.
@QuantumExplorer, understood — the scope of this PR is well-defined (network-scoped wallet-row lookup) and the derivation-pipeline refactor is legitimately a separate cross-language effort. Would you like me to open a GitHub issue to track the follow-up (platform_wallet_derive_identity_key FFI entry + removing the Swift-side mnemonic→seed→path→key pipeline from PlatformWalletPersistenceHandler)?
✏️ Learnings added
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 3772
File: packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1814-1820
Timestamp: 2026-06-01T07:59:33.680Z
Learning: In dashpay/platform, the Swift-side `deriveAndStoreIdentityKey` method in `PlatformWalletPersistenceHandler.swift` still implements the full mnemonic→seed→DIP-9 path→private-key derivation pipeline in Swift instead of delegating to a Rust FFI entry point. This is known technical debt that predates PR `#3772`; the planned fix is a new `platform_wallet_derive_identity_key` (or similar) FFI entry exposed from `rs-platform-wallet-ffi` that accepts walletId + network scope and returns the derived key material, removing the mnemonic/seed/path/key logic from Swift. This cross-language refactor is tracked as a follow-up PR requiring coordination with the swift-rust-ffi engineer and should not be flagged again on unrelated Swift-only PRs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 3581
File: packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swift:30-30
Timestamp: 2026-05-04T18:54:22.991Z
Learning: In the dashpay/platform repo, SwiftData schema changes in the Swift SDK are intentionally not migrated. When reviewing code in packages/swift-sdk/**/*.swift, do not flag schema updates (e.g., adding new Model types or changing relationships) for missing MigrationStage entries or missing VersionedSchema bumps—this repo expects dev stores to be rebuilt from scratch on schema changes, and SwiftData migrations are not required.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding reconciliation for 076c0a0: carried forward STILL VALID prior findings: #2:network-independent-wallet-caches-read-side-leak; fixed/outdated prior findings: #1:derive-identity-key-bare-walletid-network-row, #3:create-wallet-partial-network-success-metadata, #4:enable-network-mnemonic-crosses-ffi-boundary, #5:identity-row-private-key-probes-sync-hot-path; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.
Latest delta (commits 65807e7 and 076c0a0) fixed the prior derive-identity-key cross-network lookup, but three prior blockers persist (read-side caches scoped only by walletId, multi-network create swallows per-network failures and writes optimistic keychain metadata, enableNetwork still round-trips the mnemonic through Swift), and the over-aggressive simplification of hasAnyPrivateKey introduced a new UI regression that mislabels masternode/evonode identities loaded via LoadIdentityView as having no keys.
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 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/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:187-269: Read-side caches (asset locks, platform addresses, shielded sync state, restored UTXOs) still bucket by network-independent walletId — cross-network state leaks back across the FFI on load
Carried forward from prior finding #2. The latest delta narrowed write/delete behavior, but the symmetric read paths still match by `walletId` alone while this PR makes the same walletId valid on multiple networks. `loadCachedAssetLocksOnQueue` uses `PersistentAssetLock.predicate(walletId:)`, `loadCachedBalancesOnQueue` uses `PersistentPlatformAddress.predicate(walletId:)`, `loadWalletList` groups unspent `PersistentTxo` by raw walletId, and shielded sync-state/note reads still cannot separate two network rows with the same walletId because the child models do not carry networkRaw. Add networkRaw/network-scoped predicates to these child cache models before feeding cached state back across the FFI.
Current-head validation: STILL VALID. Current child cache models still lack a networkRaw/network discriminator (PersistentTxo, PersistentAssetLock), and read paths still key cache state by walletId.
|
Correction to my review at #3772 (review): the submitted top-level body undercounted the carried-forward findings while reconciling the prior round. The Current-head reconciliation:
I am not opening duplicate inline threads for findings already raised on older review rounds; this correction records the verifier-kept set for the exact reviewed SHA. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta in CreateWalletView correctly surfaces partial multi-network creates and only stamps keychain metadata for networks where the row landed. However, the prior blocking finding about child-cache network scoping is only partially addressed: shielded loaders now filter via inNetworkWalletIds(), but asset-lock, platform-address, and unspent-UTXO read paths used during loadWalletList still match by raw walletId, and PersistentPlatformAddress is globally unique on address/addressHash — so once one mnemonic is added to two networks, write-side uniqueness collisions and cross-network read leaks corrupt the rehydrated FFI snapshot. Two smaller suggestions on the new CreateWalletView create flow.
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:186-260: Carried-forward: asset-lock, platform-address, and UTXO caches still bucket by network-independent walletId
Re-validated at cee4b2cb against the prior finding from 076c0a0a — partially fixed (shielded note/sync-state loaders now filter via `inNetworkWalletIds()`), but the remaining read paths used during multi-network restore are still keyed by raw `walletId`:
- `loadCachedAssetLocksOnQueue(walletId:)` at PlatformWalletPersistenceHandler.swift:192 uses `PersistentAssetLock.predicate(walletId:)`. `PersistentAssetLock` has no `networkRaw` column, so when the same `walletId` has rows persisted from a mainnet handler and a testnet handler, the mainnet loader rehydrates testnet's tracked asset locks. `loadWalletList` invokes this per-restored wallet at line 3243 and feeds the result into `entry.tracked_asset_locks` — Rust then revives the foreign-network locks into `unused_asset_locks` for the wrong manager.
- `loadCachedBalancesOnQueue(walletId:)` at line 241 uses `PersistentPlatformAddress.predicate(walletId:)`. The bucketed call at 3086 mixes cached balances from another network into `entry.platform_address_balances`.
- The unspent UTXO fetch at 2964–3004 grabs every `isSpent == false` `PersistentTxo` row and groups by `row.walletId` (or `row.account.wallet.walletId` for legacy rows) with no network constraint, so a wallet that exists on two networks gets the other network's UTXOs routed into its restore buffer at line 3213.
Compounding this, `PersistentPlatformAddress` has `@Attribute(.unique) public var addressHash: Data` (PersistentPlatformAddress.swift:40). `addressHash` is the raw 21-byte payload, which is network-independent — the same derived key collides across networks regardless of the differing bech32m HRP on the `address` string. Once one mnemonic exists on multiple networks the second handler's write either conflicts or overwrites the first handler's row, so `loadCachedBalancesOnQueue(walletId:)` can only ever restore whichever network wrote last.
The write side is otherwise network-scoped (each `PlatformWalletManager` owns a per-network handler), so the corruption only manifests when the same walletId has been persisted on more than one network — which is exactly the scenario this PR introduces. Apply the same `inNetworkWalletIds()` filter the shielded loaders use AND add a `networkRaw` discriminator to `PersistentAssetLock`, `PersistentPlatformAddress`, and `PersistentTxo` so their uniqueness/predicates can scope to `(walletId, networkRaw)` the way `walletRecordPredicate` already does for `PersistentWallet`.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift:385-396: All-networks-already-exist path throws misleading "could not be created" error
When every selected network reports `"already exists"`, `presentNetworks` ends up populated but `createdWalletId` stays `nil` because it's only assigned on the success branch at line 363. The guard at 385 then throws `AllNetworksFailed` with an empty `detail` (since `failures` is empty), surfacing "Wallet could not be created on any selected network." even though the wallet actually exists on every ticked network — directly contradicting the inline comment at 367 ("An 'already exists' throw means the wallet is already on this network — benign, the row is present, so count it").
This is reachable any time a user re-imports an already-present mnemonic with the same network selections. Either resolve the walletId from `WalletStorage` / a SwiftData lookup when `createdWalletId` is `nil` but `presentNetworks` is non-empty (treat as a no-op success and proceed to metadata refresh + dismiss), or short-circuit before throwing when there are no real failures.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift:366-382: Brittle substring match on "already exists" to classify a benign error
The branch classifying the per-network failure as benign relies on a case-insensitive substring search for `"already exists"` against `error.localizedDescription`. This matches the current `PlatformWalletError::WalletAlreadyExists` Display string, but it's an implicit coupling: a tweak to the Rust error message, a localization layer, or any wrapping that prepends context can silently break the classification — partial creates would then be reported as full failures (or vice versa). The same pattern is repeated in `WalletDetailView.swift:752`. Prefer exposing a typed FFI error code / dedicated Swift error case (e.g. `PlatformWalletError.walletAlreadyExists`) and switching on it, with the string match kept only as a last-resort fallback.
| let message = error.localizedDescription | ||
| // An "already exists" throw means the wallet | ||
| // is already on this network — benign, the | ||
| // row is present, so count it. Any other | ||
| // error is a genuine failure to record. | ||
| if message.range(of: "already exists", options: .caseInsensitive) != nil { | ||
| presentNetworks.append(net) | ||
| SDKLogger.error( | ||
| "Wallet already present on \(net.displayName); continuing" | ||
| ) | ||
| } else { | ||
| failures.append((net, message)) | ||
| SDKLogger.error( | ||
| "Wallet creation failed for \(net.displayName): \(message)" | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Brittle substring match on "already exists" to classify a benign error
The branch classifying the per-network failure as benign relies on a case-insensitive substring search for "already exists" against error.localizedDescription. This matches the current PlatformWalletError::WalletAlreadyExists Display string, but it's an implicit coupling: a tweak to the Rust error message, a localization layer, or any wrapping that prepends context can silently break the classification — partial creates would then be reported as full failures (or vice versa). The same pattern is repeated in WalletDetailView.swift:752. Prefer exposing a typed FFI error code / dedicated Swift error case (e.g. PlatformWalletError.walletAlreadyExists) and switching on it, with the string match kept only as a last-resort fallback.
source: ['claude']
There was a problem hiding this comment.
Agreed it's brittle. Acknowledged but not changing in this PR — the proper fix is a typed FFI error code / dedicated PlatformWalletError.walletAlreadyExists case to switch on, which is an FFI surface change (rs-platform-wallet-ffi) beyond this Swift-only bugfix. The substring match (here and WalletDetailView) is the current best available signal; tracking the typed-error work as a follow-up.
Two defects in the prior review-round fixes, both caught by CodeRabbit / thepastaclaw on #3772: - deleteWalletData: `isLastNetworkRow` was computed from the sibling count alone, so a handler asked to delete a wallet it doesn't own (walletRow == nil) but whose sibling network still has a row (count == 1) would read as "last row" and wipe the shared txo / pending-input / asset-lock tables out from under the other network. Gate the last-row check on `walletRow != nil`. - CreateWalletView multi-network create: when every selected network reported "already exists", `createdWalletId` stayed nil (only set on the fresh-create branch) while `presentNetworks` was populated, so the guard threw "Wallet could not be created on any selected network" with an empty detail — contradicting the benign "already exists" handling. Re-importing an already-present mnemonic is now a no-op dismiss; the failure path only fires when there's a genuine (non-"already exists") failure and nothing succeeded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-validated cee4b2c's three prior findings against head 815751a: #1 (cross-network read-path leak via walletId-only cache keys) STILL VALID and confirmed in source at PlatformWalletPersistenceHandler.swift L192-211/L241-260 (asset-lock + balance loaders) and L2976-3016 + L3098/L3225/L3255 (load-wallet-list path). #2 (all-networks-already-exist throwing AllNetworksFailed) FIXED at CreateWalletView.swift L387-396 (now dismisses cleanly when failures.isEmpty). #3 (substring 'already exists' classifier) STILL VALID at CreateWalletView L371 and now duplicated into WalletDetailView L752 by this PR's new per-network enable path. No new latest-delta-only findings beyond the carried-forward ones. CodeRabbit returned no inline findings to react to.
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:186-260: Multi-network restore still cross-feeds asset locks, platform balances, and UTXOs across networks for same-mnemonic wallets
Carried-forward from cee4b2cb — re-validated at 815751a9 and STILL VALID. The new `isLastNetworkRow` guard added to `deleteWallet` (L2737-2783) explicitly documents that `PersistentAssetLock`, `PersistentTxo`, and `PersistentPendingInput` are keyed by network-independent `walletId` and shared by every per-network `PersistentWallet` row for the same mnemonic. That sharing is now fenced on the delete side, but the read paths used by multi-network restore still bucket by raw `walletId`:
- `loadCachedAssetLocksOnQueue(walletId:)` (L192-211) fetches via `PersistentAssetLock.predicate(walletId:)` with no network filter. The wallet-load callback rehydrates these into the Rust `unused_asset_locks` map at L3255-3261.
- `loadCachedBalancesOnQueue(walletId:)` (L241-260) fetches via `PersistentPlatformAddress.predicate(walletId:)` with no network filter. L3098 reseeds the network-bound provider with the union of every sibling network's cached balances/nonces.
- The unspent-UTXO bucketing pass at L2976-3016 buckets every `PersistentTxo` where `isSpent == false` by raw `walletId`, then `unspentBuckets[w.walletId]` at L3225 feeds `buildUtxoRestoreBuffer`.
Net effect: this PR's create flow (CreateWalletView writes the same `walletId` into per-network `PersistentWallet` rows for each selected network) will, on the next per-network manager load, hand each network's Rust manager the union of all networks' UTXOs, address balances, and tracked asset locks — exactly the corruption the new delete-side `isLastNetworkRow` guard was added to prevent on the write side. Add a network discriminator column to `PersistentTxo` / `PersistentPlatformAddress` / `PersistentAssetLock`, expose `predicate(walletId:network:)` variants, and scope every loader in this file to `self.network` when the handler is network-bound.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift:365-382: Benign 'wallet already exists' classification still relies on a substring match against a Rust display string, now duplicated into WalletDetailView
Carried-forward from cee4b2cb — re-validated and STILL VALID. The per-network create loop classifies a benign duplicate by `error.localizedDescription.range(of: "already exists", options: .caseInsensitive)` (L371) instead of a typed error. This branch controls whether `presentNetworks` includes the network and whether the all-failed `AllNetworksFailed` path fires — i.e. whether this PR's freshly-fixed 'no wallet could be created' regression stays fixed. Any rewording on the Rust side (`PlatformWalletError::WalletAlreadyExists` display impl), localization wrapper, or future error chaining silently flips a benign case into the `failures` bucket and re-surfaces the misleading 'Wallet could not be created on any selected network' alert.
This PR also propagates the same heuristic to `WalletDetailView.swift:752` in the new `enableNetwork(_:)` button — the per-network `+` action — so a single wording change now breaks two user-visible flows. Replace with a typed FFI error code / dedicated Swift error case for `WalletAlreadyExists` (e.g. matching on `PlatformWalletError.walletAlreadyExists` rather than the string) and keep the substring fallback only as a defense-in-depth check.
| } catch { | ||
| let message = error.localizedDescription | ||
| // An "already exists" throw means the wallet | ||
| // is already on this network — benign, the | ||
| // row is present, so count it. Any other | ||
| // error is a genuine failure to record. | ||
| if message.range(of: "already exists", options: .caseInsensitive) != nil { | ||
| presentNetworks.append(net) | ||
| SDKLogger.error( | ||
| "Wallet already present on \(net.displayName); continuing" | ||
| ) | ||
| } else { | ||
| failures.append((net, message)) | ||
| SDKLogger.error( | ||
| "Wallet creation failed for \(net.displayName): \(message)" | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Benign 'wallet already exists' classification still relies on a substring match against a Rust display string, now duplicated into WalletDetailView
Carried-forward from cee4b2c — re-validated and STILL VALID. The per-network create loop classifies a benign duplicate by error.localizedDescription.range(of: "already exists", options: .caseInsensitive) (L371) instead of a typed error. This branch controls whether presentNetworks includes the network and whether the all-failed AllNetworksFailed path fires — i.e. whether this PR's freshly-fixed 'no wallet could be created' regression stays fixed. Any rewording on the Rust side (PlatformWalletError::WalletAlreadyExists display impl), localization wrapper, or future error chaining silently flips a benign case into the failures bucket and re-surfaces the misleading 'Wallet could not be created on any selected network' alert.
This PR also propagates the same heuristic to WalletDetailView.swift:752 in the new enableNetwork(_:) button — the per-network + action — so a single wording change now breaks two user-visible flows. Replace with a typed FFI error code / dedicated Swift error case for WalletAlreadyExists (e.g. matching on PlatformWalletError.walletAlreadyExists rather than the string) and keep the substring fallback only as a defense-in-depth check.
source: ['claude', 'codex']
There was a problem hiding this comment.
Same finding as the existing thread on this file (#discussion_r3331111688) — acknowledged and deferred. The robust fix is a typed PlatformWalletError.walletAlreadyExists case to switch on (an rs-platform-wallet-ffi error-surface change), with the substring match kept only as a defense-in-depth fallback. Tracking as a follow-up rather than expanding this Swift-only bugfix PR; the substring check is the current best available signal in both CreateWalletView and WalletDetailView.
Adopts the network-scoped wallet id from rust-dashcore #793 (merged to dev as 3d0d5dcd) to structurally fix the carried-forward blocking finding #2: child caches keyed by the network-INDEPENDENT walletId (PersistentTxo / PersistentPendingInput / PersistentAssetLock / PersistentPlatformAddress / PersistentShieldedSyncState) cross-fed state between a single mnemonic's per-network wallets. With #793 the wallet id is network-scoped BY DEFAULT at construction (`Wallet::from_mnemonic` / `from_seed` fold a domain-tagged, wire-stable network byte into the digest), so the same mnemonic now yields a DISTINCT id per network. Every walletId-keyed structure becomes network-correct by construction — no networkRaw columns needed on the child models, and the leak is eliminated at the source. - Cargo.toml / Cargo.lock: pin all 8 rust-dashcore crates to 3d0d5dcd (the squash-merge of #793 on dev). - rs-platform-wallet: no behavior change needed — construction already stamps the scoped id. `register_wallet` carries an explanatory comment. Added two unit tests proving same-mnemonic → 4 pairwise- distinct ids across networks, and stable per-(seed,network). - CreateWalletView: the multi-network create now persists the mnemonic + metadata + `isImported` per FRESHLY-CREATED network's scoped walletId (each is independently recoverable), instead of once under a single shared id; metadata records just that network. All-already- exist is a benign dismiss. - WalletDetailView.enableNetwork: stores the mnemonic under the newly-enabled network's scoped walletId so that wallet's own keychain lookups resolve. - PlatformWalletManager.deleteWallet: comment updated for the scoped-id model (the walletRowCountAcrossNetworks guard stays correct). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Finding #2 (cross-network cache leak) — closed via network-scoped walletId (14d353e)Rather than add a This builds on dashpay/rust-dashcore#793 (now merged to
Verification: Rust + FFI build clean on the merged rev; the iOS app builds and launches clean. End-to-end multi-network create couldn't be shown in a freshly-wiped simulator (standing up a non-active network's manager needs per-network infra the clean sim lacks — mainnet connectivity prefetch, devnet quorum URL, regtest docker), which is orthogonal to the id scoping — hence the deterministic Rust-level proof. Still open (deliberately deferred, separate follow-ups — not closed by this commit):
Branch tip |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3772 +/- ##
============================================
- Coverage 87.19% 87.18% -0.01%
============================================
Files 2611 2624 +13
Lines 320018 321014 +996
============================================
+ Hits 279034 279892 +858
- Misses 40984 41122 +138
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (1)
503-516: ⚡ Quick winExpand stability assertions to all supported networks.
The stability contract is defined for any
(mnemonic, network)pair, but this test currently checks only Mainnet/Testnet. Add Devnet/Regtest assertions to fully cover the intended invariant.🤖 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/wallet_lifecycle.rs` around lines 503 - 516, The test same_mnemonic_same_network_is_stable currently only asserts stability for Network::Testnet and Network::Mainnet; extend it to include Network::Devnet and Network::Regtest by adding assertions that wallet_id_for(Network::Devnet) equals itself and wallet_id_for(Network::Regtest) equals itself so the stability invariant is covered for all supported Network variants; update the same_mnemonic_same_network_is_stable test to call wallet_id_for with Devnet and Regtest and assert equality just like the existing Testnet/Mainnet checks.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:
- Around line 503-516: The test same_mnemonic_same_network_is_stable currently
only asserts stability for Network::Testnet and Network::Mainnet; extend it to
include Network::Devnet and Network::Regtest by adding assertions that
wallet_id_for(Network::Devnet) equals itself and wallet_id_for(Network::Regtest)
equals itself so the stability invariant is covered for all supported Network
variants; update the same_mnemonic_same_network_is_stable test to call
wallet_id_for with Devnet and Regtest and assert equality just like the existing
Testnet/Mainnet checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9cbda7b-1d14-4f77-9863-f14af89324ff
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlpackages/rs-platform-wallet/src/manager/wallet_lifecycle.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
CodeRabbit nitpick on #3772: same_mnemonic_same_network_is_stable only asserted Mainnet/Testnet. Loop over all four Network variants (adds Devnet/Regtest) so the per-(seed, network) stability invariant — which the watch-only restore path depends on — is covered for every supported network. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The shift to network-scoped walletIds at this head structurally fixes the prior cross-network asset-lock/UTXO/balance leak — same-mnemonic wallets now get distinct walletIds per network, so every walletId-keyed table is network-correct by construction. However, the Swift side was not fully updated to that new id model: WalletDetailView.loadNetworkStates still assumes one walletId shared across networks (the inline comment even states this), so the network-state UI can only ever see the row it was opened from; and WalletDetailView.enableNetwork writes the mnemonic for the freshly created scoped walletId but never mirrors the per-wallet keychain metadata that orphan-recovery and bootstrap pre-warming rely on. The two carried-forward suggestions about classifying duplicates via Rust display-string substring ("already exists") remain valid in both CreateWalletView and WalletDetailView.
🔴 2 blocking | 🟡 2 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/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:631-646: loadNetworkStates can't discover sibling-network wallets after switch to network-scoped walletIds
The inline comment claims that a wallet now has one `PersistentWallet` row per network with the *same* `walletId` and distinct `networkRaw`, and the fetch keys on `PersistentWallet.predicate(walletId:)` (walletId-only predicate at PersistentWallet.swift:145-147). That assumption was invalidated by the same push: `register_wallet` in `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs:131-143` documents that the walletId is now network-scoped — the same mnemonic produces a DISTINCT id per network — which is the structural fix relied on elsewhere in this PR. As a result, this fetch returns at most the current network's row, so `mainnetEnabled` / `testnetEnabled` / `regtestEnabled` / `devnetEnabled` only ever reflect the network the row was opened on. Wallet Info therefore shows a wallet that exists on both testnet and mainnet as if it lived on only one, and `loadAccountCounts()` stamps the current row's account count onto every "enabled" network label. That directly breaks the PR's stated multi-network UX. The lookup needs to span sibling network-scoped ids (e.g. via shared mnemonic / metadata, or by recomputing the scoped id per network and probing each).
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:743-778: enableNetwork persists the mnemonic for the new scoped walletId but not its WalletKeychainMetadata
`enableNetwork` correctly creates a freshly scoped wallet on the target network and stores the mnemonic under the new `created.walletId` (lines 743-760), but it never writes a `WalletKeychainMetadata` blob for that new id. The create-wallet flow in `CreateWalletView` (lines 458-470) sets metadata per network — `networks: [created.network.networkName]`, name, birthHeight — and the rest of the app relies on it: `ContentView.recoverWallet` chooses the restore network via `entry.network ?? metadata?.resolvedNetworks.first ?? platformState.currentNetwork` (ContentView.swift:457-458), and the post-launch warmup is keyed off the same metadata. A wallet enabled via the `+` button therefore has a mnemonic in the keychain but no metadata, so after a wipe its orphan entry falls back to whichever network is currently active and can recreate the wallet on the wrong chain. Mirror the same `WalletKeychainMetadata(name:walletDescription:networks:birthHeight:)` write here using `created.walletId` and the target network, in the same `do/try? storage.setMetadata` shape used by CreateWalletView.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:761-778: Same substring-match fragility duplicated in enableNetwork
Carried forward from prior reviews and still valid. `enableNetwork`'s catch arm now uses the same `description.range(of: "already exists", options: .caseInsensitive)` heuristic to decide between a benign already-enabled state and a real failure. The fragility is identical to the create flow: a change to the Rust Display string makes legitimate already-present cases reach the user as "Failed to add <Network>: …", and an unrelated error containing those words gets swallowed. Same remediation — branch on a typed error case once the FFI exposes one — and ideally route both call sites through the shared helper.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift:365-386: Duplicate-wallet classification still relies on substring-matching a Rust display string
Carried forward from prior reviews (cee4b2cb, 815751a9) and still valid at 14d353eb. The per-network create loop classifies a benign duplicate by `error.localizedDescription.range(of: "already exists", options: .caseInsensitive)` rather than a typed error. That branch decides whether an already-present wallet is recorded as a no-op or surfaced via the `failures` array as a user-visible "Wallet could not be created". Any wording, capitalisation, localisation, or FFI-wrapper text change to `PlatformWalletError`'s Display impl breaks this classification silently. Expose a typed Rust variant (e.g. `PlatformWalletError::WalletAlreadyRegistered`) across the FFI so Swift can match by case instead of substring.
| let message = error.localizedDescription | ||
| // An "already exists" throw means the wallet | ||
| // is already on this network — benign. We do | ||
| // NOT resolve the existing scoped walletId to | ||
| // re-store the mnemonic: a wallet that already | ||
| // exists on this network had its mnemonic + | ||
| // metadata stored under that scoped id at its | ||
| // original creation, so there is nothing to | ||
| // write. It is also not counted as a freshly- | ||
| // created wallet. Any other error is a genuine | ||
| // failure. | ||
| if message.range(of: "already exists", options: .caseInsensitive) != nil { | ||
| SDKLogger.error( | ||
| "Wallet already present on \(net.displayName); continuing" | ||
| ) | ||
| } else { | ||
| failures.append((net, message)) | ||
| SDKLogger.error( | ||
| "Wallet creation failed for \(net.displayName): \(message)" | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Duplicate-wallet classification still relies on substring-matching a Rust display string
Carried forward from prior reviews (cee4b2c, 815751a) and still valid at 14d353e. The per-network create loop classifies a benign duplicate by error.localizedDescription.range(of: "already exists", options: .caseInsensitive) rather than a typed error. That branch decides whether an already-present wallet is recorded as a no-op or surfaced via the failures array as a user-visible "Wallet could not be created". Any wording, capitalisation, localisation, or FFI-wrapper text change to PlatformWalletError's Display impl breaks this classification silently. Expose a typed Rust variant (e.g. PlatformWalletError::WalletAlreadyRegistered) across the FFI so Swift can match by case instead of substring.
source: ['claude', 'codex']
There was a problem hiding this comment.
Acknowledged, deferring (same as the existing thread on this file). The robust fix is a typed PlatformWalletError.walletAlreadyExists to switch on, which is an rs-platform-wallet-ffi error-surface change beyond this Swift-side bugfix. Tracking as a follow-up; the substring match is the current best available signal.
| } catch { | ||
| let description = error.localizedDescription | ||
| // An "already exists" throw means the wallet is already on | ||
| // this network — a genuine no-op, so fall through to refresh. | ||
| // Any other failure (SDK build error, Rust-side error, etc.) | ||
| // must surface to the user instead of silently doing nothing. | ||
| if description.range(of: "already exists", options: .caseInsensitive) == nil { | ||
| SDKLogger.error( | ||
| "enableNetwork(\(network.displayName)) failed: \(description)" | ||
| ) | ||
| errorMessage = "Failed to add \(network.displayName): \(description)" | ||
| showError = true | ||
| return | ||
| } | ||
| SDKLogger.error( | ||
| "enableNetwork(\(network.displayName)) create returned: \(description)" | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Same substring-match fragility duplicated in enableNetwork
Carried forward from prior reviews and still valid. enableNetwork's catch arm now uses the same description.range(of: "already exists", options: .caseInsensitive) heuristic to decide between a benign already-enabled state and a real failure. The fragility is identical to the create flow: a change to the Rust Display string makes legitimate already-present cases reach the user as "Failed to add : …", and an unrelated error containing those words gets swallowed. Same remediation — branch on a typed error case once the FFI exposes one — and ideally route both call sites through the shared helper.
source: ['claude', 'codex']
There was a problem hiding this comment.
Acknowledged, deferring — same typed-error follow-up as the CreateWalletView thread. Once the FFI exposes a typed walletAlreadyExists case I'll route both this and CreateWalletView through it (ideally a shared helper). Substring match stays as the interim signal.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest push only expands the Rust scoped-walletId stability test to cover all four Network variants; no production code changed since 14d353e. Re-validated all four prior findings against head 5e76921 — every one remains valid against the current Swift sources. Both blocking findings (loadNetworkStates' walletId-only fetch and enableNetwork's missing keychain metadata write) directly break the PR's stated multi-network UX; the two substring-match suggestions are unchanged carry-forwards. CodeRabbit produced no actionable inline findings.
🔴 2 blocking | 🟡 2 suggestion(s)
No new inline comments were opened in this review round because all four current findings are carried-forward issues with existing open review threads. They are listed here so this exact head SHA still has an explicit cumulative review.
Prior finding reconciliation
- [blocking] loadNetworkStates cannot discover sibling-network wallets after the switch to network-scoped walletIds (carried forward from 14d353e)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:631-646
Status: STILL VALID on5e76921e. This is carried forward from the 14d353e review; the existing inline thread is still open, so I am not duplicating the inline comment here. - [blocking] enableNetwork persists the mnemonic for the new scoped walletId but not its WalletKeychainMetadata (carried forward from 14d353e)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:743-778
Status: STILL VALID on5e76921e. This is carried forward from the 14d353e review; the existing inline thread is still open, so I am not duplicating the inline comment here. - [suggestion] Duplicate-wallet classification still relies on substring-matching a Rust display string (carried forward)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift:365-386
Status: STILL VALID on5e76921e. This is carried forward from the 14d353e review; the existing inline thread is still open, so I am not duplicating the inline comment here. - [suggestion] Same substring-match fragility duplicated in enableNetwork (carried forward)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:761-778
Status: STILL VALID on5e76921e. This is carried forward from the 14d353e review; the existing inline thread is still open, so I am not duplicating the inline comment here.
CodeRabbit produced no actionable inline findings for this push; no CodeRabbit reactions were posted.
Two blocking regressions from the network-scoped-walletId change (thepastaclaw review on #3772): 1. WalletDetailView.loadNetworkStates discovered a wallet's sibling- network rows by matching `walletId`. Under scoped ids the same mnemonic has a DIFFERENT id per network, so the fetch only ever returned the row the view was opened on — the "Networks" section showed a multi-network wallet as living on one network. Fix: introduce `walletGroupId`, the network-INDEPENDENT id (the `None`-scoped digest = SHA256(rootPubKey || chainCode), i.e. what walletId was before scoping). It's computed in Rust at register_wallet and threaded through the wallet-metadata changeset → FFI callback → Swift persister → a new PersistentWallet.walletGroupId column. loadNetworkStates now groups sibling rows by it. Every network's wallet for one seed shares the same group id, while their scoped walletIds differ. 2. WalletDetailView.enableNetwork stored the mnemonic under the new scoped walletId but never wrote its WalletKeychainMetadata, so an orphan-recovered wallet could fall back to the active network and recreate on the wrong chain. Now writes the same metadata blob CreateWalletView does, keyed by the new id + target network. Layers touched: - changeset.rs: WalletMetadataEntry gains wallet_group_id: [u8; 32]. - wallet_lifecycle.rs: register_wallet computes it via compute_wallet_id_from_root_extended_pub_key(root, None) (falls back to the scoped id for keyless watch-only/external wallets). Added a unit test proving the group id is network-independent and differs from the scoped id. - rs-platform-wallet-ffi/persistence.rs: on_persist_wallet_metadata_fn gains a wallet_group_id pointer param. - PersistentWallet.swift: walletGroupId column + index + predicate. - PlatformWalletPersistenceHandler.swift: marshal + persist it. - WalletDetailView.swift: group by walletGroupId; write metadata in enableNetwork. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This review round reconciled against
A unit test proves the group id is identical across all four networks while the scoped walletId differs; The two 🟡 suggestions (substring |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified at head 7f14210. Prior blocking findings #1 (loadNetworkStates sibling discovery) and #2 (enableNetwork keychain metadata) are FIXED in this commit — loadNetworkStates now groups by walletGroupId with a legacy single-row fallback, and enableNetwork writes the WalletKeychainMetadata blob under the new scoped walletId alongside the mnemonic. Prior suggestion findings #3 and #4 (substring-matching the Rust 'already exists' display string in CreateWalletView and WalletDetailView.enableNetwork) remain STILL VALID at this head; both reviewers (Claude and Codex) independently re-flag them and confirmed the FFI layer still collapses WalletAlreadyExists into a generic display-string error. No new latest-delta findings.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:792-809: enableNetwork repeats the same substring-match heuristic on the FFI error description
`enableNetwork`'s catch arm uses an identical `description.range(of: "already exists", options: .caseInsensitive) == nil` test on `error.localizedDescription` to distinguish a benign already-on-this-network result from a real failure. The fragility is the same as `CreateWalletView`'s site — Rust's typed `WalletAlreadyExists` is flattened into a localized free-form string before Swift inspects it, and this code path decides whether to fall through to a UI refresh (implying the network was enabled) or surface an alert. A phrasing or localization drift on the Rust side will either hide real failures or spuriously alarm the user. Should be fixed in lockstep with the CreateWalletView site by introducing a structured `walletAlreadyExists` case on the wallet-FFI error surface and matching on that case from both Swift call sites. Carried forward from 14d353eb / 5e76921e — STILL VALID at 7f142107.
| } catch { | ||
| let description = error.localizedDescription | ||
| // An "already exists" throw means the wallet is already on | ||
| // this network — a genuine no-op, so fall through to refresh. | ||
| // Any other failure (SDK build error, Rust-side error, etc.) | ||
| // must surface to the user instead of silently doing nothing. | ||
| if description.range(of: "already exists", options: .caseInsensitive) == nil { | ||
| SDKLogger.error( | ||
| "enableNetwork(\(network.displayName)) failed: \(description)" | ||
| ) | ||
| errorMessage = "Failed to add \(network.displayName): \(description)" | ||
| showError = true | ||
| return | ||
| } | ||
| SDKLogger.error( | ||
| "enableNetwork(\(network.displayName)) create returned: \(description)" | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: enableNetwork repeats the same substring-match heuristic on the FFI error description
enableNetwork's catch arm uses an identical description.range(of: "already exists", options: .caseInsensitive) == nil test on error.localizedDescription to distinguish a benign already-on-this-network result from a real failure. The fragility is the same as CreateWalletView's site — Rust's typed WalletAlreadyExists is flattened into a localized free-form string before Swift inspects it, and this code path decides whether to fall through to a UI refresh (implying the network was enabled) or surface an alert. A phrasing or localization drift on the Rust side will either hide real failures or spuriously alarm the user. Should be fixed in lockstep with the CreateWalletView site by introducing a structured walletAlreadyExists case on the wallet-FFI error surface and matching on that case from both Swift call sites. Carried forward from 14d353e / 5e76921 — STILL VALID at 7f14210.
source: ['claude', 'codex']
There was a problem hiding this comment.
Acknowledged, deferring — same typed-error follow-up as the CreateWalletView site (threads 3331111688 / 3334667296). The robust fix is a structured PlatformWalletError.walletAlreadyExists case exposed across rs-platform-wallet-ffi, matched from both Swift call sites (ideally via a shared helper). That's an FFI error-surface change out of scope for this Swift-side bugfix PR; tracking as a follow-up. Leaving open (not resolving) so it stays visible until that work lands.
With network-scoped walletIds the network is folded into the id digest, so `walletId` is globally unique on its own — a given id maps to exactly one (seed, network) pair. The `(walletId, networkRaw)` composite was a leftover from the pre-scoping model, where one seed shared a single id across networks and `networkRaw` was the only distinguishing column. Drop it to `#Unique([\.walletId])` so the constraint states the actual invariant. Pre-release: dev stores rebuild, no migration. Comment updated to explain the history. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta (92b6b28) tightens PersistentWallet's SwiftData uniqueness from the composite (walletId, networkRaw) to walletId alone, with refreshed doc comments. Given the upstream change that folds a domain-tagged network byte into the walletId digest, this is consistent and correct — no new defects in the latest delta itself. Both prior findings from 7f14210 (substring-matching the Rust FFI error display string "already exists" to drive Swift control flow in CreateWalletView and WalletDetailView) are carried forward STILL VALID — the flagged lines are unchanged at this head. One additional low-confidence nitpick on a now-over-specified predicate helper is included.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift:792-809: Carried forward — enableNetwork repeats the same FFI-display-string heuristic
Re-verified at 92b6b28; lines unchanged from the 7f142107 review. `enableNetwork(_:)`'s catch arm uses `description.range(of: "already exists", options: .caseInsensitive) == nil` against `error.localizedDescription` to distinguish a benign already-on-this-network outcome (fall through to `loadNetworkStates()` / `loadAccountCounts()`) from a real failure (surface to the user). The fragility is symmetric to the `CreateWalletView` site, but the asymmetric failure mode here is more dangerous: a real Rust-side failure whose display text happens to contain "already exists" (the `IdentityAlreadyExists` / DashPay variants in `rs-platform-wallet/src/error.rs`, or any wrapped error that mentions "already exists" in context) is silently treated as a successful add-to-network — the function falls through, the user sees the network as added, but no per-network wallet row, keychain mnemonic, or metadata was actually written. Same root cause and same remediation as the CreateWalletView finding: propagate a typed `WalletAlreadyExists` discriminator across the FFI and switch on the type rather than the display string. Tracking both sites together is reasonable.
| } catch { | ||
| let description = error.localizedDescription | ||
| // An "already exists" throw means the wallet is already on | ||
| // this network — a genuine no-op, so fall through to refresh. | ||
| // Any other failure (SDK build error, Rust-side error, etc.) | ||
| // must surface to the user instead of silently doing nothing. | ||
| if description.range(of: "already exists", options: .caseInsensitive) == nil { | ||
| SDKLogger.error( | ||
| "enableNetwork(\(network.displayName)) failed: \(description)" | ||
| ) | ||
| errorMessage = "Failed to add \(network.displayName): \(description)" | ||
| showError = true | ||
| return | ||
| } | ||
| SDKLogger.error( | ||
| "enableNetwork(\(network.displayName)) create returned: \(description)" | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Carried forward — enableNetwork repeats the same FFI-display-string heuristic
Re-verified at 92b6b28; lines unchanged from the 7f14210 review. enableNetwork(_:)'s catch arm uses description.range(of: "already exists", options: .caseInsensitive) == nil against error.localizedDescription to distinguish a benign already-on-this-network outcome (fall through to loadNetworkStates() / loadAccountCounts()) from a real failure (surface to the user). The fragility is symmetric to the CreateWalletView site, but the asymmetric failure mode here is more dangerous: a real Rust-side failure whose display text happens to contain "already exists" (the IdentityAlreadyExists / DashPay variants in rs-platform-wallet/src/error.rs, or any wrapped error that mentions "already exists" in context) is silently treated as a successful add-to-network — the function falls through, the user sees the network as added, but no per-network wallet row, keychain mnemonic, or metadata was actually written. Same root cause and same remediation as the CreateWalletView finding: propagate a typed WalletAlreadyExists discriminator across the FFI and switch on the type rather than the display string. Tracking both sites together is reasonable.
source: ['claude']
| public static func predicate( | ||
| walletId: Data, | ||
| network: Network | ||
| ) -> Predicate<PersistentWallet> { | ||
| let networkRaw = network.rawValue | ||
| return #Predicate<PersistentWallet> { | ||
| $0.walletId == walletId && $0.networkRaw == networkRaw | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: predicate(walletId:network:) is over-specified now that walletId is globally unique
With the latest delta's invariant that walletId is network-scoped and globally unique on its own (the network byte is folded into the digest, and the #Unique constraint on the model is now [\.walletId]), the composite predicate $0.walletId == walletId && $0.networkRaw == networkRaw can never narrow the result set further than the walletId clause alone, but it CAN spuriously return empty for rows whose networkRaw is nil — the property's own doc-comment (lines 47–49) explicitly permits pre-persistWalletMetadata rows to have networkRaw == nil. This is a low-impact mismatch with the model rather than a correctness break, and is worth flagging only because the just-rewritten walletId doc-comment advertises that the network is already baked into walletId — keeping a sibling lookup helper that still requires a networkRaw match invites confusion at future call sites. Consider either dropping the network clause from this predicate or removing the helper in favor of predicate(walletId:).
source: ['claude']
2 participants
Issue being fixed or feature implemented
A set of wallet-flow fixes in the iOS SwiftExampleApp / swift-sdk for operating across multiple networks. The same mnemonic can now live on several networks, and the create / rescan / add-to-network flows behave correctly when the target network isn't the currently-active one.
Bugs fixed:
platformQuorumURLoverride was applied to every network, leaking a non-httpshttp://…quorum URL into mainnet/testnet SDK construction. Rust's trusted context provider rejects a non-https custom quorum URL on those networks, so the build failed and no wallet was created.CreateWalletView, but the pushedSeedBackupViewstayed on top with its submit button stuck disabled — the user was stranded with no visible error.@Query, listing every wallet across every network instead of just the active-network ones.What was done?
Per-network persistence infrastructure
PersistentWallet.swift— make rows unique per(walletId, network)and addpredicate(walletId:)/predicate(walletId:network:)helpers, so the same mnemonic can coexist across networks without colliding.PlatformWalletPersistenceHandler.swift/PlatformWalletManager.swift— scope persister wallet lookups to the manager's network, so each per-network manager restores only its own rows.Create flow
CreateWalletView.swift— create the wallet in every ticked network viaWalletManagerStore.backgroundManager(for:)(each network has its own network-locked Rust manager);walletIdis network-independent, so the Keychain mnemonic + metadata are written once andisImportedis stamped on every per-network row. On failure, pop the pushed backup screen so the error alert becomes visible.SDK.swift— gate theplatformQuorumURL(and DAPI address) override behinduseOverrideAddresses(regtest/devnet or opt-in docker). Mainnet/testnet now use the SDK's automatic canonical quorum endpoints.Add-to-network / identities / rescan
WalletDetailView.swift— implement the per-network "+" add-to-network action.IdentitiesContentView.swift— take an explicitnetworkand scope the identities@Queryto it;IdentitiesView.swiftpasses the active network;ContentView.swiftthreads the active network through.SearchWalletsForIdentitiesView.swift— addplatformState, query all wallets, and filter to the active network via a computed property (mirrorsWalletsContentView—@Querycan't reference a runtime env-object value at property-init time). The rescan sheet re-injectsplatformState.All changes are Swift-side only (persist / load / bridge) — no Rust/FFI changes, no protocol changes.
How Has This Been Tested?
Verified on the booted iOS simulator (iPhone 17 Pro) against the live SwiftData store:
networkRaw=1row and no devnet fallback row.currentNetwork = mainnetand 9 persisted wallet rows across 4 networks (mainnet ×1, testnet ×3, devnet ×2, regtest ×3), the Re-scan for Identities picker now lists only the single mainnet wallet — count dropped 9 → 1, matching the active-network set exactly.no DAPI addresses provided, using defaults for network/Mainnet(no leaked devnethttp://…:8080quorum URL), while the devnet manager still uses its caller-provided quorum URL.Breaking Changes
None. Swift SDK / example-app behavior only; no consensus or protocol changes.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes