Skip to content

fix: standardise range queries to half-open intervals [from, to) (#236) - #584

Merged
Miracle656 merged 1 commit into
Divinity-Labs:mainfrom
Nwapu-TrustJah:fix/window-boundaries-236
Oct 23, 2026
Merged

Miracle656 merged 1 commit into
Divinity-Labs:mainfrom
Nwapu-TrustJah:fix/window-boundaries-236

Conversation

@Nwapu-TrustJah

Copy link
Copy Markdown
Contributor

Summary

Standardized all range queries across the codebase from inclusive upper bounds (<=) to half-open intervals [from, to) (<) to prevent double-counting samples on shared boundaries. Updated OpenAPI specifications and added a dedicated adjacent-window test suite.

Related issue

Closes #236

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Docs
  • Tests

@drips-wave

drips-wave Bot commented Oct 20, 2026

Copy link
Copy Markdown

@Nwapu-TrustJah 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 added a commit that referenced this pull request Oct 22, 2026
  tsc --noEmit   clean
  vitest         106 files, 1232 passing (up from 102 / 1157)
  dropped-tests  none

Four of the eight screened in this group. The other four were held, and the
reason is worth stating because it keeps recurring: each one merges cleanly
and fails on its own.

  #590  72 type errors alone, including syntax errors in src/api/rest.ts and
        src/api/schemas.ts -- "Declaration or statement expected",
        "Expression expected". Not type mismatches; the file does not parse.
  #584  3 type errors alone
  #592  conflicts
  #595  conflicts

Merged together before screening, that group produced 68 failing test files
and syntax errors I first mistook for a bad three-way merge. They were not:
#590 carries them on its own branch. Screening each PR individually before
batching costs one typecheck per PR and saves an hour of bisection, which is
the lesson of the day.

Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Three type errors on its own branch against current main — npx tsc --noEmit will show them after a rebase.

Standardising range queries to half-open intervals [from, to) is the right call and worth doing properly: with closed intervals, consecutive windows double-count the boundary row, which quietly inflates every volume and count that straddles it. That is the kind of error nobody notices because the number still looks plausible.

Worth adding a test that pins exactly that, if there is not one already: two adjacent windows over a row that sits on the boundary should total the same as one window covering both.

@Nwapu-TrustJah

Copy link
Copy Markdown
Contributor Author

​I have completed all the requested changes:
​Rebased onto main and fixed the TypeScript compilation errors (npx tsc --noEmit passes clean).
​Standardized range queries to half-open intervals [from, to) across all endpoints.
​Added unit tests (windowBoundaries.test.ts) confirming that adjacent windows equal a single combined window without boundary double-counting. All tests are passing.
​Ready for re-review whenever you get a chance!

@Miracle656
Miracle656 merged commit 431bb87 into Divinity-Labs:main Oct 23, 2026
@Miracle656

Copy link
Copy Markdown
Collaborator

Merged to main as 431bb87. The three type errors are gone, and you added exactly the test I asked for.

src/__tests__/windowBoundaries.test.ts is what made this mergeable rather than merely plausible — "counts a sample landing exactly on the boundary exactly once" and "excludes the boundary sample from the earlier window and includes it in the later one", asserted for both routes. That pins the invariant rather than restating the change, which is the difference between a test that proves a fix and one that documents it.

The network predicate is the bigger fix and it isn't in your title. The changeset is where I found it: /candles and /basket were reading rows from both chains, and /benchmark was computing its deviation figures from both. A pair's price being a blend of mainnet and testnet is worse than the boundary double-count, and far harder to notice. Worth putting that in the PR title next time — I nearly reviewed this as a one-line interval change. The comments explaining that price_points indexes network as the leading column of (network, pair_key, timestamp), so the predicate is needed for the index as well as for correctness, are the right thing to leave behind.

The breaking-change note is also well written: saying plainly that callers relying on the inclusive end must widen to by one tick, with a worked example at interval=1m, is what a consumer actually needs.

One follow-up, deliberately out of scope here. Two closed-interval predicates remain:

  • src/api/graphql.ts:338 — bucket <= $N
  • src/routes/movers.ts:89 — timestamp <= $3

So GraphQL priceHistory and /movers now disagree with the REST routes about the same data. graphqlPriceHistory.test.ts:216 pins the closed form, so changing it is a deliberate decision that needs its own breaking note rather than something to slip into this PR. Flagging it so the inconsistency is a known one rather than a surprise — happy to take it as a follow-up if you want it.

Lens: 114 files, 1,411 tests passing, typecheck clean.

Miracle656 added a commit that referenced this pull request Oct 23, 2026
#584 documented the half-open [from, to) semantics on the `from` and `to`
query parameters in openapi.yaml, and the generated client carries those
descriptions as JSDoc. I merged it without regenerating, so #403's
`git diff --exit-code -- clients/typescript` guard failed on main — which is
the guard doing exactly its job one merge after it was added.

Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Miracle656 added a commit that referenced this pull request Oct 23, 2026
computeTWAP already took a `network` and filtered on it (`AND network = $2`),
but the routes never passed one, so the figures were computed for whatever the
default was rather than the network the caller asked for. Same class of bug as
#584 found in /candles, /basket and /benchmark — a price that silently mixes
or mistakes chains.

Both routes now resolve `req.network ?? query.network ?? activeNetwork`,
thread it into the computation and echo it in the response, so a caller can
see which chain answered. 122 lines of new route tests cover it.

Conflicts, all additive against work that landed today: #542 added the
decimal-format handling in the same schema and handler blocks this PR adds
`network` to. Unioned — the query schema now carries decimals, decimalFormat
and network, and the handler does both.

Two things I regenerated rather than merged by hand: openapi.json, because it
is generated from openapi.yaml and a hand-merged copy fails the
generator-match test, and the TypeScript client, because #403's drift guard
checks it. Took main's package-lock.json; this PR changes no dependency and
main's is the copy carrying the @rolldown Linux binding CI needs.

Lens: 116 files, 1,426 tests passing, typecheck clean.

Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Miracle656 added a commit that referenced this pull request Oct 23, 2026
listPairs returned bare strings, so a client wanting the assets, the network
or a price had to parse the pair key and then ask again. `pairs` returns the
structured form, and listPairs stays — deprecated with a reason rather than
removed, which is the right call for a published API.

The resolver is good: one query per request using a LATERAL join to pull the
latest price for each active pair, rather than a query per pair, and scoped by
`network = $1` so it cannot blend chains — the bug #584 and #605 both had to
fix elsewhere today.

Conflicts, resolved by keeping main's richer side and adding this PR's:

- imports: kept main's (NetworkName, the oracle adapter and comparison) and
  added getActivePairs
- the schema declaration: kept main's `export const schema`, which the SDL
  artifact generator depends on, and added the Asset and Pair types
- the Query block: kept main's documented getPriceHistory with its from/to
  range and the oracle query, and added `pairs` plus the @deprecated marker on
  listPairs

Regenerated schema.graphql afterwards. src/__tests__/graphqlSchemaDrift.test.ts
compares the committed SDL against the code-defined schema, so adding types
without regenerating fails it — the same generated-artifact guard as
openapi.json and the TypeScript client.

Lens: 118 files, 1,432 tests passing, typecheck clean.

Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
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.

Window boundaries are inclusive at both ends

2 participants