feat(syncing): gate Core spending during initial restore sync - #995
feat(syncing): gate Core spending during initial restore sync#995llbartekll wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThe change adds wallet-scoped initial restore tracking and shared Core spend and asset-lock proof availability policies. Core-funded payment, identity, transfer, and UI flows now enforce these policies. Wallet-origin APIs replace Boolean import flags, with lifecycle and payment preflight tests added. ChangesRestore synchronization and Core spend gating
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Wallet
participant SwiftDashSDKHost
participant InitialRestoreSyncStore
participant SwiftDashSDKSPVCoordinator
participant CoreSpendAvailability
Wallet->>SwiftDashSDKHost: create or restore wallet
SwiftDashSDKHost->>InitialRestoreSyncStore: record pending restore
SwiftDashSDKSPVCoordinator->>InitialRestoreSyncStore: record effective completion
InitialRestoreSyncStore->>CoreSpendAvailability: publish availability change
CoreSpendAvailability-->>Wallet: enable or block Core-funded actions
sequenceDiagram
participant PaymentClient
participant BIP70PaymentService
participant CoreSpendAvailability
participant WalletSendService
PaymentClient->>BIP70PaymentService: confirm or headless send
BIP70PaymentService->>CoreSpendAvailability: run preflight
CoreSpendAvailability-->>BIP70PaymentService: allow or initialRestoreSync
BIP70PaymentService->>WalletSendService: build and send transaction
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
llbartekll
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletSending.swift (1)
22-26: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTwo Core-spend preflights map every error to
BIP70Error.initialRestoreSync. Both sites wrapCoreSpendAvailability.shared.requireAllowed()in a catch-all.requireAllowed()throws onlyCoreSpendAvailabilityError.initialRestoreSynctoday, so the labels are correct now. IfCoreSpendAvailability.Decisiongains a second blocked case, both sites will report the wrong reason to the user. Match the concrete case instead.
DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletSending.swift#L22-L26: replace the catch-all withcatch CoreSpendAvailabilityError.initialRestoreSyncand rethrow other errors unchanged.DashWallet/Sources/Infrastructure/SwiftDashSDK/BIP70PaymentService+App.swift#L19-L26: apply the same specific catch inside thecoreSpendPreflightclosure.♻️ Proposed fix for both sites
- do { - try await CoreSpendAvailability.shared.requireAllowed() - } catch { - throw BIP70Error.initialRestoreSync - } + do { + try await CoreSpendAvailability.shared.requireAllowed() + } catch CoreSpendAvailabilityError.initialRestoreSync { + throw BIP70Error.initialRestoreSync + }🤖 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 `@DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletSending.swift` around lines 22 - 26, Replace the catch-all around CoreSpendAvailability.shared.requireAllowed() in SwiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletSending.swift:22-26 with a specific catch for CoreSpendAvailabilityError.initialRestoreSync, while rethrowing other errors unchanged. Apply the same handling inside the coreSpendPreflight closure in DashWallet/Sources/Infrastructure/SwiftDashSDK/BIP70PaymentService+App.swift:19-26.DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKHost.swift (1)
408-415: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
publishalready refreshesCoreSpendAvailability, so line 415 repeats the work.
publish(handles:wallet:)callsCoreSpendAvailability.shared.refresh()at line 894. The explicit call at line 415 runs a second resolution and a secondmigrateLegacyFlagIfNeededpass in the same turn. The result is identical, so this is a redundancy, not a defect. Remove one call to keep a single refresh owner.♻️ Proposed cleanup
publish(handles: handles, wallet: createdWallet) - CoreSpendAvailability.shared.refresh()🤖 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 `@DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKHost.swift` around lines 408 - 415, Remove the redundant CoreSpendAvailability.shared.refresh() call after publish(handles:wallet:) in the wallet creation flow; keep the refresh owned by publish and preserve the existing publish behavior.DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletWiper.swift (1)
174-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider merging this main-queue hop into the existing commit block.
performWipealready runs aDispatchQueue.main.syncblock at lines 159-167 for the main-actor commit work. This second hop adds another synchronous main-queue round trip for a single UserDefaults write. MoveInitialRestoreSyncStore.shared.removeAll()into the earlier block if the ordering relative toSPVChainResyncMarker.resetForWipe()does not matter.🤖 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 `@DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletWiper.swift` around lines 174 - 176, Update performWipe by moving InitialRestoreSyncStore.shared.removeAll() into its existing DispatchQueue.main.sync commit block alongside the main-actor wipe work, eliminating the second synchronous main-queue hop. Preserve the current ordering relative to SPVChainResyncMarker.resetForWipe() unless that ordering is required.DashWallet/Sources/Infrastructure/SwiftDashSDK/WalletEnvironment.swift (1)
190-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the SwiftLint warnings on the new declarations.
SwiftLint reports three issues in this hunk: closure end indentation at line 193, attribute placement at line 214, and
static_over_final_classat line 214. The repository guidelines require SwiftLint conventions.♻️ Proposed fix
observers.append(notificationCenter.addObserver( forName: name, object: nil, queue: .main) { [weak self] _ in - Task { `@MainActor` in self?.refresh() } - }) + Task { `@MainActor` in self?.refresh() } + })- `@objc` nonisolated class func coreSpendBlockedError() -> NSError? { + `@objc` + nonisolated static func coreSpendBlockedError() -> NSError? { guard blockedSnapshot else { return nil } return CoreSpendAvailabilityError.initialRestoreSync as NSError }As per coding guidelines: "Use SwiftFormat and SwiftLint conventions".
Also applies to: 214-214
🤖 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 `@DashWallet/Sources/Infrastructure/SwiftDashSDK/WalletEnvironment.swift` around lines 190 - 194, Apply SwiftFormat/SwiftLint conventions in WalletEnvironment: correct the closing observer closure indentation, place the declaration attribute in the required position, and resolve static_over_final_class on the declaration around line 214 by using the appropriate type design for its static-only API. Preserve the existing observer behavior and MainActor refresh flow.Sources: Coding guidelines, Linters/SAST tools
DashWalletTests/PassiveWalletStateUITailTests.swift (1)
169-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the new SwiftLint warnings.
Move
InitialRestoreSyncStoreTeststo its own test file. Replace the implicitly unwrapped properties with initialized or safely optional test fixtures.As per coding guidelines: “Use SwiftFormat and SwiftLint conventions.”
🤖 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 `@DashWalletTests/PassiveWalletStateUITailTests.swift` around lines 169 - 172, Remove the SwiftLint warnings in InitialRestoreSyncStoreTests by moving the test class into its own test file and replacing the implicitly unwrapped suiteName, defaults, and store properties with initialized values or safely handled optionals. Preserve the test fixture setup and behavior while following SwiftFormat and SwiftLint conventions.Sources: Coding guidelines, Linters/SAST tools
DashWallet/Sources/UI/Payments/Pay/SendViewModel.swift (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
CoreSpendAvailabilityobservation boilerplate across three view models.SendViewModel,InternalTransferViewModel, andDashSpendPayViewModeleach hand-roll the same Combine subscription to mirrorCoreSpendAvailability.shared.decisioninto a local flag and re-trigger their own validation. The shared root cause is the lack of a common helper for this pattern.
DashWallet/Sources/UI/Payments/Pay/SendViewModel.swift#L107-118: extract theCoreSpendAvailability.shared.$decision.map(\.isBlocked).removeDuplicates().sink { ... }subscription into a small reusable helper (e.g. a protocol default or a shared Combine extension) that any@MainActor ObservableObjectview model can call with its own update closure.DashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferViewModel.swift#L550-560: reuse the same helper instead of repeating the identical subscription.DashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift#L273-278: reuse the same helper; this site can also drop the redundant.receive(on: RunLoop.main), sinceCoreSpendAvailabilitymutations already happen on the main actor.🤖 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 `@DashWallet/Sources/UI/Payments/Pay/SendViewModel.swift` at line 1, Extract the repeated CoreSpendAvailability.shared.$decision.map(\.isBlocked).removeDuplicates() subscription into a reusable helper for `@MainActor` ObservableObject view models, accepting each caller’s local update closure and preserving its validation trigger. Replace the duplicated subscriptions in SendViewModel, InternalTransferViewModel, and DashSpendPayViewModel with the helper, and remove DashSpendPayViewModel’s redundant receive(on: RunLoop.main).DashWallet/Sources/Models/PaymentProtocol/BIP70PaymentService.swift (1)
178-185: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove the default
coreSpendPreflightclosure.The tracked
scripts/bip70_manual_test/main.swiftfactory omitscoreSpendPreflightand relies on the no-op default. Remove the default and pass{}explicitly there if the manual test intentionally bypasses the app preflight gate. Production and unit-test construction sites already provide the closure.🤖 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 `@DashWallet/Sources/Models/PaymentProtocol/BIP70PaymentService.swift` around lines 178 - 185, Remove the default value from the coreSpendPreflight parameter in the PaymentService initializer, making the closure required at all call sites. Update the tracked bip70_manual_test factory to pass an explicit no-op closure where it intentionally bypasses preflight, while preserving the existing closures supplied by production and unit-test callers.
🤖 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/Identity/DWIdentityRegistrationCoordinator.swift`:
- Around line 491-497: Update the Core availability guard in the registration
coordinator to require Core availability only when both recoveryLock and the
existing identity lookup are absent. Reuse lookupExistingIdentityId (or its
existing result) so pending recoveries with a consumed or removed asset-lock row
reach topUpResumedIdentityIfNeeded, while preserving the later Core top-up check
for resumes that actually need funding.
In
`@DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKSPVCoordinator.swift`:
- Around line 66-68: Use SyncingActivityMonitor.state == .syncDone as the sole
restore-completion authority: remove the raw-SPV/progress logic from
isEffectivelyComplete in
SwiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKSPVCoordinator.swift
(lines 66-68), complete InitialRestoreSyncStore from a lifecycle observer of
.syncDone at lines 626-633, and update
DashWalletTests/PassiveWalletStateUITailTests.swift lines 249-255 to verify the
monitor completion contract rather than the raw SPV predicate.
In
`@DashWallet/Sources/UI/DashPay/Setup/CreateUsername/CreateUsernameViewController.swift`:
- Around line 282-283: Replace the direct-purchase checks in
CreateUsernameViewController with one operation-specific predicate owned by an
`@MainActor` ObservableObject ViewModel or shared policy, matching
startPurchaseUsername’s requireAllowed() conditions (no identity or insufficient
heldCredits). Reuse that predicate for button availability, SyncGateNote
visibility, and coordinator flow at the referenced locations, rather than
applying coreSpendAvailability.isBlocked unconditionally.
In `@DashWallet/Sources/UI/Payments/Pay/SendScreen.swift`:
- Around line 1134-1143: Update SyncGateNote to accept operation-specific
explanatory text instead of hardcoding the Transparent-balance sending message.
Provide appropriate messages at its send, CreateUsernameViewController, and
SDKIdentityProfileSheet call sites so each flow accurately describes the blocked
operation.
---
Nitpick comments:
In `@DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKHost.swift`:
- Around line 408-415: Remove the redundant
CoreSpendAvailability.shared.refresh() call after publish(handles:wallet:) in
the wallet creation flow; keep the refresh owned by publish and preserve the
existing publish behavior.
In
`@DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletSending.swift`:
- Around line 22-26: Replace the catch-all around
CoreSpendAvailability.shared.requireAllowed() in
SwiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletSending.swift:22-26
with a specific catch for CoreSpendAvailabilityError.initialRestoreSync, while
rethrowing other errors unchanged. Apply the same handling inside the
coreSpendPreflight closure in
DashWallet/Sources/Infrastructure/SwiftDashSDK/BIP70PaymentService+App.swift:19-26.
In
`@DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletWiper.swift`:
- Around line 174-176: Update performWipe by moving
InitialRestoreSyncStore.shared.removeAll() into its existing
DispatchQueue.main.sync commit block alongside the main-actor wipe work,
eliminating the second synchronous main-queue hop. Preserve the current ordering
relative to SPVChainResyncMarker.resetForWipe() unless that ordering is
required.
In `@DashWallet/Sources/Infrastructure/SwiftDashSDK/WalletEnvironment.swift`:
- Around line 190-194: Apply SwiftFormat/SwiftLint conventions in
WalletEnvironment: correct the closing observer closure indentation, place the
declaration attribute in the required position, and resolve
static_over_final_class on the declaration around line 214 by using the
appropriate type design for its static-only API. Preserve the existing observer
behavior and MainActor refresh flow.
In `@DashWallet/Sources/Models/PaymentProtocol/BIP70PaymentService.swift`:
- Around line 178-185: Remove the default value from the coreSpendPreflight
parameter in the PaymentService initializer, making the closure required at all
call sites. Update the tracked bip70_manual_test factory to pass an explicit
no-op closure where it intentionally bypasses preflight, while preserving the
existing closures supplied by production and unit-test callers.
In `@DashWallet/Sources/UI/Payments/Pay/SendViewModel.swift`:
- Line 1: Extract the repeated
CoreSpendAvailability.shared.$decision.map(\.isBlocked).removeDuplicates()
subscription into a reusable helper for `@MainActor` ObservableObject view models,
accepting each caller’s local update closure and preserving its validation
trigger. Replace the duplicated subscriptions in SendViewModel,
InternalTransferViewModel, and DashSpendPayViewModel with the helper, and remove
DashSpendPayViewModel’s redundant receive(on: RunLoop.main).
In `@DashWalletTests/PassiveWalletStateUITailTests.swift`:
- Around line 169-172: Remove the SwiftLint warnings in
InitialRestoreSyncStoreTests by moving the test class into its own test file and
replacing the implicitly unwrapped suiteName, defaults, and store properties
with initialized values or safely handled optionals. Preserve the test fixture
setup and behavior while following SwiftFormat and SwiftLint conventions.
🪄 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: 74a9f33b-e231-4504-9ef7-d1cb767b8753
📒 Files selected for processing (30)
DashWallet.xcodeproj/project.pbxprojDashWallet/Sources/Application/Syncyng Activity Monitor/SyncingActivityMonitor.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/BIP70PaymentService+App.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/Identity/DWIdentityRegistrationCoordinator.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/PlatformAddressSyncCoordinator.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKHost.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKKeyMigrator.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKSPVCoordinator.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletCreator.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletSending.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletWiper.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/WalletEnvironment.swiftDashWallet/Sources/Models/PaymentProtocol/BIP70Error.swiftDashWallet/Sources/Models/PaymentProtocol/BIP70PaymentService.swiftDashWallet/Sources/Models/Transactions/WalletSendService.swiftDashWallet/Sources/UI/DashPay/Profile/SDKIdentityProfileSheet.swiftDashWallet/Sources/UI/DashPay/Setup/CreateUsername/CreateUsernameViewController.swiftDashWallet/Sources/UI/Explore Dash/Merchants & ATMs/Details/Views/POIDetailsView.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swiftDashWallet/Sources/UI/Menu/Security/Wallets/WalletsScreen.swiftDashWallet/Sources/UI/Menu/Security/Wallets/WalletsViewModel.swiftDashWallet/Sources/UI/Payments/Amount/Model/Send/SendAmountModel.swiftDashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferViewModel.swiftDashWallet/Sources/UI/Payments/InternalTransfer/ShieldedTransferCoordinator.swiftDashWallet/Sources/UI/Payments/Pay/SendScreen.swiftDashWallet/Sources/UI/Payments/Pay/SendViewModel.swiftDashWallet/Sources/UI/Payments/PaymentModels/DWPaymentProcessor.mDashWallet/Sources/UI/Setup/RecoverWallet/DWRecoverWalletCommand.mDashWalletTests/PassiveWalletStateUITailTests.swiftDashWalletTests/PaymentProtocolTests.swift
💤 Files with no reviewable changes (1)
- DashWallet/Sources/UI/Setup/RecoverWallet/DWRecoverWalletCommand.m
|
Addressed the review in Also applied all seven hidden nitpick cleanups: specific BIP70 error mapping, removal of redundant refresh/main-queue work, SwiftLint declaration formatting, a shared blocked-state publisher, explicit BIP70 preflight injection, and moving restore-store tests into a dedicated file without implicitly unwrapped fixtures. Validation:
The suggestion to complete restore state from the global |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/bip70_manual_test/main.swift (1)
220-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the new preflight contract in the harness.
Line 220 passes an always-successful closure. The harness cannot detect whether
confirmAndSendskips preflight or calls it afterbuildSignedTransaction. Record apreflightevent and assert that it occurs beforebuild. Add a throwing-preflight case that asserts neither build nor broadcast occurs.Suggested test instrumentation
func makeService(_ transport: FakeTransport, _ wallet: FakeWallet, receive: FakeReceive = FakeReceive(), auth: FakeAuth = FakeAuth(), - allowUntrusted: Bool = true) -> BIP70PaymentService { + allowUntrusted: Bool = true, + recordPreflight: Bool = false) -> BIP70PaymentService { BIP70PaymentService(transport: transport, verifier: PaymentRequestVerifier(), wallet: wallet, - receiveAddress: receive, auth: auth, coreSpendPreflight: {}, + receiveAddress: receive, auth: auth, + coreSpendPreflight: { + if recordPreflight { wallet.calls.append("preflight") } + }, allowUntrustedUnsigned: allowUntrusted) } -// Call order build → post/ACK → broadcast. +// Call order preflight → build → post/ACK → broadcast. ... - let svc = makeService(t, w) + let svc = makeService(t, w, recordPreflight: true) ... - check("confirmAndSend order == [build, post, broadcast]", w.calls == ["build", "post", "broadcast"]) + check( + "confirmAndSend order == [preflight, build, post, broadcast]", + w.calls == ["preflight", "build", "post", "broadcast"])Also applies to: 234-242
🤖 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 `@scripts/bip70_manual_test/main.swift` around lines 220 - 221, Update the manual-test harness around confirmAndSend to use an instrumented preflight closure that records a “preflight” event, then assert the event precedes “build”. Add a separate throwing-preflight case and verify that neither buildSignedTransaction nor broadcast is invoked, while preserving the existing successful send assertions.
🤖 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 `@scripts/bip70_manual_test/main.swift`:
- Around line 220-221: Update the manual-test harness around confirmAndSend to
use an instrumented preflight closure that records a “preflight” event, then
assert the event precedes “build”. Add a separate throwing-preflight case and
verify that neither buildSignedTransaction nor broadcast is invoked, while
preserving the existing successful send assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e7d61df6-a2ba-410a-bd0e-ccafcb49e71f
📒 Files selected for processing (17)
DashWallet.xcodeproj/project.pbxprojDashWallet/Sources/Infrastructure/SwiftDashSDK/BIP70PaymentService+App.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/Identity/DWIdentityRegistrationCoordinator.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKHost.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletSending.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletWiper.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/WalletEnvironment.swiftDashWallet/Sources/Models/PaymentProtocol/BIP70PaymentService.swiftDashWallet/Sources/UI/DashPay/Profile/SDKIdentityProfileSheet.swiftDashWallet/Sources/UI/DashPay/Setup/CreateUsername/CreateUsernameViewController.swiftDashWallet/Sources/UI/DashPay/Setup/CreateUsername/CreateUsernameViewModel.swiftDashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swiftDashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferViewModel.swiftDashWallet/Sources/UI/Payments/Pay/SendScreen.swiftDashWallet/Sources/UI/Payments/Pay/SendViewModel.swiftDashWalletTests/InitialRestoreSyncStoreTests.swiftscripts/bip70_manual_test/main.swift
💤 Files with no reviewable changes (1)
- DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKHost.swift
🚧 Files skipped from review as they are similar to previous changes (12)
- DashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift
- DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletWiper.swift
- DashWallet/Sources/UI/Payments/Pay/SendScreen.swift
- DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletSending.swift
- DashWallet/Sources/Infrastructure/SwiftDashSDK/BIP70PaymentService+App.swift
- DashWallet/Sources/UI/DashPay/Profile/SDKIdentityProfileSheet.swift
- DashWallet/Sources/UI/DashPay/Setup/CreateUsername/CreateUsernameViewController.swift
- DashWallet/Sources/UI/Payments/Pay/SendViewModel.swift
- DashWallet/Sources/Models/PaymentProtocol/BIP70PaymentService.swift
- DashWallet/Sources/Infrastructure/SwiftDashSDK/WalletEnvironment.swift
- DashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferViewModel.swift
- DashWallet/Sources/Infrastructure/SwiftDashSDK/Identity/DWIdentityRegistrationCoordinator.swift
|
Addressed the incremental CodeRabbit nitpick in The standalone BIP70 harness now records and asserts |
|
The gate fails open, and one of the two ways it does that looks unintentional.
If yes, this is closed and worth a line in the code saying so, since the fail-open branch is otherwise indistinguishable from the legacy one. If there's a window where the wallet is spendable and the marker isn't yet persisted, that window is exactly an unblocked restore. |
romchornyi
left a comment
romchornyi
left a comment
There was a problem hiding this comment.
Approving — the narrowing is the right call.
The old gate blocked every Core spend until SyncingActivityMonitor.state == .syncDone, which folded in masternode/quorum sync. A mainnet user of ours is sitting on exactly that: Headers/Filters/Blocks at 100%, masternode list stuck at 0, and every send refused even though the UTXO set was fully discovered. The stale-UTXO reasoning behind that gate really only describes the initial restore scan, where the wallet does not yet know its own outputs — a routine catch-up carries the same residual risk as the second after .syncDone. So the gate now matches its actual justification instead of over-reaching.
Also good: the check runs before authentication and before any input reservation, and it covers the paths that matter rather than just the obvious send screen — BIP70, gift cards, identity funding, internal transfer, and sendToContact.
Two things left as comments, neither blocking: the fail-open branch in refresh() (posted separately), and the fact that the 217 lines of new tests have not been executed by anyone — the author reports XCTest could not bootstrap locally, and this repo runs no test job in CI. Worth a local run on a working configuration before merge, given what this code gates.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/WalletEnvironment.swift`:
- Line 152: Update the AssetLockProofAvailability type documentation to
explicitly justify the lifecycle of its static shared singleton, or introduce a
protocol seam and inject the policy instead. Prefer the smallest change that
satisfies the guideline while preserving existing
AssetLockProofAvailability.shared usage.
- Around line 201-213: Update WalletEnvironment.decision to replace the
masternodes.state == .synced gate with SyncingActivityMonitor’s .syncDone
status, while preserving the boundWalletId/progressWalletId check and existing
height validation. Use the established monitor instance or synchronization
result rather than introducing a separate SPV-state condition.
- Around line 132-136: Update AssetLockProofAvailabilityError.errorDescription
to use operation-neutral wording that applies to transfers, identity
registration, and platform funding, while preserving the existing explanation
that operations become available after masternode list synchronization
completes.
🪄 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: 3f7b4b8e-a6fe-4b6d-accf-0f86e4dfb03e
📒 Files selected for processing (16)
DashWallet/Sources/Infrastructure/SwiftDashSDK/AssetLockRecoveryService.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/Identity/DWIdentityRegistrationCoordinator.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/PlatformAddressSyncCoordinator.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKSPVCoordinator.swiftDashWallet/Sources/Infrastructure/SwiftDashSDK/WalletEnvironment.swiftDashWallet/Sources/UI/DashPay/Profile/SDKIdentityProfileSheet.swiftDashWallet/Sources/UI/DashPay/Setup/CreateUsername/CreateUsernameViewController.swiftDashWallet/Sources/UI/Home/Views/CoinJoinMoveFundsSheet.swiftDashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferConfirmSheet.swiftDashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferScreen.swiftDashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferViewModel.swiftDashWallet/Sources/UI/Payments/InternalTransfer/ShieldedTransferCoordinator.swiftDashWallet/Sources/UI/Payments/Pay/SendScreen.swiftDashWallet/Sources/UI/Payments/Pay/SendViewModel.swiftDashWallet/Sources/UI/Tx/Details/TxDetailViewController.swiftDashWalletTests/InitialRestoreSyncStoreTests.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- DashWallet/Sources/Infrastructure/SwiftDashSDK/PlatformAddressSyncCoordinator.swift
- DashWallet/Sources/UI/DashPay/Profile/SDKIdentityProfileSheet.swift
- DashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferViewModel.swift
- DashWallet/Sources/UI/Payments/Pay/SendViewModel.swift
- DashWallet/Sources/Infrastructure/SwiftDashSDK/Identity/DWIdentityRegistrationCoordinator.swift
| var errorDescription: String? { | ||
| NSLocalizedString( | ||
| "The masternode list is still syncing. Transfers that require InstantSend will be available once it finishes.", | ||
| comment: "Asset-lock operation blocked while the masternode list syncs") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use operation-neutral error text.
AssetLockProofAvailabilityError also blocks identity registration and platform funding. The current text says only “Transfers,” so it gives an incorrect action description in those flows.
Proposed fix
- "The masternode list is still syncing. Transfers that require InstantSend will be available once it finishes.",
+ "The masternode list is still syncing. Operations that require InstantSend will be available once it finishes.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var errorDescription: String? { | |
| NSLocalizedString( | |
| "The masternode list is still syncing. Transfers that require InstantSend will be available once it finishes.", | |
| comment: "Asset-lock operation blocked while the masternode list syncs") | |
| } | |
| var errorDescription: String? { | |
| NSLocalizedString( | |
| "The masternode list is still syncing. Operations that require InstantSend will be available once it finishes.", | |
| comment: "Asset-lock operation blocked while the masternode list syncs") | |
| } |
🤖 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 `@DashWallet/Sources/Infrastructure/SwiftDashSDK/WalletEnvironment.swift`
around lines 132 - 136, Update AssetLockProofAvailabilityError.errorDescription
to use operation-neutral wording that applies to transfers, identity
registration, and platform funding, while preserving the existing explanation
that operations become available after masternode list synchronization
completes.
| var isBlocked: Bool { self == .blockedMasternodeSync } | ||
| } | ||
|
|
||
| static let shared = AssetLockProofAvailability() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the singleton justification or add a protocol seam.
AssetLockProofAvailability.shared is a new Infrastructure singleton. The type documentation describes its policy, but it does not justify the singleton lifecycle. Add that justification or inject the policy through a protocol.
As per coding guidelines: “Do not add a static let shared singleton without a protocol seam or written justification in the type documentation.”
🤖 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 `@DashWallet/Sources/Infrastructure/SwiftDashSDK/WalletEnvironment.swift` at
line 152, Update the AssetLockProofAvailability type documentation to explicitly
justify the lifecycle of its static shared singleton, or introduce a protocol
seam and inject the policy instead. Prefer the smallest change that satisfies
the guideline while preserving existing AssetLockProofAvailability.shared usage.
Source: Coding guidelines
| static func decision( | ||
| boundWalletId: Data?, | ||
| progressWalletId: Data?, | ||
| masternodes: MasternodesSubProgress? | ||
| ) -> Decision { | ||
| guard let boundWalletId, | ||
| boundWalletId == progressWalletId, | ||
| let masternodes, | ||
| masternodes.state == .synced, | ||
| masternodes.currentHeight > 0, | ||
| masternodes.targetHeight > 0, | ||
| masternodes.currentHeight >= masternodes.targetHeight | ||
| else { return .blockedMasternodeSync } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Replace the SPV-state gate with SyncingActivityMonitor.
Line 209 makes proof availability depend on masternodes.state == .synced. Keep the wallet-ID binding, but derive synchronization completion from SyncingActivityMonitor and .syncDone. The current condition can disagree with the app-wide synchronization contract and leave asset-lock operations blocked or enabled at the wrong time.
As per coding guidelines: “Never gate synchronization on SPV state == .synced; use SyncingActivityMonitor and .syncDone.”
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 204-204: Declaration masternodes contains the term "master" which is not considered inclusive
(inclusive_language)
[Warning] 208-208: Declaration masternodes contains the term "master" which is not considered inclusive
(inclusive_language)
🤖 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 `@DashWallet/Sources/Infrastructure/SwiftDashSDK/WalletEnvironment.swift`
around lines 201 - 213, Update WalletEnvironment.decision to replace the
masternodes.state == .synced gate with SyncingActivityMonitor’s .syncDone
status, while preserving the boundWalletId/progressWalletId check and existing
height validation. Use the established monitor instance or synchronization
result rather than introducing a separate SPV-state condition.
Source: Coding guidelines
2 participants
Summary
Validation
Notes
Summary by CodeRabbit
New Features
Bug Fixes