Repository navigation
fix(dashdex): show which provider can swap each coin right now - #1297
romchornyi wants to merge 9 commits into
Conversation
The coin picker labelled and offered coins from SwapKit's static /tokens lists, so a coin stayed selectable while neither NEAR nor Maya could route it, and the address screen then reported a 404 noRoutesFound as a connection problem. Probe each coin with a quote forced to one provider (about $50 of the sold asset, at most two requests in flight, cached): NEAR is checked on sentinel coins first and treated as down for every coin when they have no route; Maya is checked per coin. Both usable -> "Multiple", one -> that provider, none -> the existing halted row. Buy is included. The address screen reads the error body: a refused address keeps the address copy, a missing route is re-checked without the address, and only a transport failure says "check your connection". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 119 files, which is 19 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.
 |
|
⛔ Final review complete — 3 blocking finding(s) (commit c7fcc41) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of head 1db5c15 confirms two blocking defects: one Maya flavor can incorrectly veto another, and expired availability answers can leave picker rows disabled. Current exact-head CI reports simulator tests, test-target membership, and accessibility passing; the PR-title check fails because swap is not an allowed scope. No local builds or tests were run.
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Use availability-neutral toast copy for non-chain outages
DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinView.swift:72
The new availability logic sets isHalted and shows this toast when every capable provider reports no route, including a NEAR DASH-leg outage without any chain halt. The existing message therefore attributes these newly handled failures to a halted chain even when none was reported. Use cause-neutral localized copy such as “Some coins can't be swapped at the moment” so the toast accurately covers both chain halts and provider routing outages.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
💬 Nitpick: Remove the unused Array.chunked helper
DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift:1178-1193
The PR removes verifyBuyRoutability, including the sole call to chunked(into:). A repository-wide search confirms this private extension now has no callers. Remove it with the batching implementation it supported.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change is substantial and intricate but modifies swap availability probing, provider filtering, and address-validation feedback rather than transaction execution, UTXO selection, cryptography, or another listed critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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 `DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift`:
- [BLOCKING] DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift:324-327: Evaluate Maya flavor refusals before collapsing them into one provider
`refusedBy` maps both MAYACHAIN and MAYACHAIN_STREAMING to `.maya`, so a refusal from either flavor removes the entire aggregated Maya state here. With `requested == [.maya]`, MAYACHAIN reporting `invalidAsset` and MAYACHAIN_STREAMING reporting `apiRequestFailed` produces `.unknown` in `classifyPerProvider`, but this loop deletes it and returns `.refused("invalidAsset")`. The same incorrect rejection occurs when the other flavor reports `sellAssetAmountTooSmall`, which the classifier treats as evidence of routability. Both flavors are requested together and have different token lists, so this can block a coin supported by the other flavor. Evaluate refusals per raw SwapKit provider before aggregating Maya, and add address-check regression cases for refusal plus inconclusive and refusal plus below-minimum responses.
- [NITPICK] DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift:1178-1193: Remove the unused Array.chunked helper
The PR removes `verifyBuyRoutability`, including the sole call to `chunked(into:)`. A repository-wide search confirms this private extension now has no callers. Remove it with the batching implementation it supported.
In `DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinViewModel.swift`:
- [BLOCKING] DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinViewModel.swift:180-184: Re-render picker rows when availability answers expire
This loop starts probes but never re-renders existing rows. `SwapRouteAvailability.readableEntry` and `readableVerdict` stop returning answers after 1,800 seconds without emitting `changes`, while inconclusive probes only update back-off timestamps. If a coin was rendered halted and subsequent refreshes return only inconclusive answers, its row remains disabled after the store has forgotten the negative answer and would offer the provider again. Returning to the same picker does not repair this because `loadCoins()` skips an already-populated list. Make expiration observable, or re-render the cached pools on freshness ticks and re-entry. Cover the halted answer → expiration → inconclusive refresh sequence at the ViewModel boundary.
In `DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinView.swift`:
- [SUGGESTION] DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinView.swift:72: Use availability-neutral toast copy for non-chain outages
The new availability logic sets `isHalted` and shows this toast when every capable provider reports no route, including a NEAR DASH-leg outage without any chain halt. The existing message therefore attributes these newly handled failures to a halted chain even when none was reported. Use cause-neutral localized copy such as “Some coins can't be swapped at the moment” so the toast accurately covers both chain halts and provider routing outages.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Coins no provider can ever carry are still listed as available — The base already returned all Sell pools and left assets outside both capability lists selectable and unlabelled. This PR does not introduce that discovery/classification mismatch, and filtering those legacy entries would expand the availability fix into a separate asset-discovery policy change. It does not meet the bar for an exceptional high-value out-of-scope follow-up.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| var states = LiveRouteState.classifyPerProvider(response, requested: requested) | ||
| for provider in refusedBy.keys { | ||
| states[provider] = nil | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
c7fcc41d; see the replies below.
🔴 Blocking: Evaluate Maya flavor refusals before collapsing them into one provider
refusedBy maps both MAYACHAIN and MAYACHAIN_STREAMING to .maya, so a refusal from either flavor removes the entire aggregated Maya state here. With requested == [.maya], MAYACHAIN reporting invalidAsset and MAYACHAIN_STREAMING reporting apiRequestFailed produces .unknown in classifyPerProvider, but this loop deletes it and returns .refused("invalidAsset"). The same incorrect rejection occurs when the other flavor reports sellAssetAmountTooSmall, which the classifier treats as evidence of routability. Both flavors are requested together and have different token lists, so this can block a coin supported by the other flavor. Evaluate refusals per raw SwapKit provider before aggregating Maya, and add address-check regression cases for refusal plus inconclusive and refusal plus below-minimum responses.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at c7fcc41d): You now keep refusals at the concrete Maya-flavor level and classify the remaining errors before eliminating the aggregate provider. The added tests verify that a usable or inconclusive sibling permits continuation and that two refusals preserve SwapKit's response order.
| while !Task.isCancelled { | ||
| if !displayedPools.isEmpty { | ||
| swapProvider.refreshRouteAvailability(for: displayedPools, direction: direction) | ||
| } | ||
| try? await Task.sleep(nanoseconds: routeRefreshInterval) |
There was a problem hiding this comment.
✅ Resolved at
c7fcc41d; see the replies below.
🔴 Blocking: Re-render picker rows when availability answers expire
This loop starts probes but never re-renders existing rows. SwapRouteAvailability.readableEntry and readableVerdict stop returning answers after 1,800 seconds without emitting changes, while inconclusive probes only update back-off timestamps. If a coin was rendered halted and subsequent refreshes return only inconclusive answers, its row remains disabled after the store has forgotten the negative answer and would offer the provider again. Returning to the same picker does not repair this because loadCoins() skips an already-populated list. Make expiration observable, or re-render the cached pools on freshness ticks and re-entry. Cover the halted answer → expiration → inconclusive refresh sequence at the ViewModel boundary.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at c7fcc41d): You now remove expired entries and verdicts at refresh entry and publish a change independently of replacement probe results. That reaches the picker's existing subscription, and the added expiry-notification tests cover both individual answers and the NEAR verdict.
…Core coins (#1287) Closes #1274. Pre-checks Core-funded asset locks against the 100-input cap before the PIN prompt, maps the SDK's refusal (FFI code 60) on every route to one Combine coins sheet, and pins platform e72022ab (dashpay/platform#5712). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: preserve failed shielded withdrawals in history * fix: reconcile failed shielded withdrawal destination tags * fix: preserve unknown shielded activity status
* chore(ci): pin platform to the KeyWallet helpers Point `.platform-ref` at the head of dashpay/platform#5727, `v5.1-dev` plus the `Mnemonic` and `ManagedPlatformWallet` helpers this branch uses. It contains the previous pin `57b5e5fa3`, and its protocol 13 and 14 (`v13.rs`, `v14.rs`) are identical to it. This is a temporary integration pin. Move it to the merge commit of #5727 once that PR is merged. * refactor(tests): share the CloudKit-free in-memory container `PlatformBalanceReaderTests` kept its in-memory SwiftData container builder private. It moves to `DashModelContainer.inMemoryWithoutCloudKit()` in shared test support, so the key tools tests can reuse it instead of copying it. * refactor(wallet): derive wallet ids with `Mnemonic.walletId` The platform SDK is dropping the key-wallet FFI, so `Wallet(mnemonic:network:)`, `Mnemonic.toSeed` and `key_wallet_derive_address_from_seed` go away. Wallet ids now come from `Mnemonic.walletId(for:network:)` in the host, the wiper's network resolver and the remove-wallet check. The migrator and creator no longer run their seed sanity check before creating the wallet. It compared two runs of a pure function, so it could never fail, and `createOrImportWallet` derives the id itself, failing through the same retry path. `SwiftDashSDKStoredWalletNetworkResolver.walletIds(for:)` maps a failed id derivation to `SwiftDashSDKWalletDeletionError.invalidMnemonic`, which `keysFailure` already classifies. `PlatformWalletError` stays a store failure because store calls throw it too. The phrase repairer takes a candidate's first receive address from `Mnemonic.firstReceiveAddress(for:network:)`, which accepts every SDK language, and the test pins the address of the all-`abandon` phrase. * refactor(crowdnode): sign and check ownership through the wallet The signer and the ownership check built a throwaway key-wallet from the mnemonic with `SwiftDashSDKHost.derivationWallet()` and scanned BIP44 account 0 in Swift, and that FFI surface is going away. Both now ask the running wallet, which runs the same scan in Rust: BIP44 account 0, external chain then internal, indexes 0 to 299, first match wins. `sign` takes the key from `bip44AddressPrivateKey(address:)`, which Rust only returns when the key controls the address, so the signer fails closed. `ownership(of:)` takes the path from `findBIP44AddressPath(address:)`, which uses public keys only, so the check never reads the phrase. A lookup that throws reads as `.walletUnavailable`, not as a miss. Nothing needs the address's hash160 any more, so `hash160(ofAddress:network:)` becomes `isP2PKH(_:network:)` and `OwnershipLookup` takes an `isP2PKH` closure. A P2SH address still reads `.foreign`. Rust tests cover the search and the key check. * refactor(tools): read the BIP44 xpub from the persisted account `ExtendedPublicKeysModel` rendered the xpub of a throwaway KeyWallet wallet built from the phrase. It now renders the persisted BIP44 account 0 `accountExtendedPubKeyBytes` with `PlatformWalletManager.accountExtendedPubKeyString(bytes:)`. The running wallet is rebuilt from the persisted account bytes, so its BIP44 account 0 exists only when they do. Without them the sheet shows "Not available", and the phrase is never read. `SwiftDashSDKWalletSource` kept the Standard `accountType` as a private constant, so it moves to an internal `PersistentAccount.standardAccountType` that both read. `loadBIP44AccountXpub` takes the model context and wallet id, so the new `KeyToolsTests` run it on an in-memory store. They pin that it ignores rows differing in account index, BIP32 tag, account type or wallet, answers nil without the row or its bytes, and renders the BIP44 account 0 row. * refactor(tools): read masternode keys from the running wallet The masternode owner and voting key screens read the running wallet's provider address pool, and `providerKeyAtIndex` derives the keys. The pool a throwaway KeyWallet wallet derived from the phrase is gone. An empty pool means the running wallet could not read a provider account of that kind, for example because it has none, and `providerKeyAtIndex` throws for it. `poolIsLive` becomes a computed `hasProviderPool`. `SwiftDashSDKHost.derivationWallet()`, the throwaway key-wallet these screens and CrowdNode used, has no callers left and is deleted.
…1308) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* feat(ui): rebuild Reset wallet info in SwiftUI The wipe warning (More → Security → Reset wallet, and removing the last wallet in Wallets) is now a SwiftUI screen with the DashUIKit nav bar and buttons, replacing the Objective-C controller and its storyboard. Same copy and behavior: Continue opens the recovery-phrase check in wipe mode, and a completed wipe still goes to the caller's DWWipeDelegate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ui): address review on the SwiftUI Reset wallet info screen - "Wipe Wallet" carries the header trait for VoiceOver. - Doc comments name both entry points (Security, and removing the last wallet in Wallets). - The hosting controller builds its root view once; the buttons reach it through a small target object instead of placeholder closures. - ResetWalletInfoWipeForwardingTests: a completed wipe reaches the DWWipeDelegate, which is held weakly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> SDD-Published-From: b74eae0efdd70d3ee1a0a80343f3a5dfc106651a
romchornyi
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of head 512adee confirms defects in expiry-driven rendering, Maya refusal aggregation, code-less error classification, and sentinel-based recovery detection. Current CI passes simulator tests, accessibility auditing, and test-target membership; only PR-title validation fails, which is unrelated to code correctness. No local builds or tests were run.
🔴 4 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Use availability-neutral toast copy for non-chain outages
DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinView.swift:78
The new haltedAssets source includes provider-wide NEAR outages and per-coin noRoutesFound responses, but this toast attributes all unavailable coins to a halted chain. A routing-provider outage can therefore display this warning while the destination chain is operating normally. Use localized availability-neutral wording for the combined state, or preserve the reason and reserve chain-halt copy for actual chain halts.
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general)
💬 Nitpick: Remove the unused Array.chunked helper
DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift:1225-1240
Replacing the old Buy verification batches removed the only chunked(into:) call. The private Array extension has no remaining callers on this head; remove it with the batching implementation it supported.
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The supplied diff includes intricate funds-movement and coin-selection changes in SwiftDashSDKTransactionSender.swift and AssetLockInputPolicy.swift, including Core coin consolidation, input budgeting, and transaction signing paths. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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 `DashWallet/Sources/Models/SwapKit/SwapRouteAvailability.swift`:
- [BLOCKING] DashWallet/Sources/Models/SwapKit/SwapRouteAvailability.swift:67-70: Keep code-less provider errors inconclusive
SwapKitProviderError.errorCode is optional, but compactMap discards errors with missing or blank codes instead of preserving them as .unknown. A forced NEAR response containing top-level noRoutesFound and a message-only provider error consequently reaches the empty-state fallback and becomes conclusive .noRoute; two such sentinel replies disable NEAR for every coin. A message-only Maya flavor error beside the other flavor's swapHalted error likewise becomes .halted instead of inconclusive. Map unclassified provider errors to .unknown so they participate in the existing most-favorable-wins rule, and test both the top-level-no-route and mixed-flavor cases.
- [BLOCKING] DashWallet/Sources/Models/SwapKit/SwapRouteAvailability.swift:382-385: Allow the third sentinel to detect recovery after chain halts
A halted sentinel increments neither noRoutes nor inconclusive, so halted + noRoute or halted + halted skips SOL even though its answer could establish a verdict. This also blocks recovery: after a cached NEAR .down verdict, BTC halted and ETH halted leave that verdict unchanged even if SOL would route. refresh then suppresses every per-coin NEAR check, keeping unrelated NEAR-capable coins disabled until another verdict can be established or the old one expires. The Proposed ADR and testAHaltedSentinelIsNotStoodInFor explicitly encode this gate, but do not address that recovery failure. Allow the third probe when the first two have established neither up nor down; this remains within the three-probe limit and does not count a halt as evidence of an outage. Update the proposed decision and tests accordingly.
In `DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinView.swift`:
- [SUGGESTION] DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinView.swift:78: Use availability-neutral toast copy for non-chain outages
The new haltedAssets source includes provider-wide NEAR outages and per-coin noRoutesFound responses, but this toast attributes all unavailable coins to a halted chain. A routing-provider outage can therefore display this warning while the destination chain is operating normally. Use localized availability-neutral wording for the combined state, or preserve the reason and reserve chain-halt copy for actual chain halts.
In `DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift`:
- [NITPICK] DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift:1225-1240: Remove the unused Array.chunked helper
Replacing the old Buy verification batches removed the only chunked(into:) call. The private Array extension has no remaining callers on this head; remove it with the batching implementation it supported.
- [BLOCKING] DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift:324-327: Evaluate Maya flavor refusals before collapsing them into one provider
(existing thread: https://github.com/dashpay/dashwallet-ios/pull/1297#discussion_r4447683220)
refusedBy collapses MAYACHAIN and MAYACHAIN_STREAMING into the same .maya key, and this loop removes their entire aggregated outcome when either flavor refuses. With requested: [.maya], a streaming invalidDestinationAddress error plus a non-streaming sellAssetAmountTooSmall answer produces .available in classifyPerProvider, but that state is deleted and the address check returns .refused. A sibling apiRequestFailed answer is similarly overridden even though inconclusive answers are intended to permit the real amount quote. Nonempty routes already pass through the earlier guard; the defect affects these route-less but usable sibling outcomes. Evaluate refusals and usability per raw provider name before collapsing Maya flavors, and cover the mixed-refusal cases in tests.
In `DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinViewModel.swift`:
- [BLOCKING] DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinViewModel.swift:180-184: Re-render picker rows when availability answers expire
(existing thread: https://github.com/dashpay/dashwallet-ios/pull/1297#discussion_r4447683227)
The freshness loop starts probes but does not re-render existing rows. After 30 minutes, readableEntry/readableVerdict silently discard the last conclusive answers, while inconclusive replacement probes only update back-off timestamps and emit no changes event. A previously halted coin can therefore remain disabled indefinitely in the published coins array even though the store now falls back to usable static capability. Returning to the same picker does not repair this because loadCoins() skips an already-loaded list. Publish effective expiry transitions, or re-render independently on freshness ticks and re-entry. Add a ViewModel regression test covering a halted answer that expires while subsequent probes remain unknown; the existing ageing test checks only direct store reads.
| let states = (response.providerErrors ?? []).compactMap { error -> LiveRouteState? in | ||
| guard let code = error.errorCode?.trimmingCharacters(in: .whitespacesAndNewlines), !code.isEmpty else { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
c7fcc41d; see the replies below.
🔴 Blocking: Keep code-less provider errors inconclusive
SwapKitProviderError.errorCode is optional, but compactMap discards errors with missing or blank codes instead of preserving them as .unknown. A forced NEAR response containing top-level noRoutesFound and a message-only provider error consequently reaches the empty-state fallback and becomes conclusive .noRoute; two such sentinel replies disable NEAR for every coin. A message-only Maya flavor error beside the other flavor's swapHalted error likewise becomes .halted instead of inconclusive. Map unclassified provider errors to .unknown so they participate in the existing most-favorable-wins rule, and test both the top-level-no-route and mixed-flavor cases.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at c7fcc41d): You now classify absent or blank provider codes as unknown instead of dropping them. The classifier and added regressions preserve that uncertainty beside a top-level no-route or another Maya flavor's halt.
| for (index, sentinel) in Self.nearSentinels.enumerated() where noRoutes < 2 { | ||
| // The third sentinel only stands in for one that gave no conclusive answer. | ||
| if index >= 2, inconclusive == 0 { | ||
| break |
There was a problem hiding this comment.
✅ Resolved at
c7fcc41d; see the replies below.
🔴 Blocking: Allow the third sentinel to detect recovery after chain halts
A halted sentinel increments neither noRoutes nor inconclusive, so halted + noRoute or halted + halted skips SOL even though its answer could establish a verdict. This also blocks recovery: after a cached NEAR .down verdict, BTC halted and ETH halted leave that verdict unchanged even if SOL would route. refresh then suppresses every per-coin NEAR check, keeping unrelated NEAR-capable coins disabled until another verdict can be established or the old one expires. The Proposed ADR and testAHaltedSentinelIsNotStoodInFor explicitly encode this gate, but do not address that recovery failure. Allow the third probe when the first two have established neither up nor down; this remains within the three-probe limit and does not count a halt as evidence of an outage. Update the proposed decision and tests accordingly.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
There was a problem hiding this comment.
Resolved (re-reviewed at c7fcc41d): Your sentinel loop now asks SOL after two halted answers because neither contributes to the no-route count. The new testTwoHaltedSentinelsDoNotHideARecovery case covers the original recovery failure.
…hecks SDD-Published-From: ed9c59385523bdfdc167485480c5aced6e69ce52
romchornyi
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of c7fcc41 confirms five prior findings are fixed, but three blocking correctness issues remain in address rechecks, concurrent cache writes, and sentinel recovery; the availability-toast suggestion also remains valid. Current CI passes target-membership, accessibility, and PR-title checks, while simulator tests are pending; no local builds or tests were run. The supplied base drift is a non-overlapping CI-title change with no reported merge conflicts.
🔴 3 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Use availability-neutral toast copy for non-chain outages
DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinView.swift:78
This toast attributes every unavailable row to a halted chain, but SwapKitSwapProvider.haltedAssets now also includes .noRoute answers and the inferred provider-wide NEAR outage. A NEAR outage on Buy can disable every Buy coin without establishing that any underlying chain is halted. Use localized, availability-neutral wording for SwapKit, or distinguish provider unavailability from an actual chain halt if Maya should retain the chain-specific explanation.
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large diff directly changes coin selection and funds movement in AssetLockInputPolicy.swift and SwiftDashSDKTransactionSender.swift, including input limits, consolidation, and transaction building and signing. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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 `DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift`:
- [BLOCKING] DashWallet/Sources/Models/SwapKit/SwapKitSwapProvider.swift:407-412: Do not blame the address after a differently sized quote succeeds
The address quote sends sellAmount "1", but this recheck calls probeRoute, which normally sends 50 divided by the cached DASH price. It therefore changes both the destination and the amount. SwapKitErrorCopy documents that top-level noRoutesFound can be amount-dependent, and LiveRouteState.classify also maps explicit below-minimum replies to .available. Consequently, a nominal quote below a route's floor followed by a differently sized route or minimum-error response becomes validation_error in addressReCheckMessage, blocking a format-valid destination before the user can enter a sufficient amount. Compare requests at the same amount when isolating the destination, and distinguish an actual successful route from a below-minimum response before attributing failure to the address. Add coverage for an amount-dependent no-route followed by both kinds of response.
In `DashWallet/Sources/Models/SwapKit/SwapRouteAvailability.swift`:
- [BLOCKING] DashWallet/Sources/Models/SwapKit/SwapRouteAvailability.swift:435-440: Prevent an older in-flight probe from overwriting a newer recorded answer
The refresh lock serializes refreshes, but not record() or probeNow(). While this await is suspended, a newer address quote can record .available for the same direction/provider/asset. The older picker probe then unconditionally stores its negative result with a new checkedAt timestamp, replacing that successful observation and making the negative answer fresh for another ten minutes. Leaving the picker does not prevent this: in-flight answers are explicitly retained, and HTTPClient's async request does not cancel its underlying request when the task is cancelled. This can disable a working coin on return or exclude its working provider from subsequent quotes. Track a per-key revision or equivalent ordering and discard a probe result superseded by a newer conclusive observation. Add a regression that records availability while an older negative probe is held open.
- [BLOCKING] DashWallet/Sources/Models/SwapKit/SwapRouteAvailability.swift:389-393: Recheck negative sentinel entries when NEAR recovers
A down round stores ordinary ten-minute .noRoute entries for BTC and ETH as well as the two-minute NEAR-down verdict. If all routes recover at t=121, BTC returns .available here and ends the sentinel round, but ETH's outage-era negative entry remains. The per-coin job construction then skips ETH because isDue(key) still uses the ten-minute interval. On Buy, ETH therefore remains disabled until approximately t=600 despite the successful two-minute recovery check; SOL is similarly affected when it supplied a negative answer during the down round. On a down-to-up transition, make the outage-era negative sentinel entries due for independent rechecking rather than treating BTC's success as proof about the other coins. Extend the recovery test to display ETH and verify that it is reprobed and restored when its own response succeeds.
In `DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinView.swift`:
- [SUGGESTION] DashWallet/Sources/UI/Swap/SelectCoin/SelectCoinView.swift:78: Use availability-neutral toast copy for non-chain outages
This toast attributes every unavailable row to a halted chain, but SwapKitSwapProvider.haltedAssets now also includes .noRoute answers and the inferred provider-wide NEAR outage. A NEAR outage on Buy can disable every Buy coin without establishing that any underlying chain is halted. Use localized, availability-neutral wording for SwapKit, or distinguish provider unavailability from an actual chain halt if Maya should retain the chain-specific explanation.
| var reAsked: [LiveRouteState] = [] | ||
| for provider in [RouteProvider.near, .maya] where noRouteProviders.contains(provider) { | ||
| reAsked.append(await routeAvailability.probeNow(asset: toAsset, provider: provider, direction: .sell) { [weak self] asset, provider, direction in | ||
| guard let self else { return .unknown } | ||
| return await self.probeRoute(asset: asset, provider: provider, direction: direction) | ||
| }) |
There was a problem hiding this comment.
🔴 Blocking: Do not blame the address after a differently sized quote succeeds
The address quote sends sellAmount "1", but this recheck calls probeRoute, which normally sends 50 divided by the cached DASH price. It therefore changes both the destination and the amount. SwapKitErrorCopy documents that top-level noRoutesFound can be amount-dependent, and LiveRouteState.classify also maps explicit below-minimum replies to .available. Consequently, a nominal quote below a route's floor followed by a differently sized route or minimum-error response becomes validation_error in addressReCheckMessage, blocking a format-valid destination before the user can enter a sufficient amount. Compare requests at the same amount when isolating the destination, and distinguish an actual successful route from a below-minimum response before attributing failure to the address. Add coverage for an amount-dependent no-route followed by both kinds of response.
source: gpt-6.1-sol (phase2-reviewer: general)
| let state = await probe(asset, key.provider, key.direction) | ||
| if state == .unknown { | ||
| inconclusiveAt[key] = now() | ||
| } else { | ||
| inconclusiveAt[key] = nil | ||
| store(state, for: key) |
There was a problem hiding this comment.
🔴 Blocking: Prevent an older in-flight probe from overwriting a newer recorded answer
The refresh lock serializes refreshes, but not record() or probeNow(). While this await is suspended, a newer address quote can record .available for the same direction/provider/asset. The older picker probe then unconditionally stores its negative result with a new checkedAt timestamp, replacing that successful observation and making the negative answer fresh for another ten minutes. Leaving the picker does not prevent this: in-flight answers are explicitly retained, and HTTPClient's async request does not cancel its underlying request when the task is cancelled. This can disable a working coin on return or exclude its working provider from subsequent quotes. Track a per-key revision or equivalent ordering and discard a probe result superseded by a newer conclusive observation. Add a regression that records availability while an older negative probe is held open.
source: gpt-6.1-sol (phase2-reviewer: general)
| let state = await probeOne(Key(direction, .near, sentinel), asset: sentinel, force: true, probe: probe) | ||
| if state == .available { | ||
| inconclusiveRoundAt[direction] = nil | ||
| setNearVerdict(.up, direction: direction) | ||
| return .up |
There was a problem hiding this comment.
🔴 Blocking: Recheck negative sentinel entries when NEAR recovers
A down round stores ordinary ten-minute .noRoute entries for BTC and ETH as well as the two-minute NEAR-down verdict. If all routes recover at t=121, BTC returns .available here and ends the sentinel round, but ETH's outage-era negative entry remains. The per-coin job construction then skips ETH because isDue(key) still uses the ten-minute interval. On Buy, ETH therefore remains disabled until approximately t=600 despite the successful two-minute recovery check; SOL is similarly affected when it supplied a negative answer during the down round. On a down-to-up transition, make the outage-era negative sentinel entries due for independent rechecking rather than treating BTC's success as proof about the other coins. Extend the recovery test to display ETH and verify that it is reprobed and restored when its own response succeeds.
source: gpt-6.1-sol (phase2-reviewer: general)
Issue being fixed or feature implemented
The DEX coin picker decides which provider carries a coin, and whether the coin can be picked at all, from SwapKit's
/tokens?provider=lists. Those lists describe capability, not live state. On 2026-11-02 on mainnet:providers: ["NEAR"], every DASH ↔ X quote returnednoRoutesFound, so NEAR's DASH leg was down in both directions;routesandswapHalted(mayanode had BTC/ETH trading halted), and ARB.* returned 404noRoutesFound;/swapTostill listed all of these coins.The picker kept offering them, labelled "Multiple" or "NEAR". After you picked one (Sell DASH → USDT on Arbitrum), the address screen turned the 404
noRoutesFoundinto "Address validation unavailable — please check your connection". It also treated HTTP 200 with no route plusswapHaltedas a valid address.What was done?
Live route availability (
SwapRouteAvailability, new):/v3/quoteforced to one provider, at about $50 of the sold asset, so a top-levelnoRoutesFoundis conclusive. Each provider's ownproviderErrorsare classified separately:swapHalted-family codes mean halted,noRoutesFoundmeans no route, below-minimum counts as routable, and anything else (including an error without a code) is inconclusive.Picker (
SelectCoinViewModel,SwapKitSwapProvider):Address screen (
validateAddress):invalidDestinationAddress, a blacklisted or invalid asset) blocks with the address copy. Maya's two flavours are judged separately; when both refuse, the first refusal in SwapKit's order is shown.Note: SwapKit's
/v3/quotedoes not validate the destination address itself (a garbage address still gets a route). The format check on the address screen stays the app's ownisAddressValid.Decisions (Proposed, in
docs/adr/; summary indocs/features/swap-live-routes/README.md):Numbers 0003–0005 are taken by #1311; whichever PR merges second renumbers if needed.
How Has This Been Tested?
SwapRouteAvailabilityTestscover every NEAR/Maya combination: NEAR up with Maya halted → "NEAR"; NEAR down with Maya up → "Maya", and NEAR-only coins halted; both down → halted; halted sentinels; one Maya provider halted while the other is inconclusive; cache ageing and back-off; cancellation and concurrent refreshes.SwapKitQuoteDecodingTestscover the per-provider classification and the address-screen decision on real SwapKit error bodies.SwapRouteAvailabilityTests,SwapKitQuoteDecodingTests,SwapKitQuoteBoundaryTests,SwapKitErrorCopyTests,SwapAddressValidatorTests,SwapNotificationProducerTests,SwapStatusUIVisibilityTests,BuySwapOrderTests) plusDashAmountFormatterTests, 192 tests, pass on the iPhone 17 simulator (dashwallet-dashpay, Debug). SwiftLint reports no new findings on the changed Swift files.DashWalletTestsrun had 6 failures, none in swap code: 5 inInSheetPushStackTestsand 1 inString_DashWallet testAssumptionThatCurrencySymbolIsEitherAtTheBeginningOrAtTheEnd.sell_coin_picker_live_labels,buy_coin_picker_near_only), Debug simulator build, mainnet, fresh wallet (2026-11-05): Sell BTC "Multiple" and the rest "NEAR"; OKB, OP and POL shown as halted rows that do not open, with the halted-chain toast; every Buy row "NEAR".Breaking Changes
None.
SwapProvidergainsnetworkLabels(for:direction:),haltedAssets(from:pools:direction:), the refresh start/stop methods androuteAvailabilityChanges, and losesonBuyRoutabilityChanged. All conformers in this repo are updated.Checklist:
ADR-0006 added,ADR-0007 added,ADR-0008 addedFor repository code-owners and collaborators only
🤖 Generated with Claude Code