fix(sdk): wallet-flow network fixes for SwiftExampleApp#3772
fix(sdk): wallet-flow network fixes for SwiftExampleApp#3772QuantumExplorer wants to merge 17 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 21391d5) |
|
✅ 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: "5c094930ea8b2f61fde3c8a3e5a6e5e763c3816379170cdefb74f4b0942620f4"
)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
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.
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.
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 69.91% 87.17% +17.25%
=============================================
Files 19 2627 +2608
Lines 2712 321631 +318919
=============================================
+ Hits 1896 280366 +278470
- Misses 816 41265 +40449
🚀 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.
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.
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.
…flow-network-fixes # Conflicts: # Cargo.lock
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified the three carried-forward prior findings at head 795284a against the current source. All three are STILL VALID and unchanged from 92b6b28 — CreateWalletView.swift:365-386 and WalletDetailView.swift:792-809 still branch on error.localizedDescription substring-matching the FFI display string "already exists", and PersistentWallet.swift:177-185 still defines the redundant predicate(walletId:network:) overload now that walletId carries the network byte and is declared #Unique. The latest delta is dominated by an unrelated merge from origin/v3.1-dev and introduces no new in-scope defects. No new latest-delta findings; only the three carried-forward prior findings remain.
🟡 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/CreateWalletView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CreateWalletView.swift:365-386: Carried forward: per-network create loop classifies duplicate wallets by FFI display text
Re-verified at 795284ac — lines unchanged from the prior 92b6b280 review and from 7f142107 before that. The per-network create loop decides whether a `mgr.createWallet(...)` failure is the benign "wallet already exists on this network" case by `message.range(of: "already exists", options: .caseInsensitive)` against `error.localizedDescription`. `localizedDescription` reflects the human-facing Rust/FFI error wording, not a typed result code, so any rewording, localization change, prefix wrap at the FFI boundary, or unrelated error that happens to contain the same phrase (e.g. "identity already exists", "keychain entry already exists") silently flips this branch. The user-facing classification — a multi-network create being reported as success vs. failure, and whether the mnemonic-storage step runs — depends on Rust `Display` output. `rs-platform-wallet-ffi` does have a `WalletAlreadyExists` concept on the Rust side; expose it as a distinguishable FFI result code / typed Swift error variant and key the branch on that instead of substring matching.
With network-scoped walletIds, `walletId` is globally unique and the model is declared `#Unique([\.walletId])`, so the `networkRaw` clause in `predicate(walletId:network:)` can never narrow the result beyond the walletId match. The overload had no remaining callers (sibling-network grouping goes through `predicate(walletGroupId:)`), so keeping it only preserved the obsolete pre-scoping mental model that `(walletId, networkRaw)` is the disambiguating key. Remove it. 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
Cumulative verifier review at head 4bdc604. Prior nitpick #3 (dead predicate(walletId:network:) overload) is FIXED — confirmed gone from the codebase. Prior findings #1 and #2 (FFI display-text duplicate-wallet heuristics in CreateWalletView and WalletDetailView) remain STILL VALID at the current head and are carried forward. One new in-scope cumulative finding from Codex: the new add-to-network path in WalletDetailView stamps the keychain metadata for the freshly-created target-network wallet with the source wallet's birthHeight, which ContentView.recoverWallet will later trust verbatim on recovery. Codex's MIN_WITHDRAWAL_AMOUNT sync-guard finding is dropped: that file's change arrived via the merge of v3.1-dev (PR #3800) and is not part of this PR's authored diff.
🟡 1 suggestion(s)
2 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:779-786: Add-to-network stores the source wallet's birthHeight against the target-network wallet id
After `mgr.createWallet` succeeds on the target `network`, the keychain metadata for `created.walletId` (the target network's scoped id) is written with `birthHeight: wallet.birthHeight`, where `wallet` is the source-network row that this detail screen was opened from. Birth heights are chain-block numbers; the source network's tip is unrelated to the target's. After a SwiftData wipe, `ContentView.recoverWallet` reads this metadata and unconditionally overrides the recovered row's birth height with the stored value (ContentView.swift:523-525), so the target-network wallet rescans from a wrong height. In the worst case the source height exceeds the target chain tip and the recovered wallet skips its entire history. Use the target-row's birth height once it lands (e.g. fetch the just-created `PersistentWallet` row for `created.walletId` and read its `birthHeight`), or omit the override and let the persister-stamped value stand.
enableNetwork wrote the add-to-network keychain metadata with `birthHeight: wallet.birthHeight`, but `wallet` is the SOURCE-network row the detail screen was opened on — birth heights are chain-block numbers, so the source network's tip is unrelated to the target's. On orphan recovery, ContentView.recoverWallet trusts the stored value verbatim, so the target-network wallet would rescan from a wrong height (and if the source height exceeds the target tip, skip its entire history). Read the birth height from the freshly-created target-network row (`created.walletId`), the same way CreateWalletView sources it — the persister stamps the correct value on that row synchronously during createWallet. 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
Latest delta (dc55e6d) correctly resolves prior finding #3 by reading birthHeight from the freshly-created target-network row instead of the source wallet. Prior findings #1 and #2 remain unfixed at this head: both CreateWalletView and WalletDetailView.enableNetwork still classify a Rust-side mgr.createWallet failure as a benign duplicate by substring-matching error.localizedDescription for "already exists", which couples Swift control flow to FFI display text. One new cumulative concern is carried in from Codex: the Identities tab's resumable asset-lock list still runs over unfiltered allAssetLocks even though the surrounding view was scoped to the active network in this PR.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
2 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/IdentitiesContentView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift:489-491: Resumable asset locks list is not filtered to the active network
This PR makes `IdentitiesContentView` network-scoped by filtering `identities`, but `crossWalletResumableLocks` is still invoked over the unfiltered `allAssetLocks`. The resume sheet is launched from the active Identities tab and `CreateIdentityView` resolves the wallet through the active `walletManager`, so an asset lock that belongs to a wallet on a different network can still be displayed in the resume list and then fail with "selected wallet isn't loaded" (or surface a resume action in the wrong network context) when the user taps it. Either filter `allAssetLocks` to wallet ids whose `PersistentWallet.networkRaw` matches the active network before passing them to the anti-join, or thread the active network into `ResumableRegistrationsList` and apply the same wallet/network filter there.
The Resumable Registrations list ran its identity/in-flight anti-join over every tracked PersistentAssetLock regardless of network, so locks belonging to wallets on another network surfaced as resumable rows after a network switch. Thread the active Network into ResumableRegistrationsList and restrict allAssetLocks to wallets on that network before the anti-join, joining through walletId to PersistentWallet.networkRaw (PersistentAssetLock has no networkRaw column of its own) — the same network-scoping pivot CoreContentView already uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Fixed in 4cf30a4 (carried-forward finding: resumable asset-lock list not network-scoped). The Resumable Registrations list now scopes `allAssetLocks` to wallets on the active network before the anti-join, instead of running `crossWalletResumableLocks` over every tracked lock. `IdentitiesContentView` threads the active `Network` into `ResumableRegistrationsList`, which derives the set of `walletId`s on that network from `allWallets` (filtering `PersistentWallet.networkRaw`) and filters `allAssetLocks` against it. This uses the same `walletId → PersistentWallet.networkRaw` pivot `CoreContentView` already uses, since `PersistentAssetLock` carries no `networkRaw` column of its own. Closes the gap where a lock from another network surfaced as a resumable row after a network switch. Verified compiling against a freshly-rebuilt simulator framework — |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review pinned to 4cf30a4. Prior findings #1 and #2 (FFI display-text classification in CreateWalletView and WalletDetailView) are STILL VALID and carried forward. Prior finding #3 (resumable asset locks not network-scoped) is FIXED by the new networkScopedAssetLocks filter in IdentitiesContentView. Two new latest-delta concerns from this push: enableNetwork pulls the mnemonic across the FFI boundary in a view (contradicts the explicit Swift SDK rule), and restorableWalletIds() enumerates wallets across all networks despite the manager being network-scoped. No blocking issues — all findings are suggestion-level architectural/logic concerns.
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
2 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:744-759: enableNetwork pulls mnemonic across FFI in a view to re-feed createWallet
`enableNetwork(_:)` calls `WalletStorage().retrieveMnemonic(for: wallet.walletId)` in a SwiftUI view and hands the cleartext mnemonic back to `mgr.createWallet(mnemonic:network:name:)`. `packages/swift-sdk/CLAUDE.md` explicitly forbids this shape: "No pulling mnemonics or seeds across the FFI boundary so Swift can 'finish' an operation Rust already knows how to complete" — and lists exactly this pattern (retrieveMnemonic in a view followed by an FFI call that re-derives) as the anti-precedent. The correct shape is a single FFI entry point (e.g. `platform_wallet_enable_on_network(wallet_id, target_network)`) that resolves seed material entirely inside `platform-wallet` and registers the per-network sibling wallet, with Swift only persisting the resulting per-network Keychain entries under the new scoped id. This also widens the window where the cleartext mnemonic lives in a Swift `String`. Not a runtime bug, but it cuts against an explicit project rule.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3944-3955: restorableWalletIds() enumerates all networks despite the handler being network-scoped
`loadWalletList()` was correctly scoped to `self.network` in this PR, but `restorableWalletIds()` still fetches every `PersistentWallet` row regardless of `networkRaw`. `PlatformWalletManager.loadFromPersistor()` calls this immediately after Rust restores only the active-network rows, so a per-network manager will ask Rust for handles to sibling-network wallet ids that were intentionally not loaded, record those get-wallet failures in `lastError`, and do unnecessary cross-network work. The successful active-network restore still completes, but the post-restore state is left with misleading error noise that undermines the PR's network-scoped manager invariant. Filter the fetch by `networkRaw == self.network.rawValue` to match `loadWalletList()`'s scoping.
CreateWalletView's multi-network create loop and WalletDetailView's enableNetwork both classified a duplicate-wallet failure as benign by substring-matching error.localizedDescription for "already exists", coupling Swift control flow to the Rust Display text. PlatformWalletError::WalletAlreadyExists already existed but the FFI flattened it to ErrorUnknown, so only the string survived. Add a dedicated PlatformWalletFFIResultCode::ErrorWalletAlreadyExists (= 15), map the variant to it (Display still carried as the message), mirror it in Swift as PlatformWalletError.walletAlreadyExists, and branch on the typed case at both sites instead of matching display text. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta cleanly resolves prior findings 1 and 2 by introducing typed FFI code 15 (ErrorWalletAlreadyExists) and typed Swift pattern matching, but introduces a new blocking regression: register_wallet flattens key_wallet_manager::WalletError::WalletExists into PlatformWalletError::WalletCreation, so the FFI never emits code 15 for the create path. Both Swift catch arms now miss the duplicate case entirely, breaking the PR's intended benign no-op for re-creating a wallet that already exists on a target network. Prior findings 3 (mnemonic across FFI in enableNetwork) and 4 (restorableWalletIds ignores network) are STILL VALID and carried forward.
🔴 1 blocking | 🟡 1 suggestion(s)
2 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/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs:245-253: Duplicate-wallet errors are flattened into WalletCreation, so the new typed code never fires
This is the latest-delta regression introduced by the PR. `wm.insert_wallet` returns `key_wallet_manager::WalletError::WalletExists(WalletId)` for duplicates (verified at `key-wallet-manager/src/accessors.rs:66`), but `register_wallet` blanket-wraps every `insert_wallet` failure into `PlatformWalletError::WalletCreation(...)`. The new FFI mapping (`error.rs:191`) only emits `ErrorWalletAlreadyExists = 15` for `PlatformWalletError::WalletAlreadyExists`, so re-creating an existing wallet still returns `ErrorUnknown` with only the display string carrying "already exists". Both Swift catch arms — `CreateWalletView.swift:375` and `WalletDetailView.swift:812` — now decide the benign duplicate case via `if case PlatformWalletError.walletAlreadyExists = error`, which will never match. The previous substring fallback was removed in this push, so the PR's intended behavior (treat "already on this network" as a no-op) is broken end-to-end: per-network create surfaces a real failure to the user, and `enableNetwork` shows an error alert and returns early. Unit-testing the FFI mapper in isolation does not catch this because no producer in `rs-platform-wallet` ever constructs `WalletAlreadyExists`.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3944-3956: Carried forward: restorableWalletIds() ignores self.network despite the handler being network-scoped
Carried forward from the prior review of 4cf30a44 — STILL VALID at 1c0fb979. `loadWalletList()` at line 2944 was correctly tightened to filter `PersistentWallet` by `self.network`, but `restorableWalletIds()` still issues an unfiltered `FetchDescriptor<PersistentWallet>()`. `PlatformWalletManager.loadFromPersistor()` calls this immediately after Rust restores only the active-network rows, then iterates every returned id and calls `platform_wallet_manager_get_wallet`. Sibling-network ids that Rust never restored fail that lookup; the catch arm writes each failure to `self.lastError`, surfacing a misleading "last error" to the UI and producing unnecessary cross-network FFI work right after a network-scoped restore. Mirror the predicate from `loadWalletList()`.
…bleWalletIds The prior commit added a typed ErrorWalletAlreadyExists FFI code and made the Swift call sites branch on PlatformWalletError.walletAlreadyExists, but register_wallet blanket-wrapped every WalletManager::insert_wallet failure into PlatformWalletError::WalletCreation. The duplicate variant (key_wallet_manager::WalletError::WalletExists) was therefore never mapped to WalletAlreadyExists, the FFI never emitted code 15, and the Swift typed checks never matched — breaking the benign 'already on this network' no-op end-to-end. Map WalletExists to WalletAlreadyExists explicitly. Add a producer-level test (register the same wallet twice) so the create path is covered, not just the FFI mapper in isolation. Also scope restorableWalletIds() to the handler's bound network to mirror loadWalletList(); the unfiltered fetch made a per-network manager request sibling-network wallet handles right after a network-scoped restore, polluting lastError with spurious get-wallet failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed in 21391d5. 🔴 Blocking — duplicate-wallet errors flattened into wm.insert_wallet(wallet, platform_info).map_err(|e| {
if matches!(e, key_wallet_manager::WalletError::WalletExists(_)) {
PlatformWalletError::WalletAlreadyExists(e.to_string())
} else {
PlatformWalletError::WalletCreation(format!(
"Failed to register wallet in WalletManager: {}", e
))
}
})?Added a producer-level test ( 🟡 Verified: 🟡 enableNetwork pulls mnemonic across FFI: Acknowledged and deferred as a separate cross-language refactor — tracked in #3808 along with the identity-key derivation site. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Reconciled the three prior PastaClaw findings against head 21391d5. Prior #1 (duplicate-wallet error mapping) and Prior #3 (restorableWalletIds network scoping) are FIXED in this push and recorded in resolved_findings. Prior #2 (enableNetwork pulls the cleartext mnemonic across the FFI to re-feed createWallet) is STILL VALID and is carried forward as the only finding — the function is actively being modified by this PR (commit dc55e6d touches enableNetwork metadata), so it is in-scope despite being acknowledged by the author as deferred. No new latest-delta defects identified: Codex's new blocking claim that matches!(e, WalletError::WalletExists(_)) moves e is a false positive (the _ wildcard inside the variant pattern does not move the scrutinee, so e.to_string() and format!("{}", e) afterward compile cleanly).
🟡 1 suggestion(s)
Note: The inline finding is carried forward from an existing open thread on this PR, so I am not re-posting a duplicate inline comment; this top-level review records the current-head revalidation for 21391d5.
Prior finding reconciliation
- FIXED — Duplicate-wallet errors are flattened into WalletCreation, so the new typed code never fires: FIXED in this push.
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs:252-261now classifieswm.insert_wallet(...)failures:key_wallet_manager::WalletError::WalletExists(_)is re-raised asPlatformWalletError::WalletAlreadyExists(e.to_string()), all other errors continue to flow throughPlatformWalletError::WalletCreation(...). The FFI mapper's typed code 15 (ErrorWalletAlreadyExists) is now reachable end-to-end throughregister_wallet(not just via the FFI mapper in isolation), and prior reviews note a covering tokio test was added. - FIXED — restorableWalletIds() ignores self.network despite the handler being network-scoped: FIXED in this push.
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3952-3962now builds aFetchDescriptor<PersistentWallet>with#Predicate { $0.networkRaw == raw }whenself.networkis non-nil, mirroring the predicate used byloadWalletList(). The unfiltered cross-network fetch remains only for legacy nil-network callers, soPlatformWalletManager.loadFromPersistor()no longer requests handles for sibling-network ids that Rust never restored on the active network.
Carried-forward prior finding detail (1)
suggestion: Carried forward: enableNetwork pulls the cleartext mnemonic across the FFI to re-feed createWallet
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift (line 744)
Verified at head 21391d5. enableNetwork(_:) calls WalletStorage().retrieveMnemonic(for: wallet.walletId) in a SwiftUI view, lifts the cleartext BIP-39 mnemonic into a Swift String, and hands it back to mgr.createWallet(mnemonic:network:name:) so Rust can finish the per-network registration. packages/swift-sdk/CLAUDE.md is explicit about this pattern: "No pulling mnemonics or seeds across the FFI boundary so Swift can 'finish' an operation Rust already knows how to complete", and names WalletStorage().retrieveMnemonic(for:) in a Swift view as the canonical anti-precedent. Two consequences worth keeping in mind: (1) Swift String is not a zeroizing buffer, so the seed phrase stays in heap memory until ARC frees the page — every cross-boundary copy widens the memory-disclosure surface for the wallet's highest-value secret; (2) the per-network keychain mirror write is best-effort, so a partial failure (source row later wiped, target row missing/garbled) is harder to reason about than a single atomic FFI entry point. The new typed WalletAlreadyExists (code 15) landed in this push lets the Swift catch block treat an already-enabled-on-this-network repeat as benign, but does not address the underlying boundary violation. Preferred direction: a single platform_wallet_manager_enable_network_for_wallet(walletId, targetNetwork) (or equivalent) FFI entry point that resolves seed material inside platform-wallet, registers the sibling-network wallet, and returns the new scoped wallet id + birth height for Swift to persist — so the mnemonic never leaves Rust. The author appears to have deferred this to a follow-up PR ("no Rust/FFI changes" in scope), which is reasonable — please track as a follow-up rather than dropping.
Dropped/false-positive candidate findings
- Duplicate-wallet error mapping moves
ebefore using it — False positive.matches!(e, key_wallet_manager::WalletError::WalletExists(_))expands tomatch e { WalletError::WalletExists(_) => true, _ => false }. The variant pattern only contains a_wildcard with no by-value bindings, so the match does NOT movee— per Rust's pattern semantics, a pattern that binds nothing by value does not consume the scrutinee.e.to_string()andformat!("{}", e)after thematches!call compile cleanly, which is consistent with the prior reviews noting the new tokio test exercises this path successfully.
3 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