feat: add GET /spreads/:asset route to calculate bid-ask spreads for … - #95
Conversation
|
@Johnsource-hub 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 for tackling #93 — the route skeleton is a good start, but a few things need fixing before it can merge:
1. The bid/ask are inverted (correctness bug). Right now bid = MAX(price) and ask = MIN(price), so bid ≥ ask always, which makes spreadBps = ((ask - bid) / ask) * 10000 always negative. A bid-ask spread needs ask ≥ bid. Also, taking max/min over all historical trade prices isn't a bid-ask spread — that's a price range. A real spread needs quote/order-book data (best bid vs best ask) or, if you only have trade points, it should be defined and named accordingly. Please rework the calculation so it returns a meaningful, non-negative spread.
2. It isn't actually registered. src/index.ts wires up routes via registerRESTRoutes/registerPriceRoutes/registerCandleRoutes/etc., but there's no registerSpreadsRoutes(app) call (and no import). As-is the endpoint is unreachable — please add the import + await registerSpreadsRoutes(app) alongside the others.
3. Check the SQL column names. Unquoted assetA = $1 OR assetB = $1 folds to asseta/assetb in Postgres and won't match the actual columns. Look at how the other raw queries reference price_points and match that exactly (quote the identifier if the column is camelCase).
4. Add the test. The checklist notes tests are still to be added — please include at least one covering a normal spread and the empty/no-data case.
Once the spread math is correct, the route is registered, and there's a test, this is good to go. 👍
|
everything done |
|
Thanks for the update — the bid/ask sign is fixed now (
Also note it's still computing max−min over all price points, which is a price range rather than a true bid-ask spread — fine if that's the intent, but worth documenting it as such. Once it's registered and tested I'll re-review. |
Miracle656
left a comment
There was a problem hiding this comment.
Sorry this has sat so long. Re-reviewed against current main — the two blockers from before are both still open, and one is a correctness bug rather than a style point.
1. The route is never registered
The PR adds src/routes/spreads.ts exporting registerSpreadsRoutes, and nothing calls it. registerSpreadsRoutes appears nowhere in src/index.ts or src/api/*, so GET /spreads/:asset returns 404 — the code cannot run as shipped.
2. The query mixes price directions, so the numbers are not comparable
PricePoint stores a directional price: assetA, assetB, and one price meaning A denominated in B. This filter takes both sides:
WHERE "asset_a" = $1 OR "asset_b" = $1So for XLM it pools rows where price ≈ 0.2 (XLM priced in USDC) with rows where price ≈ 5 (USDC priced in XLM). Those are reciprocals of each other, and MIN/MAX across the mixture produces a "spread" in the thousands of bps that reflects nothing real.
Fixing it means either constraining to one direction (asset_a = $1), or inverting the price on rows where the asset is the counter before aggregating.
3. MIN/MAX is a price range, not a bid/ask
Even with direction fixed, MIN(price) and MAX(price) over the whole table are the lowest and highest prices ever recorded — a historical range. A bid/ask spread is the best buy and best sell available right now, from the order book. Two consequences:
- The value drifts permanently wider as history accumulates, and never recovers.
- There is no time bound at all, so a price from months ago sets today's "bid".
If the intent is an order-book spread, it needs SDEX offer data rather than price_points. If the intent is observed price dispersion over a window, that is a legitimate and useful endpoint — but it should say so, take a window parameter, and not be called bid/ask.
Also
No test accompanies the route, and spreadBps divides by ask without guarding the bid > ask case that the direction mixing can produce.
Happy to help scope this if you would rather narrow it to the dispersion version — that is a smaller, honest endpoint and it would land.
… wire the route up The route existed but was never registered in src/index.ts, so GET /spreads/:asset returned 404 in every deployment — the code was unreachable. It is now registered alongside /volumes. Reworked to meet the acceptance criteria in Miracle656#93, which asked for a spread per venue and the tightest aggregated. The previous query took MIN and MAX of price across the whole table with no grouping, no time bound and no network filter, which is a lifetime price range rather than a spread: - Grouped by (source, pair_key). One venue quoting XLM/USDC and XLM/EURC has two books at different price scales; collapsing them takes the min and max across unrelated scales and calls the gap a spread. - Time-bounded, default 5m. Widen the window far enough and you stop measuring liquidity and start measuring the day's price drift. - Filtered by network. Prices from two chains are not comparable, and pooling them does not give a wide spread, it gives a meaningless one. Now that the ingesters run per network (Miracle656#117) this was a live correctness bug. - Basis points against the mid, not the ask. Dividing by the ask makes the number depend on which side you divided by, so two venues quoting the same absolute range score differently. - Venues with a single observation are dropped: a lone print has a zero range by construction and would beat every liquid venue for 'tightest'. - tightest is null, not a zeroed object, when nothing quoted the asset — 'nobody quoted it' and 'the spread is zero' must not look alike. The doc comment states plainly that Lens stores one price per observation, so this measures print dispersion rather than a top-of-book gap. It is a real liquidity signal but not the number a venue's own book would report. 11 tests; full suite 388 passed / 1 skipped.
Miracle656
left a comment
There was a problem hiding this comment.
Approved and merging — I reworked this on your branch (a9ccf91) rather than leave it open, since the wave has closed.
Credit where it's due first: you caught the bid/ask inversion yourself in the second commit and fixed the column identifiers to the real asset_a / asset_b. That was the sharpest issue in my original review and you found it before I re-read it.
The blocker was that the route was never registered. registerSpreadsRoutes was exported and never called from src/index.ts, so GET /spreads/:asset returned 404 in every deployment. It's an easy thing to miss because everything about the file looks finished — nothing fails, no test goes red, the code just never runs. It's wired up next to /volumes now.
What I changed, and why each one mattered:
-
Grouped by
(source, pair_key). #93 asked for a spread per venue with the tightest aggregated; the query had noGROUP BYat all. It also has to be per pair, not just per venue — one venue quoting XLM/USDC and XLM/EURC has two books at different price scales, and taking min/max across both reports the gap between unrelated scales as a spread. Not imprecise; meaningless. -
Time-bounded, default 5m.
MIN/MAXover the whole table is a lifetime price range, not a spread. Widen the window far enough and you stop measuring liquidity and start measuring the day's price drift. -
Filtered by
network. This one became a live bug rather than a theoretical one when #155 landed and the ingesters started running per network. Testnet and mainnet prices in one aggregate don't give a wide spread — they give a number with no meaning. Added as a query param defaulting to the active network, rejecting anything else instead of silently falling back. -
Basis points against the mid, not the ask. Yours used
(ask - bid) / ask. Dividing by one side makes the figure depend on which side you picked, so two venues quoting the same absolute range score differently. The mid is symmetric. There's a test pinning bid 99 / ask 101 to exactly 200 bps — it's 198.02 against the ask. -
Venues with a single observation are dropped. This is the subtle one. A lone print has a zero range by construction, so it reports a perfect 0 bps spread and beats every genuinely liquid venue for "tightest" — the endpoint would confidently point you at the emptiest venue on the network. One observation is not evidence of liquidity.
-
tightestisnull, not a zeroed object, when nothing quoted the asset. "Nobody quoted it" and "the spread is zero" must not look alike; the second reads as free to trade.
I also wrote the doc comment to say plainly what these numbers are: Lens stores one price per observation, not a quoted bid and ask, so this measures the dispersion of a venue's recent prints rather than a top-of-book gap. That's a real liquidity signal, but it isn't the number the venue's own order book would report, and an endpoint that blurs the two would mislead someone sizing a trade.
11 tests added. Full suite 388 passed / 1 skipped, tsc --noEmit clean.
One thing your PR surfaced that isn't yours to fix: /volumes/:asset has the same missing network filter and is aggregating across both chains today. Filing that separately.
Six zero-byte files (because, decisions, it, that, to, and one with a mojibake name) were swept into the #95 merge by a 'git add -A' after a shell-quoting mistake on my side created them in the repo root. They were never referenced by anything.
#closes
#93
New route added and registered.
Basic error handling implemented.
Documentation and inline comments included.
Tests (to be added) cover functionality.
No new dependencies introduced.