feat(swift-example-app): bind every wallet's shielded sub-wallet for multi-wallet flows#4038
Conversation
QuantumExplorer
requested review from
ZocoLini,
llbartekll and
shumkov
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThis PR moves shielded balance and address resolution in the Swift example app from a single-wallet ChangesMulti-wallet shielded binding
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit f946509) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift (1)
227-291: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEngine-bind loop is not isolated from earlier throwing steps.
engineBindOtherWalletsis documented and tested (ShieldedEngineBindPlanTests) to be independent/best-effort per non-mirror wallet, but it only runs if every priortryin thisdoblock succeeds (platform-address sync configure/start, shielded bind/start). If any of those throw, the whole function bails into the outercatchand no other-wallet engine binding happens this pass — even though those failures are logically unrelated to shielded engine binding. This weakens the "one wallet's failure can't block the rest" guarantee at the function level and can delay the SH-14/15/16 cross-wallet shielded flows until the next unrelated rebind trigger.Consider isolating the engine-bind loop (e.g., wrap the preceding throwing calls in their own local
do/catchwithSDKLogger.error, or move the loop earlier) so an unrelated sync-start failure can't suppress it.🔧 Example restructuring
- do { - let platformAddressWallet = try wallet.platformAddressWallet() - platformBalanceSyncService.configure(...) - if try !walletManager.isPlatformAddressSyncRunning() { - try walletManager.startPlatformAddressSync() - } - ... - if try !walletManager.isDashPaySyncRunning() { - try walletManager.startDashPaySync() - } - - engineBindOtherWallets(...) { ... } - } catch { - SDKLogger.error("Failed to bind wallet-scoped services: \(error.localizedDescription)") - } + do { + let platformAddressWallet = try wallet.platformAddressWallet() + platformBalanceSyncService.configure(...) + if try !walletManager.isPlatformAddressSyncRunning() { + try walletManager.startPlatformAddressSync() + } + ... + } catch { + SDKLogger.error("Failed to bind wallet-scoped services: \(error.localizedDescription)") + } + + // Runs regardless of the outcome above — independent of unrelated sync-start failures. + engineBindOtherWallets(...) { otherWalletId in + shieldedService.bindEngine(...) + } + + do { + if try !walletManager.isDashPaySyncRunning() { + try walletManager.startDashPaySync() + } + } catch { + SDKLogger.error("Failed to start DashPay sync: \(error.localizedDescription)") + }🤖 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/SwiftExampleAppApp.swift` around lines 227 - 291, The engine-bind loop in SwiftExampleAppApp’s startup flow is currently gated by earlier throwing sync setup calls, so unrelated failures can prevent engine binding for the remaining wallets. Isolate engineBindOtherWallets from the platformAddressWallet/platformBalanceSyncService and shieldedService start path by wrapping those earlier throws in their own local do/catch or moving the loop outside that failure boundary, while keeping the best-effort behavior in shieldedService.bindEngine and the non-mirror wallet iteration intact.
🧹 Nitpick comments (3)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (1)
253-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated bind sequence between
bind()andbindEngine().The
dbPathcomputation, account dedup/sort, andconfigureShielded/bindShieldedcalls are now duplicated verbatim betweenbind()(lines 253-268) andbindEngine()(lines 397-405). A future change to this sequence (e.g., additional validation, retry, or ordering) risks being applied to only one path.♻️ Proposed extraction of the shared bind sequence
+ private func performShieldedFFIBind( + walletManager: PlatformWalletManager, + walletId: Data, + network: Network, + resolver: MnemonicResolver, + accounts: [UInt32] + ) throws -> [UInt32] { + let dbPath = Self.dbPath(for: network) + let sortedAccounts = Array(Set(accounts)).sorted() + try walletManager.configureShielded(dbPath: dbPath) + try walletManager.bindShielded( + walletId: walletId, + resolver: resolver, + accounts: sortedAccounts + ) + return sortedAccounts + }
bind()andbindEngine()would then call this helper inside their owndo/catchblocks and apply their respective published-state / logging side effects.Also applies to: 390-416
🤖 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/Services/ShieldedService.swift` around lines 253 - 268, The shielded bind flow is duplicated between bind() and bindEngine(), including dbPath resolution, account dedup/sort, and the configureShielded/bindShielded sequence. Extract that shared logic into a single helper method on ShieldedService (or a nearby private helper) and have both bind() and bindEngine() call it inside their existing do/catch blocks, keeping their own logging and published-state updates separate.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift (1)
67-94: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider snapshotting the shielded lookup once per render.
shieldedAccountsForThisWallet(an FFI call) is evaluated at Line 103, then again at Lines 132/134, andshieldedAddress(for:)is called per row. This mirrors the exact patternSendTransactionView.coreBalanceSnapshot()was introduced to avoid ("hitting the FFI repeatedly on a typing-heavy form"). Not a correctness issue, but worth caching the bound-flag/address once inbodyfor consistency with that precedent.Also applies to: 103-103, 132-138
🤖 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/AccountListView.swift` around lines 67 - 94, The shielded wallet lookup in AccountListView is being repeated multiple times during a single render, which should be avoided for consistency with the snapshot pattern used in SendTransactionView.coreBalanceSnapshot(). Cache the result of shieldedAccountsForThisWallet and the per-account shieldedAddress(for:) lookup once in body, then reuse those cached values for the section check and row rendering instead of calling walletManager.shieldedDefaultAddress repeatedly.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ReceiveAddressView.swift (1)
101-116: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMemoize
shieldedReceiveAddressper render.The switch to an FFI-backed
shieldedDefaultAddresscall (replacing the previously cached mirror value) means every access ofcurrentAddress/hasValidAddresson the.shieldedtab now re-triggers the FFI +DashAddress.encodeOrchardwork.currentAddressalone is read at least 4 times per render (QR generation, address label, faucet flows). Consider resolving once into a localletat the top ofbody(or a@Statecache keyed onselectedTab), matching the codebase's owncoreBalanceSnapshot()precedent for FFI-heavy computed properties.Also applies to: 117-128, 156-165
🤖 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/ReceiveAddressView.swift` around lines 101 - 116, The shielded receive address is being recomputed too often because `shieldedReceiveAddress` is a computed property that re-runs the FFI-backed `walletManager.shieldedDefaultAddress` and `DashAddress.encodeOrchard` on every `currentAddress`/`hasValidAddress` access. Update `ReceiveAddressView.body` to resolve the shielded address once per render into a local `let` (or a `@State` cache tied to `selectedTab`) and have the `.shielded` paths reuse that value, following the same pattern used by `coreBalanceSnapshot()` for expensive FFI calls.
🤖 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/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift`:
- Around line 1743-1777: The shielded-note balance in CreateIdentityView is
computed with a one-off modelContext.fetch in shieldedPoolBalance(for:), so the
UI won’t refresh when new PersistentShieldedNote rows arrive while the sheet is
open. Add an `@Query-backed` source for shielded notes in this view, then filter
and sum client-side in shieldedPoolBalance(for:) and any denomination-related
logic, following the pattern used by the sibling shielded views so
shieldedOptionAvailable(for:) updates reactively.
---
Outside diff comments:
In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift`:
- Around line 227-291: The engine-bind loop in SwiftExampleAppApp’s startup flow
is currently gated by earlier throwing sync setup calls, so unrelated failures
can prevent engine binding for the remaining wallets. Isolate
engineBindOtherWallets from the platformAddressWallet/platformBalanceSyncService
and shieldedService start path by wrapping those earlier throws in their own
local do/catch or moving the loop outside that failure boundary, while keeping
the best-effort behavior in shieldedService.bindEngine and the non-mirror wallet
iteration intact.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- Around line 253-268: The shielded bind flow is duplicated between bind() and
bindEngine(), including dbPath resolution, account dedup/sort, and the
configureShielded/bindShielded sequence. Extract that shared logic into a single
helper method on ShieldedService (or a nearby private helper) and have both
bind() and bindEngine() call it inside their existing do/catch blocks, keeping
their own logging and published-state updates separate.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift`:
- Around line 67-94: The shielded wallet lookup in AccountListView is being
repeated multiple times during a single render, which should be avoided for
consistency with the snapshot pattern used in
SendTransactionView.coreBalanceSnapshot(). Cache the result of
shieldedAccountsForThisWallet and the per-account shieldedAddress(for:) lookup
once in body, then reuse those cached values for the section check and row
rendering instead of calling walletManager.shieldedDefaultAddress repeatedly.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ReceiveAddressView.swift`:
- Around line 101-116: The shielded receive address is being recomputed too
often because `shieldedReceiveAddress` is a computed property that re-runs the
FFI-backed `walletManager.shieldedDefaultAddress` and
`DashAddress.encodeOrchard` on every `currentAddress`/`hasValidAddress` access.
Update `ReceiveAddressView.body` to resolve the shielded address once per render
into a local `let` (or a `@State` cache tied to `selectedTab`) and have the
`.shielded` paths reuse that value, following the same pattern used by
`coreBalanceSnapshot()` for expensive FFI calls.
🪄 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: 9eb65d8b-2240-4d6d-95a4-534fbfc64508
📒 Files selected for processing (11)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedEngineBindPlan.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ReceiveAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/ShieldedEngineBindPlanTests.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers codex-general/codex-ffi-engineer completed; claude-general/claude-ffi-engineer failed due Claude extra-usage quota; verifier recovered with codex after Claude verifier quota failure.
The reported ordering bug is valid. This PR adds automatic engine binding for every non-mirror wallet, but the app starts the shielded sync loop before those wallets are registered, so the loop's immediate first pass can snapshot only the mirror wallet and delay the rest behind the next cadence/cooldown window.
🔴 1 blocking
🤖 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/SwiftExampleAppApp.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift:254-291: Register all shielded wallets before starting the sync loop
`startShieldedSync()` starts a Rust background thread whose first iteration calls `sync_now(false)` before sleeping. The coordinator snapshots the currently registered shielded accounts at the start of that pass, and a no-activity pass sets the coordinator-wide caught-up cooldown. Since this PR registers the non-mirror wallets only after starting the loop, a cold-start first pass can run with just `firstWallet`, emit results only for that wallet, and leave the newly added wallets waiting for a later tick/manual sync. Move the `engineBindOtherWallets` block before `startShieldedSync()` so the first shared pass sees the complete loaded wallet set.
| if try !walletManager.isDashPaySyncRunning() { | ||
| try walletManager.startDashPaySync() | ||
| } | ||
|
|
||
| // Engine-bind every OTHER loaded wallet into the shared | ||
| // network-scoped shielded coordinator. `firstWallet` above | ||
| // already drives the UI mirror AND its own engine | ||
| // registration via `bind(...)`; this loop registers the | ||
| // remaining wallets so a single shielded sync pass | ||
| // trial-decrypts against the union of every wallet's viewing | ||
| // keys (SH-14/15/16 cross-wallet flows). Each bind is | ||
| // best-effort + independent — one wallet's missing mnemonic | ||
| // must not block the others. Reading each mnemonic is a | ||
| // device-unlock-only keychain read (no biometric prompt), so | ||
| // eager binding at startup is safe. The iteration seam is a | ||
| // pure free function (`engineBindOtherWallets`) so its | ||
| // "visit every non-mirror wallet" contract can be | ||
| // unit-tested without a configured manager. | ||
| engineBindOtherWallets( | ||
| allWalletIds: walletManager.wallets.keys, | ||
| mirrorWalletId: wallet.walletId | ||
| ) { otherWalletId in | ||
| shieldedService.bindEngine( | ||
| walletManager: walletManager, | ||
| walletId: otherWalletId, | ||
| network: platformState.currentNetwork, | ||
| resolver: shieldedResolver | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Register all shielded wallets before starting the sync loop
startShieldedSync() starts a Rust background thread whose first iteration calls sync_now(false) before sleeping. The coordinator snapshots the currently registered shielded accounts at the start of that pass, and a no-activity pass sets the coordinator-wide caught-up cooldown. Since this PR registers the non-mirror wallets only after starting the loop, a cold-start first pass can run with just firstWallet, emit results only for that wallet, and leave the newly added wallets waiting for a later tick/manual sync. Move the engineBindOtherWallets block before startShieldedSync() so the first shared pass sees the complete loaded wallet set.
| if try !walletManager.isDashPaySyncRunning() { | |
| try walletManager.startDashPaySync() | |
| } | |
| // Engine-bind every OTHER loaded wallet into the shared | |
| // network-scoped shielded coordinator. `firstWallet` above | |
| // already drives the UI mirror AND its own engine | |
| // registration via `bind(...)`; this loop registers the | |
| // remaining wallets so a single shielded sync pass | |
| // trial-decrypts against the union of every wallet's viewing | |
| // keys (SH-14/15/16 cross-wallet flows). Each bind is | |
| // best-effort + independent — one wallet's missing mnemonic | |
| // must not block the others. Reading each mnemonic is a | |
| // device-unlock-only keychain read (no biometric prompt), so | |
| // eager binding at startup is safe. The iteration seam is a | |
| // pure free function (`engineBindOtherWallets`) so its | |
| // "visit every non-mirror wallet" contract can be | |
| // unit-tested without a configured manager. | |
| engineBindOtherWallets( | |
| allWalletIds: walletManager.wallets.keys, | |
| mirrorWalletId: wallet.walletId | |
| ) { otherWalletId in | |
| shieldedService.bindEngine( | |
| walletManager: walletManager, | |
| walletId: otherWalletId, | |
| network: platformState.currentNetwork, | |
| resolver: shieldedResolver | |
| ) | |
| } | |
| // Engine-bind every OTHER loaded wallet into the shared | |
| // network-scoped shielded coordinator. `firstWallet` above | |
| // already drives the UI mirror AND its own engine | |
| // registration via `bind(...)`; this loop registers the | |
| // remaining wallets so a single shielded sync pass | |
| // trial-decrypts against the union of every wallet's viewing | |
| // keys (SH-14/15/16 cross-wallet flows). Each bind is | |
| // best-effort + independent — one wallet's missing mnemonic | |
| // must not block the others. Reading each mnemonic is a | |
| // device-unlock-only keychain read (no biometric prompt), so | |
| // eager binding at startup is safe. The iteration seam is a | |
| // pure free function (`engineBindOtherWallets`) so its | |
| // "visit every non-mirror wallet" contract can be | |
| // unit-tested without a configured manager. | |
| engineBindOtherWallets( | |
| allWalletIds: walletManager.wallets.keys, | |
| mirrorWalletId: wallet.walletId | |
| ) { otherWalletId in | |
| shieldedService.bindEngine( | |
| walletManager: walletManager, | |
| walletId: otherWalletId, | |
| network: platformState.currentNetwork, | |
| resolver: shieldedResolver | |
| ) | |
| } | |
| if try !walletManager.isShieldedSyncRunning() { | |
| try walletManager.startShieldedSync() | |
| } | |
| // DashPay contact-request + profile sweep (background | |
| // loop). Wallet-driven — every registered wallet is swept | |
| // each pass — so manager scope is the right place to start | |
| // it, same as the address / shielded loops above. | |
| // Idempotent: starting while running is a no-op. | |
| if try !walletManager.isDashPaySyncRunning() { | |
| try walletManager.startDashPaySync() | |
| } |
source: ['codex-general', 'codex-ffi-engineer', 'codex-verifier']
There was a problem hiding this comment.
Resolved in cce68d0 — Register all shielded wallets before starting the sync loop no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
Review-fix round (cce68d0) — applied five self-review findings plus the CodeRabbit comment:
CI failure root cause ( 🤖 Generated with Claude Code |
|
CI update: the header fix in cce68d0 worked — the Fixed in 50a53a7: 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: the previous startup ordering blocker is fixed; current head binds non-mirror shielded wallets at SwiftExampleAppApp.swift lines 275-285 before startShieldedSync() at lines 287-288. New findings in the latest delta: the post-Clear manualSync() recovery path still exits before the new non-mirror rebind-all pass when the mirror wallet rebind fails, so one mirror-wallet credential failure can prevent otherwise recoverable wallets from syncing.
🔴 1 blocking
🤖 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/Services/ShieldedService.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:497-501: Do not let mirror rebind failure skip other wallets after Clear
After `clearShielded()` drops every coordinator registration, `manualSync()` is supposed to re-register every loaded wallet on the next Sync Now tap. The new non-mirror `engineBindOtherWallets` pass is below this `guard isBound else { return }`, so if the mirror wallet's `bind(...)` fails because its mnemonic is missing, declined, or corrupt, the method returns before attempting to bind any other loaded wallet and before calling `syncShieldedNow()`. That contradicts the best-effort multi-wallet recovery contract documented in this file: one wallet's bind failure must not prevent other wallets from being re-registered and synced. Run the non-mirror bind pass independently of the mirror bind result, then skip the sync only if no wallet could be registered.
Source: reviewers: opus (general, failed_or_unparseable), claude-sonnet-5 (general, failed_or_unparseable), gpt-5.5 (general), opus (ffi-engineer, failed_or_unparseable), claude-sonnet-5 (ffi-engineer, failed_or_unparseable), gpt-5.5 (ffi-engineer); verifier: gpt-5.5; specialists: ffi-engineer
| @@ -428,6 +501,28 @@ class ShieldedService: ObservableObject { | |||
| guard isBound else { return } | |||
There was a problem hiding this comment.
🔴 Blocking: Do not let mirror rebind failure skip other wallets after Clear
After clearShielded() drops every coordinator registration, manualSync() is supposed to re-register every loaded wallet on the next Sync Now tap. The new non-mirror engineBindOtherWallets pass is below this guard isBound else { return }, so if the mirror wallet's bind(...) fails because its mnemonic is missing, declined, or corrupt, the method returns before attempting to bind any other loaded wallet and before calling syncShieldedNow(). That contradicts the best-effort multi-wallet recovery contract documented in this file: one wallet's bind failure must not prevent other wallets from being re-registered and synced. Run the non-mirror bind pass independently of the mirror bind result, then skip the sync only if no wallet could be registered.
source: ['codex']
There was a problem hiding this comment.
Confirmed real and fixed in 7b01aba. The mirror bind in manualSync is now best-effort like everything else: on failure it no longer bails (its lastError stays visible), the engine-bind pass always runs so other wallets with intact mnemonics re-register, and bindEngine now reports success so the pass can tell whether ANYTHING registered — the sync is skipped only in the nothing-registered case, preserving the original "do not chain a sync that will fail the same way" intent per-wallet. Regression-checked on-simulator: Clear → visit wallet A → Sync Now still restores both wallets' sync-state rows.
There was a problem hiding this comment.
Resolved in 7b01aba — Do not let mirror rebind failure skip other wallets after Clear no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift (1)
728-748: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNil
ownShieldedAddressproduces a misleading error message.If
walletManager.shieldedDisplayAddressreturnsnil(e.g. this wallet's engine bind hasn't run yet or failed — now a real possibility per-wallet under the new eager multi-wallet binding),enteredRecipient != ownShieldedAddressis stilltrue, so the user is told they "entered the wrong address" instead of being told their own shielded address isn't resolvable yet.🐛 Suggested fix
let ownShieldedAddress = walletManager.shieldedDisplayAddress( walletId: wallet.walletId, network: network ) - if !enteredRecipient.isEmpty, - enteredRecipient != ownShieldedAddress { + guard let ownShieldedAddress else { + error = "Your shielded address isn't available yet — try re-syncing this wallet." + return + } + if !enteredRecipient.isEmpty, + enteredRecipient != ownShieldedAddress { error = "Shield always sends to your own shielded " + "address; enter your own shielded address as " + "the recipient" 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/ViewModels/SendViewModel.swift` around lines 728 - 748, The own-shielded-address validation in SendViewModel’s send flow can produce a misleading “wrong address” error when walletManager.shieldedDisplayAddress(walletId:network:) returns nil. Add an explicit nil check for ownShieldedAddress before comparing it to enteredRecipient, and surface a dedicated message that the wallet’s shielded address is not available yet (or the engine bind has not completed) instead of treating it as a recipient mismatch. Keep the existing comparison/guard logic in SendViewModel, but make the nil case return early with the correct error.
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift (1)
370-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared bind scaffolding to remove duplication between
bind()andbindEngine().
bindEnginere-implements thedbPathcomputation,sortedAccountsdedup/sort, and theconfigureShielded+bindShieldedtry/catch scaffold that already exists inbind()(lines 253-268). Both call sites will need to be kept in sync manually for any future change to that sequence (e.g., a new FFI parameter).♻️ Suggested extraction
+ /// Shared `configureShielded` + `bindShielded` scaffold used by both + /// `bind()` (UI mirror) and `bindEngine()` (other wallets). Returns + /// the deduped/sorted accounts actually passed to Rust so callers can + /// use it for their own post-processing. + `@discardableResult` + private func configureAndBindShielded( + walletManager: PlatformWalletManager, + walletId: Data, + network: Network, + resolver: MnemonicResolver, + accounts: [UInt32] + ) throws -> [UInt32] { + let dbPath = Self.dbPath(for: network) + let sortedAccounts = Array(Set(accounts)).sorted() + try walletManager.configureShielded(dbPath: dbPath) + try walletManager.bindShielded( + walletId: walletId, + resolver: resolver, + accounts: sortedAccounts + ) + return sortedAccounts + } + func bindEngine( walletManager: PlatformWalletManager, walletId: Data, network: Network, resolver: MnemonicResolver, accounts: [UInt32] = [0] ) { - let dbPath = Self.dbPath(for: network) - let sortedAccounts = Array(Set(accounts)).sorted() - do { - try walletManager.configureShielded(dbPath: dbPath) - try walletManager.bindShielded( + let sortedAccounts = try configureAndBindShielded( + walletManager: walletManager, walletId: walletId, + network: network, resolver: resolver, accounts: sortedAccounts ) SDKLogger.log( "Shielded engine-bound: walletId=\(walletId.prefix(4).map { String(format: "%02x", $0) }.joined())… network=\(network.networkName) accounts=\(sortedAccounts)", minimumLevel: .medium ) } catch { ... } }🤖 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/Services/ShieldedService.swift` around lines 370 - 428, Extract the shared shielded binding setup used by bind() and bindEngine() into a common helper so the dbPath calculation, sortedAccounts normalization, and configureShielded/bindShielded try/catch sequence live in one place. Keep bindEngine’s wallet-specific logging and non-mirroring behavior, but have both bind() and bindEngine() call the shared helper using the existing symbols walletManager, dbPath(for:), and bindShielded to avoid duplicated scaffolding and future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:
- Around line 728-748: The own-shielded-address validation in SendViewModel’s
send flow can produce a misleading “wrong address” error when
walletManager.shieldedDisplayAddress(walletId:network:) returns nil. Add an
explicit nil check for ownShieldedAddress before comparing it to
enteredRecipient, and surface a dedicated message that the wallet’s shielded
address is not available yet (or the engine bind has not completed) instead of
treating it as a recipient mismatch. Keep the existing comparison/guard logic in
SendViewModel, but make the nil case return early with the correct error.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- Around line 370-428: Extract the shared shielded binding setup used by bind()
and bindEngine() into a common helper so the dbPath calculation, sortedAccounts
normalization, and configureShielded/bindShielded try/catch sequence live in one
place. Keep bindEngine’s wallet-specific logging and non-mirroring behavior, but
have both bind() and bindEngine() call the shared helper using the existing
symbols walletManager, dbPath(for:), and bindShielded to avoid duplicated
scaffolding and future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52b35624-8a68-453d-a256-cf5474c68070
📒 Files selected for processing (17)
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentShieldedNote.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/PlatformWalletManager+ShieldedDisplay.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ReceiveAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PersistentShieldedNotePredicateTests.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.mdpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/PersisterRestartClassificationIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvRestartIntegrationTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swiftpackages/swift-sdk/build_ios.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/ReceiveAddressView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift
- packages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/CreateIdentityView.swift
There was a problem hiding this comment.
Code Review
Source: reviewers claude-general (opus, failed: extra-usage quota), codex-general (gpt-5.5); verifier codex (gpt-5.5, recovered from opus verifier failure); specialists: none.
Carried-forward prior findings: the prior cce68d01 blocker is STILL VALID at 50a53a7e. manualSync() still returns after a failed mirror rebind before running the non-mirror engineBindOtherWallets pass, so the post-Clear recovery path can skip otherwise recoverable wallets.
New findings in latest delta: none. The latest delta only migrates integration tests to the TestWalletWrapper.sendToAddresses helper. The earlier startup-ordering issue from ac684c9d remains fixed.
🔴 1 blocking
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.
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:497-501: Do not let mirror rebind failure skip other wallets after Clear
After `clearShielded()` drops every wallet registration from the Rust coordinator, `manualSync()` is documented as the recovery path that re-registers every loaded wallet on the next Sync Now tap. The mirror-wallet recovery branch still exits at `guard isBound else { return }` when `bind(...)` fails, so the non-mirror `engineBindOtherWallets` pass below never runs. That means one missing, declined, or corrupt mirror-wallet mnemonic can keep otherwise valid loaded wallets unregistered after Clear and prevent `syncShieldedNow()` from recovering them. Run the non-mirror bind pass independently of the mirror bind result, and only skip the sync when no wallet could be registered.
There was a problem hiding this comment.
Code Review
Source: reviewers claude-general (opus, failed: extra-usage quota), codex-general (gpt-5.5), claude-ffi-engineer (opus, failed: extra-usage quota), codex-ffi-engineer (gpt-5.5); verifier codex (gpt-5.5, recovered from opus verifier quota failure); specialists: ffi-engineer.
Carried-forward prior findings: prior-1 is FIXED at 7b01abaffff35c155ef69bab8f99e9032f5b1b59. manualSync() no longer returns after a failed mirror bind before running the non-mirror engine-bind pass; it now runs engineBindOtherWallets(...), tracks whether any wallet registered, and skips the sync only when no wallet could be registered.
New findings in latest delta: one suggestion remains in the same recovery path. When only non-mirror wallets bind after a mirror bind failure, the service clears the mirror bind error and opens manual sync timing even though its event handler will drop completion events while isBound is false.
1 suggestion
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/Services/ShieldedService.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:545-555: Only start mirror timing when the mirror wallet is bound
When the mirror `bind(...)` fails but at least one non-mirror `bindEngine(...)` succeeds, `anyWalletRegistered` is true while `isBound` remains false. This path then clears the mirror bind error and opens the manual sync timing bracket, but `handleShieldedSyncEvent` drops the completion at `guard isBound else { return }`, so `endSyncTiming()` is never reached for this pass. Keep syncing the successfully registered non-mirror wallets, but only clear the mirror error and start mirror timing when the mirror wallet is actually bound; otherwise close any stale timing state after the awaited sync returns.
There was a problem hiding this comment.
Code Review
Source: reviewers: opus (general, failed: extra-usage quota), gpt-5.5 (general), opus (ffi-engineer, failed: extra-usage quota), gpt-5.5 (ffi-engineer); verifier: gpt-5.5 (recovered from Opus verifier quota failure); specialists: ffi-engineer.
Prior findings reconciliation: prior-1 from 7b01abaf is STILL VALID as the same mirror-timing root cause, now through the manager-wide sync-state subscription. The latest delta fixes the direct manualSync() timing start, but the subscription can still open mirror timing while the mirror wallet is unbound.
Carried-forward prior findings: 1 suggestion, re-anchored to packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:316-317.
New findings in latest delta: none.
🟡 1 suggestion
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/Services/ShieldedService.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:316-317: Gate sync-state timing on the mirror being bound
The latest change gates the `manualSync()` fast-path timing start on `isBound`, but this subscription still starts the mirror timing bracket on any manager-wide sync start. That remains reachable after a mirror bind failure because `bind(...)` still installs the subscription, other wallets can be engine-bound, and the shared shielded loop can run for those wallets. Since `handleShieldedSyncEvent` returns immediately while `isBound` is false, the unbound mirror never closes the timing bracket for that background pass. Keep the manager-wide spinner behavior if desired, but only start mirror timing when the mirrored wallet is actually bound.
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
I don't see the build.rs you told me about but anyway, I left a couple of comments in the build_ios.sh and in the Integrations test files
…multi-wallet flows Engine-bind EVERY loaded wallet's shielded (Orchard) sub-wallet with the Rust sync coordinator, not just the lexicographically-smallest wallet. Unblocks SH-14/15/16 (cross-wallet shielded transfer / unshield / withdraw between two on-device wallets). App-side only — the Rust NetworkShieldedCoordinator was already fully multi-wallet. Design is "single UI mirror + multi-engine-bind": ShieldedService still mirrors one wallet (firstWallet) for the global Sync-status surface via bind(...); the new bindEngine(...) does engine-only registration for every other wallet. rebindWalletScopedServices() drives this through a pure, unit-testable seam (engineBindOtherWallets). Per-wallet surfaces (Receive->Shielded address, wallet balances, CreateIdentity funding gate, send self-shield guard) now read shieldedDefaultAddress(walletId:) / PersistentShieldedNote per wallet instead of the singleton mirror. Eager binding: each wallet's mnemonic is device-unlock-only in Keychain (no biometric access control), so N reads at startup trigger no prompts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…elpers, reactive pool balance Review + CodeRabbit follow-ups on the multi-wallet shielded bind: - Sync Now (manualSync) now unconditionally re-registers every loaded wallet's shielded sub-wallet. clearShielded drops EVERY wallet from the coordinator, and a detail-view switchTo in between re-binds only the viewed wallet, so the recovery branch alone left the other wallets unscanned until an app restart. Verified on-simulator: Clear -> visit wallet A -> Sync Now restores both wallets' ZPERSISTENTSHIELDEDSYNCSTATE rows. - Moved the rebind engine-bind loop ahead of the fallible shielded / DashPay start calls so an unrelated throw cannot skip it. - Deduplicated the resolve+encode address pattern (3 sites) into PlatformWalletManager.shieldedDisplayAddress, and the unspent-note predicate (4 sites) into PersistentShieldedNote.unspentPredicate. - CreateIdentityView pool balance is now @Query-backed (reactive while the sheet is open; one materialized query instead of a FetchDescriptor per call site) — addresses the CodeRabbit comment. - No "already bound" fast path in bindEngine on purpose: the only cheap probe (shieldedDefaultAddress) reflects the wallet-level sub-wallet binding, which survives clearShielded; skipping on it would leave post-Clear wallets permanently unregistered. - build_ios.sh: cargo clean the four bundled-header crates per target before building. Their build.rs writes cbindgen headers to the shared target include dir but only re-executes on fingerprint change, so a target dir reused across branches (CI runners, local branch switches) can pair one branch's Swift with another rev's header — the phantom wallet_manager_import_wallet_from_bytes signature mismatch seen on CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…allets' recovery Post-Clear, manualSync's recovery branch bailed on a failed mirror bind (guard isBound) before the engine-bind pass ran, so a missing / declined / corrupt mirror mnemonic left every OTHER loaded wallet unregistered and skipped the sync — violating the documented best-effort contract. The mirror bind is now best-effort like the rest: the engine pass always runs (bindEngine reports success so the pass can tell), and the sync is skipped only when NO wallet at all could be registered, preserving the original "don't chain a doomed sync" intent per-wallet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…view The stale-header mitigation (cargo clean of the bundled-header crates before each target build) is superseded by the header-pipeline rework in #3853, which owns forced regeneration. The underlying mechanism — build.rs writes cbindgen headers to the shared target/<triple>/<profile>/include/ path but is skipped when the crate's fingerprint is fresh, so a target dir reused across branches can pair one branch's Swift with another rust-dashcore rev's header — is left to that PR to solve at the build.rs level. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
claude/affectionate-perlman-a750af
branch
from
b4c3c3d to
f946509
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude-general (opus, failed: extra-usage quota), codex-general (gpt-5.5), claude-ffi-engineer (opus, failed: extra-usage quota), codex-ffi-engineer (gpt-5.5); verifier codex (gpt-5.5, recovered from opus verifier failure); specialists: ffi-engineer.
Carried-forward prior findings: prior-1 is still valid; the manager-wide shielded sync-state subscription still opens the UI mirror timing bracket even when the mirrored wallet is not bound. New findings in latest delta: the latest commit also removed the isBound guard in manualSync(), so non-mirror-only syncs can clear the mirror bind error and leave stale timing state. No CodeRabbit inline findings were provided to validate.
🟡 2 suggestion(s)
2 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/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:316-322: Carried-forward prior: Gate sync timing on the mirror being bound
The `$shieldedSyncIsSyncing` publisher is manager-wide, but this service mirrors one wallet. `bind(...)` installs this subscription even after `bindShielded` fails and leaves `isBound == false`; another wallet can still be engine-bound and drive the shared shielded sync loop. In that case this subscriber opens `currentSyncStartedAt`, while `handleShieldedSyncEvent` immediately returns at `guard isBound else { return }`, so the timing bracket is never closed for a sync pass that did not belong to the mirrored wallet.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/ShieldedService.swift:545-561: New latest-delta: Do not mutate mirror timing for non-mirror syncs
The latest delta removed the `isBound` guard around `manualSync()`'s mirror-only state updates. If the mirror bind fails but another loaded wallet binds successfully, `anyWalletRegistered` is true while this mirror remains unbound; `manualSync()` now clears the bind error and opens timing before running a sync whose completion event is dropped by `handleShieldedSyncEvent`. Keep the mirror error and timing changes scoped to a bound mirror, and close any unbound timing opened by the manager-wide publisher.
3 participants
Issue being fixed or feature implemented
The iOS SwiftExampleApp only bound the lexicographically-smallest wallet's shielded (Orchard) sub-wallet to the Rust sync coordinator. With multiple wallets loaded, every non-first wallet's shielded pool was invisible: no receive address, no balance, no note sync. This blocked the multi-wallet shielded QA items SH-14 (A→B shielded transfer between two on-device wallets), SH-15 (unshield A→B platform address), and SH-16 (shielded withdraw A→B core address).
This is app-side only — the Rust
NetworkShieldedCoordinatorwas already fully multi-wallet (one shared per-network commitment tree; each sync pass trial-decrypts every block against the union of all registered viewing keys and routes hits to each wallet's own persister). The single-bind limit was purely a UI-service choice.What was done?
Design: "single UI mirror + multi-engine-bind".
ShieldedServicestill mirrors exactly one wallet (firstWallet) for the global Sync-status surface viabind(...).ShieldedService.bindEngine(walletManager:walletId:network:resolver:accounts:)does engine-only registration (configureShieldedidempotent +bindShielded), no mirror repoint, best-effort (catches + logs, never throws).rebindWalletScopedServices()now engine-binds every other loaded wallet through a pure, unit-testable seamengineBindOtherWallets(allWalletIds:mirrorWalletId:bindEngine:).walletManager.shieldedDefaultAddress(walletId:account:0)+DashAddress.encodeOrchard(ReceiveAddressView,AccountListView,SendViewModelself-shield guard).PersistentShieldedNotescoped bywalletId(BalanceCardView,SendTransactionView,CreateIdentityView).CreateIdentityView.shieldedOptionAvailable(for:)relaxed from "mirror-bound" to "engine-bound + own pool > 0 + fallback address".CoreContentView) stays mirror-backed (unchanged).Eager vs. lazy binding: chose eager (bind all wallets at rebind time). Each wallet's mnemonic is stored
kSecAttrAccessibleWhenUnlockedThisDeviceOnly(device-unlock only;.biometryCurrentSetguards only the separate opt-in biometric seed item), so N reads at startup are fast keychain reads with no biometric/passcode prompts. This matches the Android eager-bind pattern to be ported next, and makes cross-wallet flows work with no wallet-swap/rebind.Clear (SH-12) scope:
clearShieldeddrops all wallets from the coordinator (global wipe, unchanged). Only the mirror re-binds on "Sync Now"; the other wallets re-bind on the nextrebindWalletScopedServicesfire. This is deliberate — an immediate re-bind + sync restart would repopulate before the tester can observe SH-12's reset. Documented inline.The change is portable: the same shape will be ported to Android (
KotlinExampleAppAppContainer.rebindWalletScopedServices+ShieldedServiceinpackages/kotlin-sdk).How Has This Been Tested?
build_ios.sh --target sim(features =shielded) then the app compiles clean — zero errors, zero Swift 6 strict-concurrency warnings.ShieldedEngineBindPlanTestspass — binds every non-mirror wallet, one failing bind doesn't block the rest, single-wallet no-op, mirror skipped by identity regardless of position.1211CF…/ B357386…):tdash1…Orchard address (A…w5af7z…, B…qjeqvfhl…). Under the old code both would have shown wallet A's mirror address —shieldedDefaultAddress(walletId:)returns non-nil only for a wallet actually registered in the coordinator.ZPERSISTENTSHIELDEDSYNCSTATEhas one row per wallet — a single sync pass processed both registrations (thewalletCount == 2behavior). No crashes.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes