From 6220a33e797b8d6bee2be00417f3bb8b45386a54 Mon Sep 17 00:00:00 2001 From: Island Bitcoin <34528298+islandbitcoin@users.noreply.github.com> Date: Mon, 20 Apr 2026 22:13:02 -0400 Subject: [PATCH] ENG-317: promote price wire field from float to double (Phase A) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `double price_v2 = 3` alongside the existing `float price = 1` (now deprecated) on PriceResponse and the equivalent on Tick. Servers populate both fields; clients prefer `price_v2` and fall back to `price` so the two repos can roll out independently. Float32 quantises non-USD/non-BTC rates (notably JMD-class rates) by ~6e-8 relative drift, which compounds to ~J$19 of uncertainty on a 1-BTC quote — the wire-level baseline behind ENG-316. Phase B (separate PR, after one prod release cycle on Phase A) removes `float price` entirely. --- .../price/__tests__/wire-precision.test.ts | 143 ++++++++++++++++++ src/services/price/index.ts | 38 ++++- src/services/price/protos/price.proto | 16 +- src/services/price/protos/price_history.proto | 7 +- 4 files changed, 194 insertions(+), 10 deletions(-) create mode 100644 src/services/price/__tests__/wire-precision.test.ts diff --git a/src/services/price/__tests__/wire-precision.test.ts b/src/services/price/__tests__/wire-precision.test.ts new file mode 100644 index 000000000..ec1e54877 --- /dev/null +++ b/src/services/price/__tests__/wire-precision.test.ts @@ -0,0 +1,143 @@ +/** + * Wire-precision smoke test for ENG-317 (float→double rollout, Phase A). + * + * Demonstrates that: + * 1. The legacy `float` field exhibits drift on JMD-class rates. + * 2. The new `double` field carries the JS number through unchanged. + * 3. The client-side `preferDouble` helper picks the new field when + * present and falls back to the old field when it isn't (i.e. when + * talking to a pre-ENG-317 server). + * + * Pure unit test — no actual gRPC server is started; we synthesize the + * decoded message shape that `@grpc/proto-loader` produces with + * `defaults: true`. + */ + +const SATS_PER_BTC = 100_000_000 + +// Float32 round-trip helper. The `Float32Array` truncates the JS number +// (which is a float64) down to 32-bit precision, then reads it back as a +// float64. This is exactly what protobuf's `float` field does on the wire. +const float32RoundTrip = (n: number): number => { + const buf = new Float32Array(1) + buf[0] = n + return buf[0] +} + +// Same shape as the helper inside src/services/price/index.ts. Kept here to +// avoid pulling in the full module (which imports gRPC + config). If you +// change one, change the other. +const preferDouble = (resp: { price?: number; price_v2?: number }): number => + resp.price_v2 || resp.price || 0 + +describe("ENG-317 wire precision", () => { + describe("float vs double round-trip drift", () => { + // Realistic, decimal-noisy rates — i.e. rates whose binary expansion + // exceeds the 24-bit float32 mantissa. Round numbers like 157.5, + // 60_000, or 9_400_000 happen to be exactly representable in float32 + // and so demonstrate nothing; real-world FX feeds are not that tidy. + const samples: ReadonlyArray<{ name: string; rate: number }> = [ + { name: "JMD per sat (the ENG-316 culprit)", rate: 9.45 }, + { name: "JMD per BTC (intraday)", rate: 9_456_321.07 }, + { name: "USD per BTC (intraday)", rate: 65_432.17 }, + { name: "JPY per BTC (intraday)", rate: 9_415_287.31 }, + ] + + for (const { name, rate } of samples) { + it(`${name}: float32 wire drifts vs float64`, () => { + // The double sits in JS already (Number === float64). The float + // round-trip is what the legacy `float price = 1` field does. + const f32 = float32RoundTrip(rate) + const f64 = rate + + // For every sample above the float32 representation differs from + // the float64 by a measurable amount. + expect(f32).not.toBe(f64) + // Float32 has ~24 bits of mantissa → relative error bounded by + // ~6e-8. Locking that in catches an accidental promotion of the + // helper to float64 (which would silently make this test pass for + // the wrong reason). + const relDrift = Math.abs(f32 - f64) / Math.abs(f64) + expect(relDrift).toBeGreaterThan(0) + expect(relDrift).toBeLessThan(1e-6) + }) + } + + it("absolute J$ drift on a 1-BTC balance is large enough to matter", () => { + // 1 BTC = 100M sats. The wire-only float32→float64 drift on the JMD + // rate is roughly J$19 of uncertainty on a single 1-BTC quote. That's + // the ENG-316 / roadmap §4.1 baseline this PR closes. + const onBtc = SATS_PER_BTC + const driftedTotal = onBtc * float32RoundTrip(9.45) + const cleanTotal = onBtc * 9.45 + + const absDrift = Math.abs(driftedTotal - cleanTotal) + // The drift is materially larger than 1 JMD-cent. + expect(absDrift).toBeGreaterThan(1) + // Sanity ceiling — keeps this test honest if anyone "fixes" it later + // by accidentally widening to float64. + expect(absDrift).toBeLessThan(1000) + }) + + it("absolute J$ drift on a typical 10k-sat retail amount is sub-cent", () => { + // The damage is amount-proportional; on a 10k-sat amount the wire + // drift is < 0.01 JMD-cent. The Phase 0 hotfix (ENG-316) was needed + // because the *non-rounding* of the displayed JMD compounded this + // into visible UI dust. The double wire eliminates the input drift + // before any display rounding has to compensate. + const tenK = 10_000 + const drift = Math.abs(tenK * 9.45 - tenK * float32RoundTrip(9.45)) + expect(drift).toBeLessThan(0.01) + expect(drift).toBeGreaterThan(0) + }) + }) + + describe("preferDouble — client field selection", () => { + it("uses price_v2 when the new server populates it", () => { + // New server — both fields populated. price_v2 is double-exact; + // price has been quantised through float32 on the wire. + const rate = 9.45 + expect(preferDouble({ price: float32RoundTrip(rate), price_v2: rate })).toBe( + rate, + ) + }) + + it("falls back to price when the server is pre-ENG-317", () => { + // Old server — only the legacy field is on the wire. proto-loader + // with `defaults: true` synthesises price_v2 = 0 for the missing + // field. The fallback must trigger. + const rate = float32RoundTrip(9.45) + expect(preferDouble({ price: rate, price_v2: 0 })).toBe(rate) + }) + + it("returns 0 when both fields are missing or zero", () => { + // Sentinel for "no price available" — caller maps this to + // PriceNotAvailableError. + expect(preferDouble({})).toBe(0) + expect(preferDouble({ price: 0, price_v2: 0 })).toBe(0) + }) + + it("never silently uses a stale float when the double is present and non-zero", () => { + // Belt-and-braces: price_v2 wins even when price is also a non-zero + // (drifted) value. + const v2 = 9.45 + const v1 = float32RoundTrip(9.45) + expect(v1).not.toBe(v2) + expect(preferDouble({ price: v1, price_v2: v2 })).toBe(v2) + }) + }) + + describe("Tick (price_history) shape", () => { + it("price_v2 takes precedence inside Tick rows the same way", () => { + const tick = { + timestamp: 1_700_000_000, + price: float32RoundTrip(9.45), + price_v2: 9.45, + } + // Same selection logic; documented at the call site in index.ts. + const selected = tick.price_v2 || tick.price + expect(selected).toBe(9.45) + expect(selected / SATS_PER_BTC).toBe(9.45 / SATS_PER_BTC) + }) + }) +}) diff --git a/src/services/price/index.ts b/src/services/price/index.ts index 847d212cf..7abfc5656 100644 --- a/src/services/price/index.ts +++ b/src/services/price/index.ts @@ -39,6 +39,26 @@ const priceHistoryClient = new PriceHistoryProtoDescriptor.PriceHistory( ) const listPrices = util.promisify(priceHistoryClient.listPrices).bind(priceHistoryClient) +/** + * ENG-317 / Phase A of the float→double rollout for the price wire format. + * + * The wire now carries two fields: `price` (float32, deprecated, original tag) + * and `price_v2` (double, new tag). Servers populate both during Phase A; + * older servers populate only `price`. We prefer `price_v2` and fall back to + * `price` so this client works against both server versions. + * + * `@grpc/proto-loader` is configured with `defaults: true`, which means + * unset scalar fields arrive as their proto3 default (`0` for floating-point + * types). A real-time price of `0` is already treated as "no price" by the + * `PriceNotAvailableError` path, so a falsy check is the correct fallback + * trigger. + * + * Phase B (separate PR, after Phase A is in prod for one release cycle) will + * remove the deprecated `price` field entirely. + */ +const preferDouble = (resp: { price?: number; price_v2?: number }): number => + resp.price_v2 || resp.price || 0 + export const PriceService = (): IPriceService => { const getSatRealTimePrice = ({ displayCurrency, @@ -78,14 +98,14 @@ export const PriceService = (): IPriceService => { } // FIXME: price server should return CentsPerSat directly and timestamp - const { price } = await getPrice({ currency: displayCurrency }) + const priceResponse = await getPrice({ currency: displayCurrency }) + const price = preferDouble(priceResponse) if (!price) return new PriceNotAvailableError() let displayCurrencyPrice = price / SATS_PER_BTC if (walletCurrency === WalletCurrency.Usd) { - const { price: usdBtcPrice } = await getPrice({ - currency: UsdDisplayCurrency, - }) + const usdPriceResponse = await getPrice({ currency: UsdDisplayCurrency }) + const usdBtcPrice = preferDouble(usdPriceResponse) if (!usdBtcPrice) return new PriceNotAvailableError() displayCurrencyPrice = price / usdBtcPrice / CENTS_PER_USD @@ -107,10 +127,12 @@ export const PriceService = (): IPriceService => { }: ListHistoryArgs): Promise => { try { const { priceHistory } = await listPrices({ range }) - return priceHistory.map((t: { timestamp: number; price: number }) => ({ - date: new Date(t.timestamp * 1000), - price: t.price / SATS_PER_BTC, - })) + return priceHistory.map( + (t: { timestamp: number; price?: number; price_v2?: number }) => ({ + date: new Date(t.timestamp * 1000), + price: preferDouble(t) / SATS_PER_BTC, + }), + ) } catch (err) { return new UnknownPriceServiceError(err) } diff --git a/src/services/price/protos/price.proto b/src/services/price/protos/price.proto index aa03f93a7..d7eff17e7 100644 --- a/src/services/price/protos/price.proto +++ b/src/services/price/protos/price.proto @@ -6,7 +6,21 @@ service PriceFeed { } message PriceResponse { - float price = 1; + // ENG-317 / Phase A of float→double rollout. + // + // `price` (float32) is lossy for non-USD/non-BTC currencies — the JMD + // round-trip exhibited ~J$0.80 of drift, see ENG-316 / lnflash/flash#282. + // It is preserved here at its original tag for wire compatibility with + // pre-ENG-317 clients and servers, and is marked deprecated. + // + // Servers MUST populate both `price` and `price_v2` for the duration of + // Phase A. Clients SHOULD prefer `price_v2` and fall back to `price` when + // talking to a server that has not yet been upgraded. + // + // Phase B (future PR) will remove `float price = 1` once all clients are + // confirmed upgraded in prod. + float price = 1 [deprecated = true]; + double price_v2 = 3; } message PriceQuery { diff --git a/src/services/price/protos/price_history.proto b/src/services/price/protos/price_history.proto index 41df871a1..cde86d34e 100644 --- a/src/services/price/protos/price_history.proto +++ b/src/services/price/protos/price_history.proto @@ -22,5 +22,10 @@ message PriceHistoryResponse { message Tick { uint64 timestamp = 1; - float price = 2; + // ENG-317 / Phase A of float→double rollout. See price.proto for the full + // rationale. Servers MUST populate both fields; clients SHOULD prefer + // `price_v2` and fall back to `price` when the server has not yet been + // upgraded. `float price = 2` will be removed in Phase B. + float price = 2 [deprecated = true]; + double price_v2 = 3; }