Skip to content

fix: keep numeric through aggregation and cast once at output - #470

Open
Godfrey-Delight wants to merge 4 commits into
Divinity-Labs:mainfrom
Godfrey-Delight:fix/230-numeric-price-precision
Open

Godfrey-Delight wants to merge 4 commits into
Divinity-Labs:mainfrom
Godfrey-Delight:fix/230-numeric-price-precision

Conversation

@Godfrey-Delight

Copy link
Copy Markdown
Contributor

Summary

  • Kept intermediate aggregations in PostgreSQL as exact arbitrary-precision numeric, removing all ::float casts inside aggregate expressions across /candles, /prices/history, /price/:assetA/:assetB/history, and GraphQL getPriceHistory.
  • Added src/pricing/precision.ts with explicit serialization edge formatters (formatPrice and formatVolume) enforcing documented precision aligned with schema.prisma: up to 18 decimal places for prices (Decimal(36, 18)) and 7 decimal places for volumes (Decimal(36, 7) matching Stellar stroop precision).
  • Fixed floating-point accumulation error across venues in GET /volumes/:asset.
  • Added src/__tests__/numericPrecision.test.ts exercising /candles, /volumes, /prices/history, and /price/*/history with values that differ between numeric and float arithmetic, asserting exact expected output and confirming no ::float remains in aggregate queries.
  • Documented output precision in README.md, openapi.yaml, openapi.json, and added a patch changeset.

Related issue

Closes #230

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Docs
  • Tests
  • CI / tooling

Checklist

  • I have read CONTRIBUTING.md
  • npx tsc --noEmit passes
  • npm run build passes
  • I added / updated tests where relevant
  • I updated docs where relevant

@drips-wave

drips-wave Bot commented Oct 17, 2026

Copy link
Copy Markdown

@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! 🚀

Learn more about application limits

@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.

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): DecimalFormat

A 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::numeric

So 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:

  1. Move every ::float out 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.
  2. Drop src/pricing/precision.ts. Use resolveDecimalFormat + formatDecimal from src/utils/decimals.ts at the edge.
  3. Keep your test, change what it asserts. src/__tests__/numericPrecision.test.ts is the most valuable thing in the PR — values chosen so numeric and float arithmetic disagree is exactly what the issue asks for. Point it at the route output in string mode and assert the exact expected digits; it will then fail against main today and pass after, which is the acceptance criterion. A number-mode assertion cannot distinguish the two, for the reason above.
  4. Document the precision, as the issue asks: 18 places for price and 7 for volume are reachable only in string mode, and number mode is explicitly lossy. That belongs in the README section you are already editing and in the openapi.yaml description for the affected fields.
  5. openapi.json is generated — run npm run openapi:gen rather than editing it, and npx --yes openapi-typescript@7.13.0 openapi.yaml -o clients/typescript/src/schema.d.ts after. 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.

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.

Prices are stored to 18 decimal places and computed as floats

2 participants