fix(swap): rank swap quotes by destination output across providers (#353)#356
fix(swap): rank swap quotes by destination output across providers (#353)#356
Conversation
Made-with: Cursor
📝 WalkthroughWalkthroughThe pull request replaces sequential first-success quote selection with concurrent parallel provider querying. All configured swap quote fetchers now execute simultaneously via Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Selection as Selection Logic
participant Kyber as Kyber Fetcher
participant OneInch as 1inch Fetcher
participant LiFi as LiFi Fetcher
Client->>Selection: Request swap quote
par Parallel Execution
Selection->>Kyber: Fetch quote
Selection->>OneInch: Fetch quote
Selection->>LiFi: Fetch quote
and
end
Kyber->>Selection: Return quote A (dstAmount: 1.5)
OneInch->>Selection: Return quote B (dstAmount: 1.8)
LiFi->>Selection: Return quote C (dstAmount: 1.6)
rect rgba(0, 200, 100, 0.5)
Selection->>Selection: Compute comparable outputs
Selection->>Selection: Select max output: B (1.8)
end
Selection->>Client: Return best quote B
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/core/chain/swap/quote/findSwapQuote.ts (1)
214-218: Non-winning quotes are currently discarded, limiting diagnostics.The winner-only return path drops alternative successful quotes and failure reasons. Consider surfacing ranked candidates (or emitting telemetry) for observability/debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/chain/swap/quote/findSwapQuote.ts` around lines 214 - 218, Currently findSwapQuote uses selectBestEligibleQuote(settled) and returns only the winning quote, dropping alternative successful quotes and failure reasons; update the function to surface ranked candidates for diagnostics by returning a richer result (e.g., { best, candidates: settled } or a ranked subset) or emit structured telemetry before returning, ensuring you include both the winner (selectBestEligibleQuote) and the full/filtered list of settled candidates along with their failure/reason metadata so downstream callers/observability can inspect alternatives and failure causes.packages/core/chain/swap/quote/findSwapQuote.selection.test.ts (2)
88-166: Add a regression case for fee-aware net-output ranking once comparator is updated.Current assertions validate raw output ranking only. A case where higher raw output loses after fee normalization would lock in the intended economic behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/chain/swap/quote/findSwapQuote.selection.test.ts` around lines 88 - 166, Add a regression test to verify fee-aware net-output ranking in findSwapQuote: create a case where one mocked provider (e.g., getKyberSwapQuote via minimalGeneralQuote) returns a higher raw expected_amount_out but with fees that reduce its net output below a later provider (e.g., getOneInchSwapQuote or getNativeSwapQuote), then call findSwapQuote and assert the comparator picks the provider with higher net output after fee normalization; reference the existing helpers minimalGeneralQuote/minimalNativeQuote and mock functions getKyberSwapQuote/getOneInchSwapQuote/getNativeSwapQuote/getLifiSwapQuote to craft the amounts and fee fields so the selection flips compared to raw-amount ranking.
168-185: All-failure assertion is a bit order-fragile.Line 184 hardcodes
'maya last error', which can break if native fetcher order changes. Prefer deriving/asserting against the effective last fetcher dynamically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/chain/swap/quote/findSwapQuote.selection.test.ts` around lines 168 - 185, The test hardcodes the final error string; instead derive the expected error from the last fetcher mock so the assertion isn't fragile. Modify the test that calls findSwapQuote to compute the expected message from the getNativeSwapQuote mock (or store the last mock's error message in a local variable) and assert rejects.toThrow(expectedMessage) rather than 'maya last error'; reference the mocks getKyberSwapQuote, getOneInchSwapQuote, getLifiSwapQuote and getNativeSwapQuote and the function under test findSwapQuote (use evmSameChainCoins as before) so the test adapts if fetcher order or implementation changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/chain/swap/quote/findSwapQuote.ts`:
- Around line 40-45: getComparableOutputAmount currently returns raw
expected_amount_out/dstAmount which lets ranking pick routes that look larger
but net less after fees; update getComparableOutputAmount (used in SwapQuote
ranking) to compute a normalized net output by subtracting all applicable fees
present on the quote (e.g., swap fees, outbound fees, gas/outbound gas costs or
any provider-specific deductions) and then normalize into a common unit (either
convert by token decimals to a base unit or to a fiat/native value if price info
is available) so comparisons use true economic net yield rather than gross
destination amount.
- Around line 204-214: The current Promise.allSettled over fetchers can hang if
a single fetcher never resolves; wrap each fetcher invocation in findSwapQuote
with a per-fetcher timeout using AbortController (or Promise.race) so the
fetcher is aborted after a configurable ms and returns/rejects promptly; ensure
the wrapper still returns the same shape ({ quote, outputAmount }) on success
and a rejected/timed-out outcome on failure so selectBestEligibleQuote receives
settled results and can ignore timed-out providers; update the fetcher mapping
where fetchers.map(fetcher => Promise.resolve().then(fetcher).then(...)) to use
the timeout wrapper and propagate useful error info for logging/diagnostics.
---
Nitpick comments:
In `@packages/core/chain/swap/quote/findSwapQuote.selection.test.ts`:
- Around line 88-166: Add a regression test to verify fee-aware net-output
ranking in findSwapQuote: create a case where one mocked provider (e.g.,
getKyberSwapQuote via minimalGeneralQuote) returns a higher raw
expected_amount_out but with fees that reduce its net output below a later
provider (e.g., getOneInchSwapQuote or getNativeSwapQuote), then call
findSwapQuote and assert the comparator picks the provider with higher net
output after fee normalization; reference the existing helpers
minimalGeneralQuote/minimalNativeQuote and mock functions
getKyberSwapQuote/getOneInchSwapQuote/getNativeSwapQuote/getLifiSwapQuote to
craft the amounts and fee fields so the selection flips compared to raw-amount
ranking.
- Around line 168-185: The test hardcodes the final error string; instead derive
the expected error from the last fetcher mock so the assertion isn't fragile.
Modify the test that calls findSwapQuote to compute the expected message from
the getNativeSwapQuote mock (or store the last mock's error message in a local
variable) and assert rejects.toThrow(expectedMessage) rather than 'maya last
error'; reference the mocks getKyberSwapQuote, getOneInchSwapQuote,
getLifiSwapQuote and getNativeSwapQuote and the function under test
findSwapQuote (use evmSameChainCoins as before) so the test adapts if fetcher
order or implementation changes.
In `@packages/core/chain/swap/quote/findSwapQuote.ts`:
- Around line 214-218: Currently findSwapQuote uses
selectBestEligibleQuote(settled) and returns only the winning quote, dropping
alternative successful quotes and failure reasons; update the function to
surface ranked candidates for diagnostics by returning a richer result (e.g., {
best, candidates: settled } or a ranked subset) or emit structured telemetry
before returning, ensuring you include both the winner (selectBestEligibleQuote)
and the full/filtered list of settled candidates along with their failure/reason
metadata so downstream callers/observability can inspect alternatives and
failure causes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4cec3ca-5fb5-46aa-9306-d9faaa8f8284
📒 Files selected for processing (3)
.changeset/rank-swap-quotes.mdpackages/core/chain/swap/quote/findSwapQuote.selection.test.tspackages/core/chain/swap/quote/findSwapQuote.ts
Conceptual review (orch2 sweep) — preferably-blockingser, jumped on this one as part of the non-vultiagent-app sweep. Right intuition (parallel-fetch + best-output-wins), but I think the current ranking is comparing apples-to-onions in a way that re-introduces #353 from a different angle. Preferably-blocking —
|
| Provider | Raw outputAmount returned by getComparableOutputAmount |
|---|---|
| THORChain (8-decimal canonical) | 100 × 1e8 = 10_000_000_000 |
| Kyber / 1inch / LI.FI (USDC has 6 decimals) | 100 × 1e6 = 100_000_000 |
THORChain's value is 100× bigger → THORChain wins every USDC-out swap regardless of actual net output. That's exactly the regression #353 was filed for ($7.88 fees on $10 USDC swap), just laundered through the new selector.
Inverse case for anything → ETH (18 decimals): Kyber's number is 1e10 larger than THORChain's for the same real output → Kyber/1inch/LI.FI wins everything, regardless of whether THORChain would actually be cheaper.
So the selector picks based on destination_token_decimals - 8, not based on net output. Probably want a normalize step in getComparableOutputAmount that re-bases native outputs against to.decimals before comparison — something like:
function getComparableOutputAmount(q: SwapQuote, toDecimals: number): bigint {
if ('native' in q.quote) {
const nativeDecimals = 8 // or getNativeSwapDecimals(toCoin)
const raw = BigInt(q.quote.native.expected_amount_out)
return toDecimals >= nativeDecimals
? raw * 10n ** BigInt(toDecimals - nativeDecimals)
: raw / 10n ** BigInt(nativeDecimals - toDecimals)
}
return BigInt(q.quote.general.dstAmount)
}(Plumb to.decimals through to selectBestEligibleQuote. The MAYA branches need extra care since they don't all return 8.)
Preferably-blocking — net vs gross output
Even with decimals normalized, expected_amount_out and dstAmount are both gross of source-chain gas (the EVM tx fee for general swaps, and outbound delay / asset fees subtracted differently for native). Two providers with identical outputAmount but radically different gas costs will tie under the current logic, and the tiebreaker (lower index = preferred) silently picks the gas-heavy one. Not fund-loss but the issue's framing was "net output" — current selector is gross output.
Probably out of scope for this PR (would need every fetcher to surface a comparable gas-in-destination-token), but worth a TODO comment and a follow-up issue, since #353 explicitly calls out the $7.88-on-$10 case which is fee-driven.
Suggestion — Promise.resolve().then(fetcher) indirection
fetchers.map(fetcher =>
Promise.resolve()
.then(fetcher)
.then(quote => ({ quote, outputAmount: getComparableOutputAmount(quote) }))
)The Promise.resolve().then(fetcher) extra microtask doesn't buy anything — fetcher() already returns a promise. fetchers.map(async fetcher => ({ quote: await fetcher(), outputAmount: ... })) reads cleaner and is one less microtask hop per provider.
Suggestion — error surfacing on all-rejected
const last = settled[settled.length - 1]
const error = last.status === 'rejected' ? last.reason : new Error('No quote')Picking settled[settled.length - 1] for the error to throw is non-deterministic re: which provider failed first / most informatively. With asyncFallbackChain the surfaced error was "the last attempt's error" (which carries the most context after the chain walked). With Promise.allSettled a more useful surface is to gather all rejection reasons into an aggregate (or at least pick the one with the most actionable message — 'dust threshold' should still propagate even if it's the second fetcher, otherwise the user sees a generic "no quote" when a friendly retry hint applies).
Concretely: if THORChain throws dust threshold and Kyber throws insufficient liquidity, current code throws Kyber's error and the user-facing "please increase amount" guard never fires.
Suggestion — index tiebreak is stable but the order is non-obvious
outputAmount === bestAmount && i < bestIndexThe earlier-index-wins tiebreak preserves the prior shouldPreferGeneralSwap ordering when amounts tie. That's correct intent but worth a code comment — without one it reads as "first fulfilled wins on tie" when it's actually "earlier in the shouldPreferGeneralSwap-ordered list wins". Future readers will tweak this.
Verified clean
- Test coverage has the right shape — picks-later-when-better, prefers-earlier-on-tie, falls-back-on-rejection, etc. Once the decimal normalization lands, the existing tests will need destination-decimal context added (the current minimal-quote helpers don't model that).
- No fund-safety surface beyond the ranking itself — no signing change, no calldata mutation, no chain-id surface.
changeset/rank-swap-quotes.mdpresent, scoped to@vultisig/core-chain. ✓
Verdict
Preferably-blocking on #1 (decimal normalization — recreates #353 in a different shape). #2 (net vs gross) is a follow-up if y'all are scoping tightly. The selector skeleton is right; the comparator function needs unit-aligning.
Happy to pair on the normalization shape if useful.
🤖 conceptual review via Claude Code (orch2 sweep — sdk batch, no runtime testing this pass)
2 participants
Closes #353
Summary by CodeRabbit
Bug Fixes
Tests