Skip to content

fix: compute route slippagePct against a true spot price - #197

Merged
Miracle656 merged 1 commit into
Miracle656:mainfrom
funmilayo-ui:fix/slippage-pct-true-spot
Oct 4, 2026
Merged

Miracle656 merged 1 commit into
Miracle656:mainfrom
funmilayo-ui:fix/slippage-pct-true-spot

Conversation

@funmilayo-ui

@funmilayo-ui funmilayo-ui commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

closes #162

Summary

slippagePct on /price/:a/:b/route was always 0: every branch set estimatedOutput to max(sdex, amm) * amount, and slippage compared that to the same max(...).

Changes

Caveats

  • With no AMM pool, or on an SDEX/SPLIT route, slippage is 0 rather than a guess. An execution price above spot is clamped to 0.
  • The AMM figure includes the pool fee (consistent with /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 parallel vitest run it hit its 30s timeout under load; all other tests passed.

@drips-wave

drips-wave Bot commented Sep 30, 2026

Copy link
Copy Markdown

@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! 🚀

Learn more about application limits

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@funmilayo-ui
funmilayo-ui force-pushed the fix/slippage-pct-true-spot branch from 7497607 to b0f6147 Compare October 3, 2026 20:40
@funmilayo-ui

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review. Both points are addressed in b0f6147.

  1. Slippage is now measured only against the spot of the venue being executed on: route === 'AMM' uses the pool's reserve-ratio spot; SDEX and SPLIT report 0. I added tests for both of your scenarios (thin pool with spot 0.6 vs SDEX fill 0.5, and the 30 bp fee case) plus a SPLIT case. All three fail if the AMM spot is used unconditionally.
  2. 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 has merged, so getAMMPrice already filters both legs by network and the stale comment is gone. I rebased onto main, kept that version, and added the spot return on top.

tsc is clean and bestRoute.test.ts passes 15/15. The property test passes on its own at full runs; in a full parallel run it can hit its 30s timeout under load.

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ≈ 0 unconditionally. 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.
  • recommendation dropped a dead ternary. Inside the diff < 0.001 branch, diff < 0.001 ? … : … could only ever take the first arm, so 'SPLIT may reduce slippage for large orders' was unreachable.

CI green. Merging.

@Miracle656
Miracle656 merged commit d53ac14 into Miracle656:main Oct 4, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

slippagePct on /price/:a/:b/route is always exactly zero

2 participants