Repository navigation
fix: compute route slippagePct against a true spot price - #197
Conversation
|
@funmilayo-ui Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
Thanks — the diagnosis is right and the fix is most of the way there. slippagePct really was the execution price compared with itself, and calculateAMMSpotPrice(rA, rB) is the correct reference with the correct units (B per A, matching output / amount). The AMM-route path in this PR is good. Two things need to change before it goes in.
1. src/aggregator/bestRoute.ts:121-130 — the reference is the AMM's spot even when the route is SDEX
spotPrice = amm.spotPrice is used unconditionally, so when route === 'SDEX' the reported slippage is an SDEX execution price measured against the AMM pool's reserve ratio. That is a cross-venue spread, not slippage, and it is attributed to a trade that never touches the pool.
I ran this against your branch with the test harness from bestRoute.test.ts:
- Pool 1,000,000 A / 500,000 B (spot 0.5), 30 bp, amount 1000, SDEX fills at 0.4985 →
route: SDEX, slippagePct: 0.30. That 0.30 is the AMM's fee, reported as slippage on an SDEX fill. - Pool 1,000 A / 600 B (a thin or stale pool, spot 0.6), amount 1000, SDEX fills at its quoted 0.5 →
route: SDEX, slippagePct: 16.67. The SDEX trade has no slippage of its own here; the whole 16.67% is an artifact of the other venue.
Because of the Math.max(0, …) clamp the field is either 0 (when SDEX ≥ AMM spot) or a number that came from the wrong venue. Neither is slippage, and a wrong slippage number is a wrong trade. The PR's own does not change which route is selected case only exercises the clamped-to-0 side, which is why this isn't visible in the suite.
The fix is small and matches what you already do elsewhere: only report slippage against the spot of the venue actually being executed on. route === 'AMM' → amm.spotPrice (exactly what you have). route === 'SDEX' or 'SPLIT' → there is no size-independent reference available, so report 0, the same honest answer you already give when there's no pool at all. A test asserting slippagePct === 0 for an SDEX route with a live pool present would lock it in.
2. src/aggregator/bestRoute.ts:12-14 — the comment is not true any more, and the query it excuses is now load-bearing
// AMM pricing reads price_points/pool_snapshots, which have no network column yet
Both tables have had one for a while: prisma/schema.prisma has network String @default("testnet") on PricePoint (line 14) and PoolSnapshot (line 35), with @@index([network, …]) on each. getAMMPrice's query filters on neither, so a mainnet request to /price/:a/:b/route can take its pool row from testnet — while getSDEXPrice is correctly per-network via the network argument.
Before this PR that mis-scoping only reached ammPrice. This PR makes the same unscoped row drive slippagePct too, so it's worth closing here: thread network into getAMMPrice and add WHERE ps.network = $2 plus AND network = $2 on the inner price_points select. #195 just landed the same predicates on /screener if you want a reference.
Heads-up on a collision: #185 rewrites getAMMPrice to filter both legs of that pool lookup. Whichever lands second will need a rebase — if you'd rather keep this PR to the slippage fix and let #185 own the network predicate, say so and just correct the stale comment here. Point 1 still needs fixing either way.
7497607 to
b0f6147
Compare
|
Thanks for the careful review. Both points are addressed in b0f6147.
|
Miracle656
left a comment
There was a problem hiding this comment.
Both points closed, and point 1 is fixed in exactly the right place.
spotPrice = route === 'AMM' ? amm.spotPrice : 0 is the whole fix: slippage is now only measured against the spot of the venue being executed on. The two cases from the review are both covered by tests, with the numbers I quoted — does not report the AMM fee as slippage on an SDEX fill (the 0.30 case) and is 0 for an SDEX route even when the AMM spot is far above the fill (the thin-pool 16.67 case) — plus SPLIT, which I had not asked for and which has the same problem.
I re-derived the curve against your fixture to check the assertions are not just self-consistent:
pool 1,000,000 A / 500,000 B, 30 bp, spot 0.5
amount 1: exec 0.498500 slippage 0.3001% <- the fee alone
amount 10,000: exec 0.493579 slippage 1.2842%
amount 100,000: exec 0.453305 slippage 9.3389%
amount 500,000: exec 0.332666 slippage 33.4668% <- 'roughly a third'
So toBeGreaterThan(0.29) / toBeLessThan(0.31) and toBeGreaterThan(30) are both pinned to the real curve, and monotonic growth with size is now a property rather than a hope.
Point 2 resolved itself the way I offered — the network predicate is on main now and this rebased onto it, so getAMMPrice takes network and the query filters both the outer scan and the subquery. The stale comment went with it.
Two things I noticed that you did not have to do and were right to:
- the property test no longer asserts
slippagePct ≈ 0unconditionally. That assertion was what let the original bug live: a field that is always zero passes a test that says it is always zero. It now checks the exact curve relationship on the AMM-only path and zero elsewhere. recommendationdropped a dead ternary. Inside thediff < 0.001branch,diff < 0.001 ? … : …could only ever take the first arm, so 'SPLIT may reduce slippage for large orders' was unreachable.
CI green. Merging.
closes #162
Summary
slippagePcton/price/:a/:b/routewas always 0: every branch setestimatedOutputtomax(sdex, amm) * amount, and slippage compared that to the samemax(...).Changes
getAMMPricenow also returns the reserve-ratio spot price (calculateAMMSpotPrice, reused frompricing/depth.ts, same convention as/depth). It keeps the per-network filters from Lens's aggregation layer now reads per network. Every function in src/aggregator/vwap.ts takes a required network and filters price_points and pool_snapshots on it, getAMMPrice filters both legs of its pool lookup, /price/:assetA/:assetB passes req.network all the way through to the aggregator, and the aggregate refresh worker runs once per enabled network instead of pinning itself to whichever network the instance happens to be indexing. #185.slippagePct = max(0, (spot - executionPrice) / spot * 100)against that pool's spot. Route selection andestimatedOutputare unchanged.Caveats
/depth).Verification
npx tsc --noEmit: clean.npx vitest run src/__tests__/bestRoute.test.ts: 15/15 pass; the 3 SDEX/SPLIT tests fail if the AMM spot is used unconditionally (verified).tests/aggregator.property.test.ts: passes at the full run count when run alone (~22s). In a full parallelvitest runit hit its 30s timeout under load; all other tests passed.