Repository navigation
fix: keep numeric through aggregation and cast once at output - #470
Godfrey-Delight wants to merge 4 commits into
Conversation
|
@Godfrey-Delight 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.
Your diagnosis of the SQL half is exactly right, and it is still a live bug on main — so I want this fixed. But the serialization half cannot work the way it is written, and the ground has moved under the PR since you cut it. Both are worth going through properly.
The half that is right, and still needed
main today:
src/api/history.ts:90 SUM(volume::float) AS volume
src/routes/candles.ts:64 MAX(price::float) AS high,
src/routes/candles.ts:65 MIN(price::float) AS low,
src/routes/candles.ts:67 SUM(base_volume::float) AS volume
src/routes/movers.ts:82 SUM(base_volume::float) AS current_volume,
src/routes/movers.ts:95 SUM(base_volume::float) AS prev_volume
Casting inside the aggregate is the bug, and it is worse than casting once: every row becomes a 53-bit double before the sum, so the error accumulates and — as the issue says — accumulates differently depending on row order, so the same window can produce two different closes. Moving the cast outside the aggregate, as you do, is the fix.
Since you branched, three more files have arrived with the same pattern: src/routes/movers.ts, src/routes/snapshot.ts and src/routes/screener.ts. The acceptance criterion is "no ::float cast remains inside an aggregate expression", so those need the same treatment for #230 to close.
The half that cannot work: formatPrice is a double
export const PRICE_DECIMAL_PLACES = 18
export function formatPrice(value: string | number | null | undefined): number | null {
…
return Number(num.toFixed(PRICE_DECIMAL_PLACES))
}The return type is number, and a JavaScript number is an IEEE-754 double — the same 53 bits the ::float cast was losing. toFixed(18) produces a string with eighteen decimal places and Number(...) immediately throws that away again:
exact string : 0.123456789012345678
Number(v).toFixed(18) : 0.123456789012345677 ← already wrong
formatPrice(v) : 0.12345678901234568 ← three digits gone
and on a day's volume:
exact : 12345678901234.1234567
formatVolume(exact) : 12345678901234.123 ← four digits gone
So the pipeline becomes: exact numeric in Postgres → exact string over the wire from pg → double in formatPrice → JSON. The ::float cast was removed from the aggregate and then performed in JavaScript instead. For the 1e-7 pair in the issue the output is still wrong, just wrong in a different place.
There is no way around this: 18 decimal places does not fit in a double, so the serialised value has to be a string (or the precision has to be explicitly reduced to something that does fit, and documented as such).
What main already does for the output edge
This is the part that changed under you. src/utils/decimals.ts landed with #248, and it solves the serialisation half correctly:
export type DecimalFormat = 'number' | 'string'
export function resolveDecimalFormat(req: FastifyRequest): DecimalFormatA client asks via X-Decimal-Format: string, Accept: application/json; decimals=strings, or ?decimals=string, and gets the exact decimal as a string; the default stays number so existing consumers are untouched. calculateVWAP now returns { num, raw } precisely so the exact digits survive to the edge, and the route handlers call formatDecimal(value, decimalFormat).
main's version of the same /pools query you touch already reads:
SELECT bucket, window, vwap::numeric, sdex_vwap::numeric, amm_vwap::numeric,
volume::numeric, trade_count, open_price::numeric, close_price::numeric,
high_price::numeric, low_price::numericSo src/pricing/precision.ts duplicates src/utils/decimals.ts with a weaker implementation, and formatPrice/formatVolume should go.
What I would merge
Rebase on main and keep the part that is yours:
- Move every
::floatout of every aggregate —candles.ts,history.ts,volumes.ts,graphql.ts, plus the three new ones (movers.ts,snapshot.ts,screener.ts). Cast outside the aggregate, or not at all where the row already arrives as a string. - Drop
src/pricing/precision.ts. UseresolveDecimalFormat+formatDecimalfromsrc/utils/decimals.tsat the edge. - Keep your test, change what it asserts.
src/__tests__/numericPrecision.test.tsis the most valuable thing in the PR — values chosen sonumericandfloatarithmetic disagree is exactly what the issue asks for. Point it at the route output instringmode and assert the exact expected digits; it will then fail againstmaintoday and pass after, which is the acceptance criterion. A number-mode assertion cannot distinguish the two, for the reason above. - Document the precision, as the issue asks: 18 places for price and 7 for volume are reachable only in
stringmode, andnumbermode is explicitly lossy. That belongs in the README section you are already editing and in theopenapi.yamldescription for the affected fields. openapi.jsonis generated — runnpm run openapi:genrather than editing it, andnpx --yes openapi-typescript@7.13.0 openapi.yaml -o clients/typescript/src/schema.d.tsafter. Both are byte-compared in CI.
Five files conflict right now, and src/api/rest.ts is one of them, so a rebase rather than a merge will give you a much cleaner starting point — git merge here produces a tree that keeps both sides of the same SELECT.
The SQL insight is good and the bug is real; I would rather have this from you, rebased, than paper over it. Ping me when it is up.
Summary
numeric, removing all::floatcasts inside aggregate expressions across/candles,/prices/history,/price/:assetA/:assetB/history, and GraphQLgetPriceHistory.src/pricing/precision.tswith explicit serialization edge formatters (formatPriceandformatVolume) enforcing documented precision aligned withschema.prisma: up to 18 decimal places for prices (Decimal(36, 18)) and 7 decimal places for volumes (Decimal(36, 7)matching Stellar stroop precision).GET /volumes/:asset.src/__tests__/numericPrecision.test.tsexercising/candles,/volumes,/prices/history, and/price/*/historywith values that differ between numeric and float arithmetic, asserting exact expected output and confirming no::floatremains in aggregate queries.README.md,openapi.yaml,openapi.json, and added a patch changeset.Related issue
Closes #230
Type of change
Checklist
npx tsc --noEmitpassesnpm run buildpasses