Repository navigation
Every list route in the API — /pools, /pairs, /candles and /prices/history — now pages with the same cursor and limit parameters and the same opaque cursor shape that /screener already used, backed by keyset pagination so rows inserted while a client pages can never shift a page boundary, with all defaults and maxima documented per route in openapi.yaml and the behaviour locked down by tests including one that inserts a row mid-page. - #429
DevScoopee wants to merge 6 commits into
Conversation
|
@DevScoopee 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! 🚀 |
…story — now pages with the same cursor and limit parameters and the same opaque cursor shape that /screener already used, backed by keyset pagination so rows inserted while a client pages can never shift a page boundary, with all defaults and maxima documented per route in openapi.yaml and the behaviour locked down by tests including one that inserts a row mid-page. Problem. /pools, /pairs, /candles and /prices/history returned everything they had in one response. That is tolerable today, but each venue added — Phoenix, Blend and DeFindex are queued — grows those responses without bound, and the first casualty will be a mobile client timing out on /pools. Offset pagination would not have been a fix either, because a row inserted while a client pages shifts every subsequent page and produces duplicates and skipped rows. What changed. The cursor-and-limit convention already proven on /screener has been extracted into a shared helper (src/pagination.ts, providing parseLimit, encodeCursor/decodeCursor and slicePage) and applied to every list route rather than reinvented per route. Every route now accepts the same two parameters: an opaque cursor, which is a base64 JSON [sortValue, id] tuple passed through unchanged from the previous page's nextCursor, and a limit, which is clamped into [1, max] and rejected with a 400 if it is not a valid integer. The cursor shape is identical everywhere, so a client can write a single paging loop for the whole API. Pagination is keyset-based, not offset-based: each route filters strictly past the cursor's sort value, so a row inserted mid-page cannot shift a page boundary. Handlers fetch limit + 1 rows to compute hasMore, so no COUNT(*) over the whole table is needed. Specifically, /pools keysets on pool_id with a default limit of 50 and a maximum of 200, /pairs on pairKey with the same 50/200, /candles on the bucket start time with 500/5000, and /prices/history on the snapshot timestamp with 1000/10000, reusing MAX_HISTORY_POINTS as the ceiling. When /prices/history is called with format=csv, the byte-exact CSV body stays untouched and the pagination envelope travels in the X-Next-Cursor and X-Has-More response headers instead. Closes Divinity-Labs#263
Miracle656
left a comment
There was a problem hiding this comment.
Same root cause as #427, different files: the Merge branch 'main' into Only-/screener-can-be-paged commit (bc41a2f) kept calls and dropped the imports behind them, and in one place kept both sides of a declaration.
src/api/rest.ts(112,16): error TS2304: Cannot find name 'listCrashes'.
src/api/rest.ts(358,28): error TS2304: Cannot find name 'getImpact'.
src/routes/screener.ts(6,3): error TS2440: Import declaration conflicts with local declaration of 'SCREENER_DEFAULT_LIMIT'.
src/routes/screener.ts(7,3): error TS2440: ... 'SCREENER_MAX_LIMIT'.
The screener one is worth looking at closely: SCREENER_DEFAULT_LIMIT and SCREENER_MAX_LIMIT are now imported and also declared locally in the same file. That is the shape a merge leaves when both sides defined the same constant and neither was chosen. TypeScript rejects it outright here, but the near-miss version of that bug — where one declaration silently shadows the other — is how a pagination limit ends up being quietly different from the one the spec documents.
Suite on this branch: 9 files failing, 6 tests failing, against 1,161 passing. The failures are the network-scoped status and indexer-state tests plus the adaptive-impact schema test, all downstream of rest.ts not compiling.
The feature itself
Keyset pagination is the right choice and the reason you give for it is the right reason: rows inserted while a client pages cannot shift a page boundary, which offset pagination cannot promise. Giving /pools, /pairs, /candles and /prices/history the same cursor shape /screener already used is better than four slightly different paginations, and a test that inserts a row mid-page is the test that actually proves the property rather than restating it.
What to do
Rebase on current main instead of merging it again — you are 29 commits behind, and a rebase surfaces a dropped import as a conflict rather than deleting it quietly. Then run npx tsc --noEmit and npx vitest run locally; tsc names all four problems in one command.
Worth knowing: #427 has the identical failure mode from its own merge commit, and main moved a lot today — seven PRs landed, including #542 which restored the @rolldown Linux binding that CI needs. Rebase rather than merge for both.
Miracle656
left a comment
There was a problem hiding this comment.
Thanks for the rework — src/pagination.ts is a good module and I want it in. I took it all the way through a local merge and the full suite, and it stops on two regressions against guarantees that landed while you were working. Both are small to fix; neither is a design problem.
src/pagination.ts is the right abstraction
Worth saying first, because the rest of this is problems. The module documents the convention rather than just implementing it, and the three-way DecodedCursor return — payload, 'skip', null — is exactly the distinction that matters: a degenerate cursor we encoded ourselves should serve the first page, and only genuinely malformed input should be a 400. Fetching limit + 1 as the sole hasMore signal, with no COUNT(*), is the right call on a table this size. Keeping each route's own response key (pools, pairs, candles, points) instead of normalising everything to data keeps it backward-compatible.
1. Blocking: the half-open window became inclusive
Both routes changed the upper bound from exclusive to inclusive:
-- src/api/history.ts:115 (main: AND ts < $4)
AND ts <= $4${cursorCondition}
-- src/routes/candles.ts:108 (main: AND timestamp < $4)
AND timestamp <= $4${cursorCondition}That reintroduces the bug #584 fixed. Six tests catch it:
src/__tests__/windowBoundaries.test.ts
× adjacent candle windows share no boundary > counts a sample landing exactly on the boundary exactly once
expected +0 to be 1
× adjacent candle windows ... > excludes the boundary sample from the earlier window and includes it in the later one
expected [] to include 1767229200000
× adjacent price history windows ... > counts a sample landing exactly on the boundary exactly once
expected 2 to be 1
× adjacent price history windows ... > excludes the boundary sample from the earlier window and includes it in the later one
expected [ 1767225660000, 1767229200000 ] to not include 1767229200000
src/__tests__/history.test.ts
× uses half-open interval [from, to) in SQL query
expected SQL to match /AND ts < \$4/
The consequence is worth spelling out: with an inclusive upper bound, a sample landing exactly on T1 belongs to both [T0, T1] and [T1, T2]. A client walking a day in hourly windows double-counts one sample per boundary — twenty-four times over a day, in volume and in the OHLC close. The /candles case is worse: the boundary sample ends up in neither window, so it is silently dropped.
I suspect this came from needing the boundary row available to the cursor predicate. If so, the fix is to leave the window half-open and let the cursor do the work — the keyset condition already excludes rows the client has seen, so it does not need the window to be inclusive. expect(sql).not.toMatch(/AND ts <= \$4/) in history.test.ts is there precisely to stop this being changed back.
2. Blocking: /pools lost the decimal-string negotiation
tests/decimalStrings.test.ts
× serializes decimal quantities as strings on /pools and /price/:a/:b/route
expect(poolsRes.headers['x-decimal-format']).toBe('string')
AssertionError: expected undefined to be 'string'
src/api/rest.ts has 9 formatDecimal call sites after the merge; main has 12. The /pools rewrite for pagination dropped the formatting and the X-Decimal-Format response header, so a client asking for exact decimals (?decimals=string, X-Decimal-Format: string) silently gets rounded doubles back from that route. That is #248's guarantee, and prices are the one thing in this API that cannot be approximately right.
3. The merge produced code that does not compile, with only one conflict reported
git merge origin/main reports a single conflict (src/routes/screener.ts) and hands back a tree tsc rejects, because git merges line regions rather than declarations:
src/api/rest.ts— your import line replaced main's, sogetImpact,listCrashes,isBootComplete,resolveDecimalFormatandformatDecimalwere dropped while every call to them stayed. EightTS2304: Cannot find nameerrors.src/routes/candles.ts—INTERVAL_SECONDSdeclared twice, andactiveNetwork,NetworkNameand the../middleware/networkside-effect import all lost.
This is the third PR of yours I have seen do this (it also affects #427 and #437), and the cause is the same each time: merging main in through the web conflict editor, which keeps both sides. Rebase instead of merging, and run npx tsc --noEmit immediately afterwards. A clean merge is not evidence of a working tree.
4. It collides with #609, which landed an hour ago
#609 extracted the screener query into src/services/screener.ts so REST and the new GraphQL screener query share one implementation — including its own base64 cursor codec. Your src/pagination.ts is a second one.
The resolution I would accept, and the one I had working locally: src/pagination.ts owns the codec and the page limits, and src/services/screener.ts imports encodeCursor, decodeCursor, slicePage, SCREENER_DEFAULT_LIMIT and SCREENER_MAX_LIMIT from it rather than defining its own. src/routes/screener.ts then stays as main has it, calling runScreener. That gives one cursor shape across /pools, /pairs, /candles, /prices/history, /screener and GraphQL, which is what both PRs were reaching for.
Your decodeCursor's 'skip' vs null distinction maps cleanly onto the service's existing ScreenerCursorError: null throws it, 'skip' serves the first page.
5. Two smaller things
A name collision with two different values. src/api/rest.ts exports HISTORY_DEFAULT_LIMIT = 100 for the per-pair /price/:assetA/:assetB/history; src/pagination.ts exports HISTORY_DEFAULT_LIMIT = 1_000 for the global /prices/history. Two routes with different defaults is fine, one name for both is not — whoever imports it next gets whichever module they happened to reach for, with a 10× difference. I renamed the new one to PRICES_HISTORY_DEFAULT_LIMIT locally; something like that.
/prices/history gains a default page size where it had none. On main that route has no limit handling at all. After this it defaults to 1,000 rows plus hasMore, so an existing client that does not read hasMore silently stops seeing part of its window. That is the unavoidable cost of paginating an unpaginated route and I think it is the right trade, but it should be called out as breaking in the changeset and in the openapi.yaml description, not just documented as a default.
To summarise what to do
- Rebase on
main(not merge), thennpx tsc --noEmit. - Restore
< $4/< $3in both routes; let the cursor exclude seen rows. - Restore
/pools'sformatDecimalcalls and theX-Decimal-Formatheader. - Point
src/services/screener.tsatsrc/pagination.tsand drop its local codec. - Rename the colliding constant.
- Note the
/prices/historydefault as breaking. openapi.jsonandclients/typescript/src/schema.d.tsare byte-compared in CI:npm run openapi:gen, thennpx --yes openapi-typescript@7.13.0 openapi.yaml -o clients/typescript/src/schema.d.ts. Do not hand-edit the JSON.
With those, 1,586 tests should be green — everything else in the PR already passes, including your own 253-line pagination suite and the mid-page insertion test, which is the hard case and which works.
Problem. /pools, /pairs, /candles and /prices/history returned everything they had in one response. That is tolerable today, but each venue added — Phoenix, Blend and DeFindex are queued — grows those responses without bound, and the first casualty will be a mobile client timing out on /pools. Offset pagination would not have been a fix either, because a row inserted while a client pages shifts every subsequent page and produces duplicates and skipped rows. What changed. The cursor-and-limit convention already proven on /screener has been extracted into a shared helper (src/pagination.ts, providing parseLimit, encodeCursor/decodeCursor and slicePage) and applied to every list route rather than reinvented per route. Every route now accepts the same two parameters: an opaque cursor, which is a base64 JSON [sortValue, id] tuple passed through unchanged from the previous page's nextCursor, and a limit, which is clamped into [1, max] and rejected with a 400 if it is not a valid integer. The cursor shape is identical everywhere, so a client can write a single paging loop for the whole API. Pagination is keyset-based, not offset-based: each route filters strictly past the cursor's sort value, so a row inserted mid-page cannot shift a page boundary. Handlers fetch limit + 1 rows to compute hasMore, so no COUNT(*) over the whole table is needed. Specifically, /pools keysets on pool_id with a default limit of 50 and a maximum of 200, /pairs on pairKey with the same 50/200, /candles on the bucket start time with 500/5000, and /prices/history on the snapshot timestamp with 1000/10000, reusing MAX_HISTORY_POINTS as the ceiling. When /prices/history is called with format=csv, the byte-exact CSV body stays untouched and the pagination envelope travels in the X-Next-Cursor and X-Has-More response headers instead.
Closes #263