Repository navigation
Preserve decimal precision in GraphQL prices - #389
Jossy-ossy wants to merge 3 commits into
Conversation
|
@Jossy-ossy 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! 🚀 |
2632c70 to
330d6ba
Compare
|
@Miracle656 I resolved the merge conflicts and updated this branch. Please take another look and merge when ready. |
tsc --noEmit clean vitest 93 files, 1068 passing (up from 91 / 1052) dropped-tests none #433 from/to range args on getPriceHistory #456 make duplicate price points impossible at the database #456 is the better of the two: a uniqueness constraint in the schema rather than a check in the ingest path, so a duplicate cannot be written even by a code path that forgets to look. It carries a migration and adds 14 tests. Two others from this group were NOT merged -- both fail on their own, so this is not a merge-order problem: #389 (preserve decimal precision in GraphQL prices) 2 tsc errors, 31 test failures in isolation: src/aggregator/vwap.ts(250): Cannot find name 'isNonZeroDecimal' The function is called twice and never defined. #462 (read per-pool AMM fees from the venue) 4 tsc errors, 2 failures in isolation: src/api/rest.ts(281): Expected 4 arguments, but got 3 src/api/rest.ts(286): 'pairKey' is specified more than once The duplicate key is the shape a repeated base merge produces. Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
This does not compile on its own branch — it is not a merge-order problem, so please do not rebase and re-request without fixing it first.
src/aggregator/vwap.ts(250,17): error TS2304: Cannot find name 'isNonZeroDecimal'
src/aggregator/vwap.ts(250,63): error TS2304: Cannot find name 'isNonZeroDecimal'
The function is called twice on line 250 and never defined or imported. Merged alone against current main, that is 2 type errors and 31 failing tests.
The goal is right and it is the same one #542 is chasing: preserving exact decimals from Postgres instead of round-tripping through a double. If isNonZeroDecimal exists on another branch of yours, it needs to come with this one; if it was meant to be new, it needs writing.
Worth coordinating with #542 before you go further — it covers the same ground for the REST boundary and has a specific unresolved gap (volume24h and priceChange24h are still summed as doubles). Two independent decimal fixes landing on the same module will conflict, and the one that lands second will look like it broke the first.
|
Note for maintainers: the compile error on this branch (isNonZeroDecimal undefined in vwap.ts) is fixed. The Float fields are deprecated, not removed, and the repo has no Money type. Please coordinate with #542 on volume24h and priceChange24h. |
Merge origin/main into the PR branch and resolve the conflicts: - vwap.ts: take main's string-format aggregate (decimalFormat: 'string') instead of a parallel copy. Add calculateVWAPExact and calculateOHLCVExact as string-returning wrappers, so aggregateRefresh keeps persisting exact Prisma.Decimal values. - priceFields.ts: withExactPriceFields maps the string aggregate to the deprecated Float fields and the *Exact Decimal fields. - graphql.ts: the getPrice resolver and the Decimal scalar, with the Asset and Pair types from main kept alongside. - rest.ts, routes/screener.ts: take main's versions, which already negotiate decimal format and delegate the screener to services/screener.ts. This also fixes the compile error reported in review, where isNonZeroDecimal was called but never defined. Regenerate schema.graphql, add a changeset, and update the tests that assumed the superseded aggregate shape.
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
Scanned clean. Two things to know here, and the second one changes what this PR should contain.
1. The branch is 79 commits behind main. Against current main it removes around 6,000 lines from src/ that are not yours — src/reconciliation/dispersion.ts (312, #448), src/__tests__/venueReconciliation.test.ts (385) and src/__tests__/observationAndSourceTime.test.ts (335, #240) among them. That is what a 79-commit-old base does, not a judgement on your code.
2. Most of this landed an hour ago in #470 (4b5ced6), so a rebase will tell you a lot. What is now on main:
- No
::floatcast remains inside any aggregate expression insrc/—SUM(x::float)becameSUM(x)::floatinsrc/api/history.ts,src/api/graphql.ts,src/routes/candles.tsandsrc/routes/movers.ts. That was the real bug: casting per row made the total depend on the order rows were summed in. src/pricing/precision.tsnarrows once at the serialization edge, withformatPriceandformatVolumeused by the GraphQL resolvers.
So please rebase and then look at what is actually left. If your approach differs from #470's, say how and I will take that seriously — there is a real question still open here that #470 did not solve:
A JavaScript number cannot carry a Decimal(36, 18) price. It is an IEEE-754 double with 15–17 significant digits, so anything returning a Float from GraphQL is lossy no matter how carefully it is rounded. #470's helpers are honest about that now rather than claiming otherwise, but the underlying limitation stands. If "preserve decimal precision in GraphQL prices" means genuinely preserving it, that needs a String scalar on those fields — which src/utils/decimals.ts already does for REST, negotiated per request.
That would be a more interesting PR than the cast fix, and it is still unclaimed. Your call whether to take it or close this one.
|
Correction to my earlier review — I got the main point wrong. I said this branch was far behind Measured properly against current Nothing would be reverted. I apologise; that framing was both alarming and inaccurate. The points about the code itself are unchanged. Most of the |
Summary
Decimalscalar and exact companion fields for monetary values.The trade-off and migration path are documented in
docs/GRAPHQL_DECIMAL_MIGRATION.md.Closes #371