Repository navigation
Preserve decimal precision in GraphQL prices - #389
Jossy-ossy wants to merge 2 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.
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