refactor(dashpay): call the SDK's ordered wallet bring-up before Core SPV - #961
Conversation
…act payments Two independent defects behind "DashPay contacts and transactions only come back after a resync". **The identity was found once, or not until relaunch.** Recovery runs inside `PlatformAddressSyncCoordinator.performStart`, which early-returns when the coordinator is already running — so a restored wallet gets exactly one scan, in the seconds right after restore, when the network is least likely to answer. A scan that came back empty was then recorded in `completedContexts` as final for the process, because an empty result was reported as success. No identity means no DashPay tabs, no contacts and no contact payment history for the whole session. Only a found identity now ends the search. An empty or failed scan retries on a 20s/60s/180s backoff. The retries run outside the runtime-start pipeline, so each pass re-resolves the live wallet through `SwiftDashSDKHost.shared.wallet` and checks `runningNetwork` instead of holding the handle it started with — a wipe, wallet switch or network switch between retries ends the search rather than scanning against a torn-down runtime. **A failed `platformAddressWallet()` took DashPay down with it.** That path returned before the shielded bind, the DashPay sync start AND identity recovery, none of which use the address wallet. It now records `lastError` and continues; `addressWallet` was already an optional the rest of the method handles, since the no-Platform-account branch sets it to nil and carries on. **The feed never learned that contact payments had arrived.** A DashPay row's true direction, amount and contact name come from `DashPayPaymentTxLookup`, whose rows are written by an app-pulled projection into entities `saveTouchesFeedRows` filters out, and are read through a computed property on rows that were already rendered. The one DashPay-aware reload fires when the identity is adopted — before the sync loop has fetched anything. So the feed kept dash-spv's misread direction (an outgoing contact payment reads as incoming) and a nameless "?" avatar until something unrelated happened to touch `PersistentTransaction`. Whether that happened decided whether the bug appeared: a wallet still catching up repainted by accident, a quiet one never did. The lookup now posts `DWDashPayPaymentTxLookupDidChange` when the snapshot actually changes (`PaymentInfo` gained `Equatable` for the comparison), and `observeDashPay()` reloads the feed on it. Gated on a real change so the projection's timer cannot turn into a periodic rebuild of the whole history list. Also stop arming the projection's 60s throttle from a call that returned early for want of an identity: that spent the launch's first window on a no-op, and the identity typically lands seconds later.
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDashPay startup now validates wallet binding and prepares contact-address subsystems before Core SPV synchronization. Readiness failures do not stop SPV startup. The SPV coordinator also centralizes repeated-start checks. ChangesDashPay contact readiness
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SwiftDashSDKSPVCoordinator
participant PlatformWalletManager
participant DashPayContactAddressReadiness
participant CoreSPV
SwiftDashSDKSPVCoordinator->>PlatformWalletManager: start(network:)
PlatformWalletManager-->>SwiftDashSDKSPVCoordinator: ManagedPlatformWallet
SwiftDashSDKSPVCoordinator->>DashPayContactAddressReadiness: await readiness
DashPayContactAddressReadiness->>PlatformWalletManager: startWalletSubsystems
DashPayContactAddressReadiness-->>SwiftDashSDKSPVCoordinator: continue despite startup errors
SwiftDashSDKSPVCoordinator->>CoreSPV: start shared SPV flow
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
…ry on teardown Both findings from the review on #950. **A successful address sync erased the address-wallet failure.** Making `platformAddressWallet()` non-fatal meant the Platform address sync now starts even when that wallet is missing — and `handleSyncEvent` clears `lastError` on every successful pass. The startup failure was published once and then wiped by the first success, so the status screen reported a healthy sync over address surfaces that cannot work. The two fail independently, so the startup error is held in its own field and re-published wherever `lastError` is cleared on success; teardown clears it. **Recovery retries could outlive the wallet.** The first attempt runs inside the serialized runtime start, which is what kept the wallet handle alive across its FFI scan. The retries deliberately run outside that serialization and only re-check the wallet *before* awaiting, so a pass suspended inside `discoverIdentities` could still be running when `fullReset` dropped the handle. `cancelPendingWork()` cancels the pending backoffs and — the part that matters — awaits each task, since a task suspended in a synchronous FFI call never observes cancellation. `fullReset` calls it before `SwiftDashSDKHost.stop()`. Two smaller consequences of the same ownership question: - `recoverIfNeeded` now cancels a superseded backoff *before* its guards. An attempt already in flight keeps `activeContexts` set, and returning early there left the old backoff scheduled behind it. - A finished retry task no longer removes itself from `retryTasks`. That dictionary is teardown's handle on the work, and a self-removal could delete an entry a newer start had already replaced, putting that task out of reach. Awaiting a finished task is free.
cee4d63 to
16f8b46
Compare
…cycle Review follow-up on #950. The doc comment claimed the value is "cleared only by teardown or a start that resolves the wallet", but `clearDisplay()` also clears it, and that runs on wipe and on network-switch preparation too. Those are all paths that invalidate the wallet the error was recorded against, so the behaviour is right — the description was not, which is exactly what the repo's comment rule exists to catch. Also drop the redundant `= nil` on the optional (SwiftLint `redundant_optional_initialization`).
16f8b46 to
89fd5aa
Compare
…retry Review follow-up on #950: "empty is provably empty, there is no reason to rescan if the network proves to you that you have no identity." That is right, and it is right *because* platform#4352 landed. Before it, an empty result could mean either "Platform says this seed owns no identity" or "we never reached Platform", so retrying an empty was the only way to survive the second case. Now an unanswered scan raises `IdentityDiscoveryIncomplete` and the two are distinguishable, which is what makes a returned empty trustworthy enough to record as final. So the backoff now fires only when the scan threw. A scan that returns — with or without an identity — marks the context complete and stops. A wallet that genuinely owns no identity no longer pays three pointless network round trips per launch. `attempt` returns "Platform answered" rather than "an identity was found"; the call site and the doc comments say so.
Review: "the swift client should not be doing this, it should be in rust." Correct, and `packages/swift-sdk/CLAUDE.md` says so outright — "No iteration / gap-limit walks / policy loops in Swift", and "if it's deciding anything — how many, which index, which path, which key, which order — move the decision to Rust. If you find a decision that Rust doesn't currently let you ask for by a single call, add the helper in the Rust library first." A hand-rolled 20s/60s/180s backoff with its own per-context bookkeeping is exactly that. Reverting it here rather than carrying it as a stopgap, so the policy has one home when it lands in `rs-platform-wallet` alongside the startup-ordering work. Little is lost in the meantime: platform#4352 already made an unreachable scan raise `IdentityDiscoveryIncomplete` instead of an empty success, so the restored behaviour marks a context complete only when Platform actually answered, and a failed scan is retried on the next runtime start. The gap that remains is healing time within one session, not correctness. `cancelPendingWork` and its `fullReset` hook go with it — they existed only to keep those retry tasks from outliving the wallet. What stays in this PR is the part that has no Rust equivalent: the transaction feed not repainting when DashPay payment rows land, and the address-wallet startup failure no longer taking the DashPay subsystems down with it.
|
Converting to draft. The reviewer is right that this belongs in Rust, and
One idea investigated and rejected before landing here: making the unattended DashPay sweep resolve its own signer Rust-side, the way The replacement is a single Rust orchestration entry point on Keeping the branch rather than closing it: the testnet evidence it produced is what justifies the Rust work — 14 deferred account builds drained where the previous build logged 350 enqueues and 0 drains, and |
Startup order is now identity → contacts → contact accounts → core sync.
A contact's DIP-15 addresses are derived from its contact account, and an
address the wallet is not watching when the scan passes its funding
height produces no transaction at all. Until now the wallet started Core
SPV first and repaired afterwards: `reconcile_dashpay_rescan` (DIP-15
§12.6) lowers the SPV synced height once a contact account finally
appears, so the scan re-walks blocks it had already covered. On a
restore that is the slowest possible path to a correct contact history.
The third step is the one that is easy to get wrong. After
`dashPaySyncNow()` the accounts still do not exist: the recurring DashPay
sweep runs unattended, holds no signer, and can only enqueue the builds
("Deferred DashPay account build"). They come into being when a
signer-present drain runs, so the sequence unlocks from the keychain and
waits for the queue to settle. Stopping after the contact sync would have
ordered the steps correctly and still started SPV with nothing extra to
watch.
Lives in the slot `SwiftDashSDKSPVCoordinator` already reserves for work
that must precede `startSpv`, alongside the CoinJoin recovery gap widen
and the pending chain resync — both there for the same reason, that the
first filter set has to be complete. Placed in `performStart(for:)` and
not the shared `performStart(manager:for:)` so a Core-only restart does
not pay for a Platform round trip it cannot benefit from. The coordinator
start order in `SwiftDashSDKWalletRuntime` is deliberately untouched:
swapping those two calls would also move the shielded bind ahead of SPV,
which nothing here needs.
Core sync is the wallet's primary function and Platform is not, so the
whole sequence is bounded at 20s and every step is best-effort. When the
budget is spent SPV starts anyway and the DIP-15 rescan backfills — the
fallback is exactly the behaviour that shipped before, never worse. The
bound is a real deadline, not just a between-steps check: the contact
sync is polled to a completion flag so a hung pass cannot hold the wallet
without a balance. It is not cancelled, because cancelling a Swift task
cannot abort a call already inside Rust, and a late pass still lands.
`reconcile_dashpay_rescan` stays load-bearing regardless — contacts
established later in a session, or on a later day, always arrive after
the scan. This removes the restore case from its workload, not the
mechanism.
Review: "the swift client should not be doing this, it should be in rust." `packages/swift-sdk/CLAUDE.md` agrees — no policy loops in Swift, and "if it's deciding anything — how many, which index, which path, which key, which order — move the decision to Rust. If you find a decision that Rust doesn't currently let you ask for by a single call, add the helper in the Rust library first." So the helper was added first. `PlatformWalletManager.startWalletSubsystems` now owns the sequence, the discovery retry policy and the budget; this file is the call site and its logging, holding no ordering logic of its own. The hand-rolled step sequencer, the wall-clock deadline and the two poll loops are gone — 209 lines down to 89, and Android gets the same behaviour without reimplementing it. The statuses are also better than what the Swift version could report. It could only say "ready" or "budget spent"; the SDK distinguishes a Platform that proved this seed owns no identity from one that never answered, so the log no longer reads as a failure when there is simply nothing to prepare. One idea rejected on the way: making the unattended DashPay sweep resolve its own signer, which would have removed the need for any sequencing at all. `MnemonicResolver` is a Swift-owned vtable pinned to a single synchronous call, and the auto-accept half of the drain needs a full identity signer — turning either into a standing capability is a security-posture change, not a refactor.
89fd5aa to
75c0fbd
Compare
romchornyi
changed the title
Follows the SDK gaining a terminal `discoveryFailed` status (dashpay/platform#4359): a wallet or persistence fault, as opposed to an unreachable Platform. Logged at error rather than warning because it is the one outcome here that nothing will clear on its own — not this session, not the next launch.
|
Unblocked and out of draft: platform#4359 merged into Re-verified after the merge, since #4359 changed on its way in (the drains became deadline-bounded and self-clearing, the scan key became a lazily-invoked resolver erased by its own guard):
Unrelated finding while doing that, worth its own fix: the Still stacked on #950; will retarget to |
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
The new startup call bypasses the existing wrong-seed guard. On a persisted restore, loadFromPersistor() calls unlockWalletFromKeychain; if the Keychain mnemonic does not bind to this wallet, that verification rejects it and publishes dashPayUnlockStatus[walletId].seedMismatch specifically so DashPay signing stays disabled. awaitReady then unconditionally calls startWalletSubsystems, whose Swift bridge creates a fresh MnemonicResolver / KeychainSigner but neither runs verify_seed_binds nor honors that mismatch state. For a wallet with an already-known identity it therefore proceeds directly to contact sync/drain using the rejected seed, potentially deriving/registering incorrect DIP-15 accounts or attempting auto-accept signing. Please enforce the seed-binding check inside the SDK startup helper (preferred, so every client is protected), or otherwise prevent mnemonic-derived startup work when the manager has already rejected that wallet/seed binding.
…ay-before-core-sync
![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
🤖 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
`@DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKSPVCoordinator.swift`:
- Around line 242-243: The performStart(manager:for:) flow must check its
existing same-network-running guard before calling
DashPayContactAddressReadiness.awaitReady, so repeated starts return without
re-running startWalletSubsystems or waiting for readiness. Preserve the current
idempotent behavior, and only perform readiness retries through a separate retry
path.
🪄 Autofix
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 Plus
Run ID: 2e53bd01-2c39-49b2-ae6b-e1a213ec6fa2
📒 Files selected for processing (4)
DashWallet.xcodeproj/project.pbxprojDashWallet/Sources/Infrastructure/SwiftDashSDK/Contacts/DashPayContactAddressReadiness.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/PlatformAddressSyncCoordinator.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKSPVCoordinator.swift
From review on #961. The readiness pass sits in `performStart(for:)` rather than the shared `performStart(manager:for:)` on purpose — a Core-only restart re-enters the shared one and must not pay for a Platform round trip it cannot benefit from. But the start-elision guard lives in that shared path, so a repeated start on a running network reached the readiness pass first and waited out the SDK's whole startup budget before hitting a guard that returns immediately. The condition now has one home, `isAlreadyRunning(manager:network:)`, consulted by both. Duplicating it inline would have left the two free to drift back apart, which is how this appeared in the first place.
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
Re-reviewed at ddb60cf. The repeated-start fix looks correct: the shared isAlreadyRunning predicate now prevents the readiness pass from adding latency when SPV is already up.
The blocker from my previous review is still unresolved, though. I also checked the current v4.2-dev implementation of startWalletSubsystems: it still constructs a fresh MnemonicResolver / KeychainSigner without performing the cached seed-binding verification or honoring dashPayUnlockStatus[walletId].seedMismatch. Therefore this call can still run mnemonic-derived contact-account/auto-accept work after unlockWalletFromKeychain has explicitly rejected that Keychain seed for this wallet. Please keep this as changes requested until the SDK helper enforces the binding (preferred) or the iOS call site skips the helper after a mismatch.
From review on #961. `startWalletSubsystems` builds its own `MnemonicResolver` and `KeychainSigner`, so it did not inherit the wrong-seed gate that `unlockWalletFromKeychain` applies. On a wallet whose identity is already known the bring-up skips discovery and goes straight to the contact sync and drain — deriving DIP-15 contact accounts from a mnemonic this wallet has already rejected. That damage does not heal. `register_contact_account`'s existence check keys on the contact pair, not the xpub, so a wrong receiving xpub is written once and every later correct-seed run no-ops over it; unlike the external-account side there is no rotation sweep to rebuild it. The wallet ends up watching addresses nobody pays to, with no symptom beyond payments that never appear. The verify runs here rather than reading the published `seedMismatch` flag, because that flag is racing this call: `SwiftDashSDKHost.loadPersistedWallet` schedules the unlock in a detached task and returns immediately, so on the launch where the seed is wrong the flag may not be set yet. Performing the verification is what makes the check race-free. It is marker-cached in the SDK, so a match costs a string comparison rather than a Keychain read. A watch-only wallet still proceeds: with no stored mnemonic there is no binding to contradict, and the bring-up already handles that case.
|
Fixed in Two things I found while tracing it that sharpen the report: The published flag cannot be the gate. The Clean On your preferred fix — enforcing this inside the SDK helper so every client is protected — I agree that is the right home, and it is not what this PR does. The reason it belongs in |
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
The seed-mismatch hole is closed, but using unlockWalletFromKeychain as the verification primitive introduces a drain race. This method is not verify-only: when pending contact-crypto exists it starts a detached drain and sets the Swift-side dashPayUnlockStatus.draining guard. Immediately afterward startWalletSubsystems runs its own Rust-side drain, which does not consult or publish that guard. The existing SDK code explicitly avoids stacking drains because two snapshots can duplicate the network/ECDH work and race channel-broken/auto-accept side effects. This is reachable both with a persisted pending queue and when the host watcher sees work queued by the inline contact sync while the startup drain is running. Please make the preflight use a verify-only binding API, or otherwise serialize/centralize the drain so the verification and ordered bring-up cannot launch competing drains.
From review on #961. The previous commit used `unlockWalletFromKeychain` as the preflight, which is not a verification primitive: when the contact-crypto queue is non-empty it also starts a detached drain, so the bring-up's own drain would race a second one over a second snapshot — duplicated network and ECDH work, and competing channel-broken / auto-accept writes. There was no verify-only API to call instead, so the gate moved into the SDK, where it belongs anyway: dashpay/platform#4368 adds `PlatformWalletManager.verifySeedBinding` and has `startWalletSubsystems` call it before touching any key material. Every client is now protected, not just this call site. This file goes back to what it was: the call site and its logging. A mismatch now surfaces as a throw from the bring-up, which the existing catch already handles the right way — log it and start Core SPV, because a wallet without a balance is worse than a wallet without DashPay state. Requires platform#4368; the SDK is a local path dependency.
|
You are right, and the drain race is exactly why there was no clean way to do this app-side: So I took your preferred option instead: dashpay/platform#4368.
One point from your first review that I want to put on the record, because it shaped the fix: the published I also stated there what I could confirm about blast radius, so the fix is not over-scoped: This PR now needs #4368 to land first — the SDK is a local path dependency. Clean |
|
Dependency cleared: dashpay/platform#4368 merged into State of this PR against that:
Diff is 4 files / 129 lines, all of it the call site: @llbartekll both of your points are addressed — the seed-binding hole in the SDK, and the drain race that the first attempt at fixing it introduced. Ready for another look whenever you have a moment. |
3 participants
Issue being fixed or feature implemented
A DashPay contact's DIP-15 payment addresses are derived from its contact account, and an address the wallet is not watching when the compact-filter scan passes its funding height produces no transaction at all. Core SPV therefore has to wait until those addresses exist, or a restored wallet's contact payment history is missing until the DIP-15 rescan repairs it.
The wallet had no way to wait: it started SPV first and repaired afterwards. Worse, nothing was building the contact accounts at all — the unattended sweep can only enqueue the work, and the unlock that drains that queue ran before the sweep had queued anything. Baseline on the previous build, testnet restore, 7 established contacts:
Deferred DashPay account build: enqueued— 350 timescontact-crypto drain skipped — no pending ops— 7 timespayments projection — 0 row(s)— 14 timesWhat was done?
DashPayContactAddressReadinessnow makes one call —PlatformWalletManager.startWalletSubsystems— from the slotSwiftDashSDKSPVCoordinatoralready reserves for work that must precedestartSpv, alongside the CoinJoin recovery-gap widen and the pending chain resync (both there for the same reason: the first filter set has to be complete).The sequence, the discovery retry policy and the budget all live in
rs-platform-wallet(platform#4359). This file holds no ordering logic of its own — it is the call site and its logging.Deliberately placed in
performStart(for:)rather than the sharedperformStart(manager:for:), so a Core-only restart does not pay for a Platform round trip it cannot benefit from. The coordinator start order inSwiftDashSDKWalletRuntimeis untouched: swapping those two calls would also move the shielded bind ahead of SPV, which nothing here needs.The statuses are better than the Swift version could report. It knew only "ready" or "budget spent"; the SDK distinguishes a Platform that proved this seed owns no identity from one that never answered, so the log no longer reads as a failure when there is simply nothing to prepare.
reconcile_dashpay_rescan(DIP-15 §12.6) stays load-bearing — contacts established later in a session, or on a later day, always arrive after the scan. This removes the restore case from its workload, not the mechanism.How Has This Been Tested?
Clean
dashpaybuild. Testnet session on the iOS 26.5 simulator, wallet with 7 established contacts:reconcile_sent_payments_from_tx_historymatched 18 txids and the payment projection went 0 → 21 rows, with contact rows rendering names and correct direction.no identity for this seed; nothing to prepare before SPV) and SPV starts immediately — verified three times in one session, no added launch latency.Worth being precise about what that run proves: the wallet needed ~12 minutes to catch up on chain, so the payments were recovered by
reconcile_dashpay_rescan, not by the ordering. The ordering targets a different case — a restore against an already-synced chain, where the blocks have been walked and nothing will re-scan them. Both mechanisms are needed; this run exercised the drain and the sequencing, and confirmed neither breaks the rescan path.No unit tests: the unit-test target is currently broken (pre-existing, noted in
CLAUDE.md), and the logic that used to live here is now tested inrs-platform-wallet(8 tests). Not yet run on a physical device.Breaking Changes
None.
For reviewers: Core SPV start can be delayed by up to the SDK's budget (20s default) on a launch that has DashPay work to do. It self-skips when there is nothing to prepare, and on expiry falls through to the previous behaviour — the fallback is never worse than what shipped before.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit