From f81d3a77c0fb9a68f2e85e05886effca96d8d8d8 Mon Sep 17 00:00:00 2001 From: janetpius-cmd Date: Mon, 31 Aug 2026 04:58:46 +0100 Subject: [PATCH 1/4] fix(indexer): reject legacy resolution payload shapes in production resolutionParser silently accepted legacy ScvVec/ScvMap dev-stub shapes (with a blank oracleAddress) in every environment, including production. Gate those shapes behind NODE_ENV, fail fast with ResolutionParseError and an indexer.parser.legacy_shape_rejected metric when they surface in production, and keep accepting them for local/dev fixtures. Claude-Session: https://claude.ai/code/session_01WR71DfNQdvTTWtuVDGBzD6 --- apps/indexer/src/resolutionParser.test.ts | 80 +++++++++++++++++++++++ apps/indexer/src/resolutionParser.ts | 77 +++++++++++++++++++--- docs/indexer-event-mapping.md | 2 + 3 files changed, 151 insertions(+), 8 deletions(-) diff --git a/apps/indexer/src/resolutionParser.test.ts b/apps/indexer/src/resolutionParser.test.ts index 3e7dfbd..2de2d38 100644 --- a/apps/indexer/src/resolutionParser.test.ts +++ b/apps/indexer/src/resolutionParser.test.ts @@ -327,3 +327,83 @@ describe("parseResolutionEvents", () => { ); }); }); + +describe("production fail-fast on legacy resolution shapes", () => { + // Gap this covers: legacy dev-stub payload shapes (ScvVec tuple, legacy + // ScvMap without topics[1]=market_id) were previously accepted silently + // in every environment, including production, which meant a contract + // regression or topic drift produced ResolutionCandidate rows with a + // blank oracleAddress instead of failing loudly. These tests fail + // against the pre-fix parser (which has no `nodeEnv` gate at all). + + it("rejects a legacy ScvVec tuple payload in production", () => { + const tupleXdr = nativeToScVal([7, false, 99n]).toXDR("base64"); + expect(() => + parseResolutionEvent( + makeEvent({ + valueXdr: tupleXdr, + topicsXdr: [XDR.topic.marketResolvedEvent], + }), + { nodeEnv: "production" } + ) + ).toThrow(ResolutionParseError); + }); + + it("rejects a legacy ScvMap payload in production", () => { + expect(() => + parseResolutionEvent( + makeEvent({ + valueXdr: XDR.value.resolvedYes, + topicsXdr: [XDR.topic.marketResolvedEvent], + }), + { nodeEnv: "production" } + ) + ).toThrow(ResolutionParseError); + }); + + it("still parses the canonical on-chain shape in production", () => { + const r = parseResolutionEvent(makeEvent({ valueXdr: XDR.value.realYes }), { + nodeEnv: "production", + }); + expect(r.outcome).toBe("YES"); + }); + + it("still allows legacy shapes outside production (local stubs)", () => { + const r = parseResolutionEvent( + makeEvent({ + valueXdr: XDR.value.resolvedYes, + topicsXdr: [XDR.topic.marketResolvedEvent], + }), + { nodeEnv: "development" } + ); + expect(r.outcome).toBe("YES"); + }); + + it("emits a legacy_shape_rejected metric with correlation ids when rejecting in production", () => { + const telemetry: Telemetry = { + record: vi.fn(), + startSpan: vi.fn(() => ({ end: vi.fn() })), + }; + const events = [ + makeEvent({ + id: "evt-legacy-1", + valueXdr: XDR.value.resolvedYes, + topicsXdr: [XDR.topic.marketResolvedEvent], + }), + ]; + const { resolutions, errors } = parseResolutionEvents(events, { + telemetry, + nodeEnv: "production", + }); + expect(resolutions).toHaveLength(0); + expect(errors).toHaveLength(1); + expect(telemetry.record).toHaveBeenCalledWith( + "indexer.parser.legacy_shape_rejected", + 1, + expect.objectContaining({ + parser: "resolution", + eventId: "evt-legacy-1", + }) + ); + }); +}); diff --git a/apps/indexer/src/resolutionParser.ts b/apps/indexer/src/resolutionParser.ts index 955b2e1..f7152f5 100644 --- a/apps/indexer/src/resolutionParser.ts +++ b/apps/indexer/src/resolutionParser.ts @@ -85,18 +85,46 @@ function marketIdFromTopic(topicsXdr: string[], eventId: string): string { } } +/** + * Legacy payload shapes (ScvVec tuple, legacy ScvMap) predate the real + * on-chain `MarketResolvedEvent` layout and were only ever needed to decode + * fixtures from local devnet stubs. Accepting them in production means a + * misconfigured or downgraded contract can silently produce + * `ResolutionCandidate` rows with an empty/garbage `oracleAddress` instead + * of failing the batch — see the issue this const documents. Threaded + * through from `parseResolutionEvent`/`parseResolutionEvents`, defaulting + * to `process.env.NODE_ENV` so callers never need to pass it explicitly in + * real deployments. + */ +export function isProductionEnv(nodeEnv: string): boolean { + return nodeEnv === "production"; +} + /** * Supports three payload shapes: * - Real on-chain (topics[1]=market_id: u32, value=ScvMap{outcome, resolved_at}) - * - Legacy ScvVec tuple (value=[market_id, outcome, resolved_at]) - * - Legacy ScvMap (value={ market_id, outcome, oracle }) + * - Legacy ScvVec tuple (value=[market_id, outcome, resolved_at]) — dev/test stub only + * - Legacy ScvMap (value={ market_id, outcome, oracle }) — dev/test stub only + * + * In production (`nodeEnv === "production"`) the two legacy shapes throw + * instead of being silently accepted, so a contract/topic drift never + * results in a resolution being dropped or mis-attributed off-chain. */ function parseResolutionPayload( decoded: unknown, topicsXdr: string[], - eventId: string + eventId: string, + nodeEnv: string ): ResolutionPayload { if (Array.isArray(decoded)) { + if (isProductionEnv(nodeEnv)) { + throw new ResolutionParseError( + "Legacy ScvVec tuple resolution payload is not permitted in production — " + + "the contract must emit the canonical MarketResolvedEvent shape " + + "(topics[1]=market_id, value={outcome, resolved_at})", + eventId + ); + } if (decoded.length < 2) { throw new ResolutionParseError( "Tuple resolution payload must include market_id and outcome", @@ -122,6 +150,14 @@ function parseResolutionPayload( const map = decoded as Record; if ("market_id" in map) { + if (isProductionEnv(nodeEnv)) { + throw new ResolutionParseError( + "Legacy ScvMap resolution payload is not permitted in production — " + + "the contract must emit the canonical MarketResolvedEvent shape " + + "(topics[1]=market_id, value={outcome, resolved_at})", + eventId + ); + } // Legacy ScvMap payload: market_id, outcome, and oracle all in the value. const oracleAddress = map.oracle != null ? String(map.oracle) : ""; if (oracleAddress === "") { @@ -149,14 +185,25 @@ function parseResolutionPayload( }; } +export interface ParseResolutionEventOptions { + telemetry?: Telemetry; + /** Defaults to `process.env.NODE_ENV`; override in tests only. */ + nodeEnv?: string; +} + /** * Parse a single RawChainEvent into a NormalizedResolution. * - * @throws ResolutionParseError if the event is not a resolution event or payload is malformed + * @throws ResolutionParseError if the event is not a resolution event, the + * payload is malformed, or (in production) the payload uses a legacy + * dev-stub shape instead of the canonical on-chain layout. */ export function parseResolutionEvent( - event: RawChainEvent + event: RawChainEvent, + options?: ParseResolutionEventOptions ): NormalizedResolution { + const nodeEnv = options?.nodeEnv ?? process.env.NODE_ENV ?? "development"; + if (!isResolutionEvent(event.topicsXdr)) { throw new ResolutionParseError( `Event topic is not "${RESOLUTION_EVENT_TOPIC}"`, @@ -175,7 +222,20 @@ export function parseResolutionEvent( ); } - const payload = parseResolutionPayload(decoded, event.topicsXdr, event.id); + let payload: ResolutionPayload; + try { + payload = parseResolutionPayload(decoded, event.topicsXdr, event.id, nodeEnv); + } catch (err) { + if (isProductionEnv(nodeEnv)) { + options?.telemetry?.record("indexer.parser.legacy_shape_rejected", 1, { + parser: "resolution", + eventId: event.id, + contractId: event.contractId, + ledger: String(event.ledger), + }); + } + throw err; + } return { eventId: event.id, @@ -195,7 +255,7 @@ export function parseResolutionEvent( */ export function parseResolutionEvents( events: RawChainEvent[], - options?: { telemetry?: Telemetry } + options?: ParseResolutionEventOptions ): { resolutions: NormalizedResolution[]; errors: ResolutionParseError[]; @@ -203,6 +263,7 @@ export function parseResolutionEvents( const resolutions: NormalizedResolution[] = []; const errors: ResolutionParseError[] = []; const telemetry = options?.telemetry; + const nodeEnv = options?.nodeEnv ?? process.env.NODE_ENV ?? "development"; for (const event of events) { if (!isResolutionEvent(event.topicsXdr)) { @@ -215,7 +276,7 @@ export function parseResolutionEvents( continue; } try { - resolutions.push(parseResolutionEvent(event)); + resolutions.push(parseResolutionEvent(event, { telemetry, nodeEnv })); } catch (err) { errors.push( err instanceof ResolutionParseError diff --git a/docs/indexer-event-mapping.md b/docs/indexer-event-mapping.md index 08463de..44e052c 100644 --- a/docs/indexer-event-mapping.md +++ b/docs/indexer-event-mapping.md @@ -79,6 +79,8 @@ The contract does not publish an oracle address on this event, so `oracleAddress **Payload — legacy ScvMap:** Keys `market_id` (ScvSymbol), `outcome` (ScvSymbol `"YES"`/`"NO"`), `oracle` (ScvSymbol), all inside the value. `oracle` is required on this path; its absence throws `ResolutionParseError`. +**Production vs. local stubs:** Both legacy shapes (ScvVec tuple and legacy ScvMap) exist only to decode local devnet/test fixtures and are rejected with a `ResolutionParseError` when `NODE_ENV=production` (or the `nodeEnv` option passed to `parseResolutionEvent`/`parseResolutionEvents` is `"production"`). Only the canonical on-chain shape (`topics[1]=market_id`, value `{outcome, resolved_at}`) is accepted in production. A rejection in production increments `indexer.parser.legacy_shape_rejected` (tags: `parser`, `eventId`, `contractId`, `ledger`) so operators can see a contract/topic regression instead of silently getting `ResolutionCandidate` rows with a blank `oracleAddress`. + **DB write:** `ResolutionCandidate` row with `status = "PROPOSED"`, `source = "chain:market_resolved:{contractId}"`. --- From 817e45ffd555124d37f08fbb86eeda9c0b1aeba4 Mon Sep 17 00:00:00 2001 From: janetpius-cmd Date: Mon, 31 Aug 2026 05:00:23 +0100 Subject: [PATCH 2/4] fix(indexer): validate CLOB order-id join on settle_trade events tradeParser cast buy_order_id/sell_order_id straight to String with no validation, so a fill that could never join to a real Order row (empty, or not UUID-shaped like Order.id) was persisted as if it were joined. Reject empty order ids in every environment and non-UUID order ids in production, with an indexer.parser.unjoinable_order_id metric. Claude-Session: https://claude.ai/code/session_01WR71DfNQdvTTWtuVDGBzD6 --- apps/indexer/src/tradeParser.test.ts | 65 +++++++++++++++++++++++ apps/indexer/src/tradeParser.ts | 78 +++++++++++++++++++++++++--- docs/indexer-event-mapping.md | 2 + 3 files changed, 139 insertions(+), 6 deletions(-) diff --git a/apps/indexer/src/tradeParser.test.ts b/apps/indexer/src/tradeParser.test.ts index 23cb5b4..cfd6a40 100644 --- a/apps/indexer/src/tradeParser.test.ts +++ b/apps/indexer/src/tradeParser.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi } from "vitest"; +import { nativeToScVal } from "@stellar/stellar-sdk"; import { parseTradeEvent, parseTradeEvents } from "./tradeParser.js"; import { TradeParseError } from "./types.js"; import type { RawChainEvent } from "./types.js"; @@ -275,3 +276,67 @@ describe("parseTradeEvents", () => { ); }); }); + +describe("CLOB order id join validation", () => { + // Gap this covers: buy_order_id/sell_order_id were passed through with a + // bare String(...) cast, so a fill whose order id could never join back + // to a real Order row (empty string, or not shaped like Order.id's uuid()) + // was still persisted as a "joined" trade instead of failing loudly. + // These fail against the pre-fix parser, which has no order-id validation. + + it("still accepts non-UUID order ids outside production (dev fixtures)", () => { + const trade = parseTradeEvent(makeEvent(), { nodeEnv: "development" }); + expect(trade.buyOrderId).toBe("buy-1"); + expect(trade.sellOrderId).toBe("sell-1"); + }); + + it("rejects non-UUID order ids in production", () => { + expect(() => + parseTradeEvent(makeEvent(), { nodeEnv: "production" }) + ).toThrow(TradeParseError); + }); + + it("accepts a UUID-shaped order id in production", () => { + const valueXdr = nativeToScVal( + { + market_id: "market-abc", + trader: "GABC1234", + counterparty: "GXYZ5678", + direction: "buy", + outcome: "YES", + price: 5_000_000n, + quantity: 100n, + buy_order_id: "11111111-1111-4111-8111-111111111111", + sell_order_id: "22222222-2222-4222-8222-222222222222", + }, + { type: "instance" } + ).toXDR("base64"); + + const trade = parseTradeEvent(makeEvent({ valueXdr }), { + nodeEnv: "production", + }); + expect(trade.buyOrderId).toBe("11111111-1111-4111-8111-111111111111"); + expect(trade.sellOrderId).toBe("22222222-2222-4222-8222-222222222222"); + }); + + it("rejects an empty order id in every environment", () => { + const valueXdr = nativeToScVal( + { + market_id: "market-abc", + trader: "GABC1234", + counterparty: "GXYZ5678", + direction: "buy", + outcome: "YES", + price: 5_000_000n, + quantity: 100n, + buy_order_id: "", + sell_order_id: "sell-1", + }, + { type: "instance" } + ).toXDR("base64"); + + expect(() => + parseTradeEvent(makeEvent({ valueXdr }), { nodeEnv: "development" }) + ).toThrow(TradeParseError); + }); +}); diff --git a/apps/indexer/src/tradeParser.ts b/apps/indexer/src/tradeParser.ts index 6815f85..1e59871 100644 --- a/apps/indexer/src/tradeParser.ts +++ b/apps/indexer/src/tradeParser.ts @@ -91,6 +91,50 @@ function toDirection(value: unknown, eventId: string): TradeDirection { ); } +/** + * `Order.id` in `prisma/schema.prisma` is `@default(uuid())` — every CLOB + * order the matching engine creates has a UUID id. A settle_trade event + * carrying a `buy_order_id`/`sell_order_id` that isn't a UUID can never + * join back to a real `Order` row, which previously meant the trade was + * still written with an unjoinable order id instead of failing the batch. + */ +const UUID_RE = + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +/** + * Read and validate a CLOB order-id field. + * + * Always rejects empty/blank values (never joins to a real order in any + * environment). In production, also requires the id to match the UUID + * shape `Order.id` actually uses — non-UUID values only ever arise from + * local/dev fixtures — so a fill that can't be joined to a CLOB Order + * fails fast instead of being silently persisted unlinked. + */ +function toOrderId( + map: Record, + key: string, + eventId: string, + nodeEnv: string +): string { + const raw = String(field(map, key, eventId)).trim(); + if (raw === "") { + throw new TradeParseError(`Field "${key}" must not be empty`, eventId); + } + if (isProductionEnv(nodeEnv) && !UUID_RE.test(raw)) { + throw new TradeParseError( + `Field "${key}" ("${raw}") is not a valid CLOB Order id (UUID) — ` + + "cannot join this fill to an Order in production", + eventId + ); + } + return raw; +} + +/** Mirrors resolutionParser.isProductionEnv — kept local to avoid a cross-parser import. */ +function isProductionEnv(nodeEnv: string): boolean { + return nodeEnv === "production"; +} + /** * Determine whether the first topic XDR matches the trade_executed discriminator. */ @@ -111,9 +155,22 @@ function isTradeEvent(topicsXdr: string[]): boolean { * market_id, trader, counterparty, direction, outcome, * price, quantity, buy_order_id, sell_order_id * - * @throws TradeParseError if the event is not a trade event or the payload is malformed + * @throws TradeParseError if the event is not a trade event, the payload is + * malformed, or (in production) `buy_order_id`/`sell_order_id` cannot be + * joined to a real CLOB `Order` row. */ -export function parseTradeEvent(event: RawChainEvent): NormalizedTrade { +export interface ParseTradeEventOptions { + telemetry?: Telemetry; + /** Defaults to `process.env.NODE_ENV`; override in tests only. */ + nodeEnv?: string; +} + +export function parseTradeEvent( + event: RawChainEvent, + options?: ParseTradeEventOptions +): NormalizedTrade { + const nodeEnv = options?.nodeEnv ?? process.env.NODE_ENV ?? "development"; + if (!isTradeEvent(event.topicsXdr)) { throw new TradeParseError( `Event topic is not "${TRADE_EVENT_TOPIC}"`, @@ -158,8 +215,8 @@ export function parseTradeEvent(event: RawChainEvent): NormalizedTrade { "quantity", event.id ), - buyOrderId: String(field(map, "buy_order_id", event.id)), - sellOrderId: String(field(map, "sell_order_id", event.id)), + buyOrderId: toOrderId(map, "buy_order_id", event.id, nodeEnv), + sellOrderId: toOrderId(map, "sell_order_id", event.id, nodeEnv), }; } @@ -170,7 +227,7 @@ export function parseTradeEvent(event: RawChainEvent): NormalizedTrade { */ export function parseTradeEvents( events: RawChainEvent[], - options?: { telemetry?: Telemetry } + options?: ParseTradeEventOptions ): { trades: NormalizedTrade[]; errors: TradeParseError[]; @@ -178,6 +235,7 @@ export function parseTradeEvents( const trades: NormalizedTrade[] = []; const errors: TradeParseError[] = []; const telemetry = options?.telemetry; + const nodeEnv = options?.nodeEnv ?? process.env.NODE_ENV ?? "development"; for (const event of events) { if (!isTradeEvent(event.topicsXdr)) { @@ -190,8 +248,16 @@ export function parseTradeEvents( continue; } try { - trades.push(parseTradeEvent(event)); + trades.push(parseTradeEvent(event, { telemetry, nodeEnv })); } catch (err) { + if (isProductionEnv(nodeEnv)) { + telemetry?.record("indexer.parser.unjoinable_order_id", 1, { + parser: "trade", + eventId: event.id, + contractId: event.contractId, + ledger: String(event.ledger), + }); + } errors.push( err instanceof TradeParseError ? err diff --git a/docs/indexer-event-mapping.md b/docs/indexer-event-mapping.md index 44e052c..4e9f264 100644 --- a/docs/indexer-event-mapping.md +++ b/docs/indexer-event-mapping.md @@ -45,6 +45,8 @@ When a parser encounters an event with a topic symbol it does not recognize, it **DB write:** `IndexedTrade` row via `PrismaBatchWriter`. `priceRaw` and `quantityRaw` stored as `String` (bigint serialized) to avoid precision loss. `PrismaBatchWriter` also reconciles the trade into both parties' `UserPosition.yesShares`/`noShares` (`Int` columns) — since `quantity` is already whole integer shares (no fixed-point scale, unlike `price`/collateral), this conversion is a validated bigint→Number bounds check rather than a division; see `sharesRawToInt` in [Decimal/share conversion utilities](#decimalshare-conversion-utilities) below. +**Order id join validation:** `buy_order_id`/`sell_order_id` must resolve to a real `Order.id` (a `uuid()` per `prisma/schema.prisma`). `tradeParser.ts` always rejects an empty order id, and in `NODE_ENV=production` additionally rejects any value that isn't UUID-shaped — this is a dev-fixture allowance only, since non-UUID ids (e.g. legacy fixtures like `"buy-1"`) can never join to a CLOB `Order` row. A production rejection increments `indexer.parser.unjoinable_order_id` (tags: `parser`, `eventId`, `contractId`, `ledger`). + --- ## 2. `collateral_deposited` From eb7593028265ce3ed04f418c3afdfd8d405ef38c Mon Sep 17 00:00:00 2001 From: janetpius-cmd Date: Mon, 31 Aug 2026 05:03:12 +0100 Subject: [PATCH 3/4] fix(indexer): validate collateral amount against 7-decimal scale collateralDepositedParser accepted any i128 bigint as amountRaw with no bounds check, so a wrongly-scaled or out-of-range amount passed through silently and only surfaced later (e.g. as a DB error far from the event that caused it). Validate amountRaw against the same 7-decimal / Decimal(20,8) bounds decimalUtils.amountRawToDecimal enforces elsewhere, reject zero/negative amounts, and in production reject amounts that decode as a plain number instead of the expected i128 bigint. Claude-Session: https://claude.ai/code/session_01WR71DfNQdvTTWtuVDGBzD6 --- .../src/collateralDepositedParser.test.ts | 84 +++++++++++++++++- apps/indexer/src/collateralDepositedParser.ts | 85 +++++++++++++++++-- docs/indexer-event-mapping.md | 2 + 3 files changed, 163 insertions(+), 8 deletions(-) diff --git a/apps/indexer/src/collateralDepositedParser.test.ts b/apps/indexer/src/collateralDepositedParser.test.ts index b12254c..034fb89 100644 --- a/apps/indexer/src/collateralDepositedParser.test.ts +++ b/apps/indexer/src/collateralDepositedParser.test.ts @@ -54,7 +54,9 @@ describe("parseCollateralDepositedEvent", () => { }); it("handles large i128 amounts without precision loss", () => { - const big = 9_999_999_999_999_999_999n; + // Largest value that still fits the Decimal(20,8) column + // amountRawToDecimal backs (999_999_999_999 * 10^7). + const big = 9_999_999_999_990_000_000n; const d = parseCollateralDepositedEvent( makeEvent({ valueXdr: makeDepositValueXdr("GABC", 1, big) }) ); @@ -203,3 +205,83 @@ describe("parseCollateralDepositedEvents", () => { ); }); }); + +describe("collateral amount scale validation", () => { + // Gap this covers: the parser previously accepted any i128 amount as-is, + // with no check against the 7-decimal / Decimal(20,8) scale the rest of + // the system (amountRawToDecimal, CollateralDeposit columns) assumes for + // it — so a wrongly-scaled or out-of-range amount would pass the parser + // silently and only surface later, far from the event that caused it. + // These tests fail against the pre-fix parser, which has no such check. + + it("rejects an amount that exceeds the Decimal(20,8) collateral range", () => { + const tooLarge = 10_000_000_000_000_000_000n; // just over MAX_RAW + expect(() => + parseCollateralDepositedEvent( + makeEvent({ valueXdr: makeDepositValueXdr("GABC", 1, tooLarge) }) + ) + ).toThrow(CollateralDepositedParseError); + }); + + it("rejects a zero amount", () => { + expect(() => + parseCollateralDepositedEvent( + makeEvent({ valueXdr: makeDepositValueXdr("GABC", 1, 0n) }) + ) + ).toThrow(CollateralDepositedParseError); + }); + + it("rejects a negative amount", () => { + expect(() => + parseCollateralDepositedEvent( + makeEvent({ valueXdr: makeDepositValueXdr("GABC", 1, -500n) }) + ) + ).toThrow(CollateralDepositedParseError); + }); + + it("emits invalid_collateral_scale metric when an amount fails validation", () => { + const telemetry: Telemetry = { + record: vi.fn(), + startSpan: vi.fn(() => ({ end: vi.fn() })), + }; + const events = [ + makeEvent({ + id: "evt-bad-scale", + valueXdr: makeDepositValueXdr("GABC", 1, -1n), + }), + ]; + const { deposits, errors } = parseCollateralDepositedEvents(events, { + telemetry, + }); + expect(deposits).toHaveLength(0); + expect(errors).toHaveLength(1); + expect(telemetry.record).toHaveBeenCalledWith( + "indexer.parser.invalid_collateral_scale", + 1, + expect.objectContaining({ + parser: "collateral_deposited", + eventId: "evt-bad-scale", + }) + ); + }); + + it("rejects a plain-number amount in production but allows it in dev", () => { + // Simulate a non-i128 decode path by building the tuple with a JS + // number amount instead of a bigint (nativeToScVal picks i32/i64 for + // small numbers, not i128 — this is the "wrong width" shape). + const numberAmountXdr = nativeToScVal(["GABC", 1, 500]).toXDR("base64"); + + expect(() => + parseCollateralDepositedEvent( + makeEvent({ valueXdr: numberAmountXdr }), + { nodeEnv: "production" } + ) + ).toThrow(CollateralDepositedParseError); + + const d = parseCollateralDepositedEvent( + makeEvent({ valueXdr: numberAmountXdr }), + { nodeEnv: "development" } + ); + expect(d.amountRaw).toBe(500n); + }); +}); diff --git a/apps/indexer/src/collateralDepositedParser.ts b/apps/indexer/src/collateralDepositedParser.ts index aaf9932..cd3e4c2 100644 --- a/apps/indexer/src/collateralDepositedParser.ts +++ b/apps/indexer/src/collateralDepositedParser.ts @@ -3,9 +3,14 @@ import type { RawChainEvent } from "./types.js"; import { CollateralDepositedParseError } from "./types.js"; import { safeStringify } from "./safeJson.js"; import type { Telemetry } from "./telemetry.js"; +import { amountRawToDecimal } from "./decimalUtils.js"; const COLLATERAL_DEPOSITED_TOPIC = "collateral_deposited"; +function isProductionEnv(nodeEnv: string): boolean { + return nodeEnv === "production"; +} + function decodeScVal(xdrBase64: string): unknown { return scValToNative(xdr.ScVal.fromXDR(xdrBase64, "base64")); } @@ -48,10 +53,29 @@ export interface NormalizedCollateralDeposit { amountRaw: bigint; } -function toBigInt(value: unknown, fieldName: string, eventId: string): bigint { +function toBigInt( + value: unknown, + fieldName: string, + eventId: string, + nodeEnv: string +): bigint { if (typeof value === "bigint") return value; - if (typeof value === "number" && Number.isInteger(value)) + if (typeof value === "number" && Number.isInteger(value)) { + // scValToNative always decodes an i128 ScVal (the contract's collateral + // amount type) to a bigint, never a plain number. Seeing a number here + // means the value arrived through a non-i128 path — most likely a + // fixture or upstream decoder using the wrong scale/width — which is + // exactly the "wrong scale vs contract 7 decimals" failure mode this + // guards against. Only tolerate it outside production. + if (isProductionEnv(nodeEnv)) { + throw new CollateralDepositedParseError( + `Field "${fieldName}" decoded as a plain number, not an i128 bigint — ` + + "refusing to guess the on-chain scale in production", + eventId + ); + } return BigInt(value); + } if (typeof value === "string") { try { return BigInt(value); @@ -65,16 +89,53 @@ function toBigInt(value: unknown, fieldName: string, eventId: string): bigint { ); } +/** + * Validate `amountRaw` against the same 7-decimal / Decimal(20,8) bounds + * `decimalUtils.amountRawToDecimal` enforces everywhere else collateral + * amounts are read. Without this, a value the parser happily returns as a + * bigint could still be silently out of the scale the rest of the system + * (UserPosition/CollateralDeposit Decimal columns) assumes for it, and the + * mismatch would only surface later — e.g. as a DB error or, worse, a + * silently truncated amount — far from the event that caused it. + */ +function validateCollateralScale(amountRaw: bigint, eventId: string): void { + if (amountRaw <= 0n) { + throw new CollateralDepositedParseError( + `Field "amount" must be a positive i128, got ${amountRaw}`, + eventId + ); + } + try { + amountRawToDecimal(amountRaw); + } catch (err) { + throw new CollateralDepositedParseError( + `Field "amount" (${amountRaw}) is out of range for the 7-decimal ` + + `collateral scale: ${err instanceof Error ? err.message : String(err)}`, + eventId + ); + } +} + /** * Parse a single RawChainEvent into a NormalizedCollateralDeposit. * * Expected on-chain value: Vec [ account: str, market_id: u32, amount: i128 ] * - * @throws CollateralDepositedParseError on wrong topic or malformed payload. + * @throws CollateralDepositedParseError on wrong topic, malformed payload, + * or an amount that fails the 7-decimal collateral scale validation. */ +export interface ParseCollateralDepositedOptions { + telemetry?: Telemetry; + /** Defaults to `process.env.NODE_ENV`; override in tests only. */ + nodeEnv?: string; +} + export function parseCollateralDepositedEvent( - event: RawChainEvent + event: RawChainEvent, + options?: ParseCollateralDepositedOptions ): NormalizedCollateralDeposit { + const nodeEnv = options?.nodeEnv ?? process.env.NODE_ENV ?? "development"; + if (!isCollateralDepositedEvent(event.topicsXdr)) { throw new CollateralDepositedParseError( `Event topic is not "${COLLATERAL_DEPOSITED_TOPIC}"`, @@ -109,6 +170,9 @@ export function parseCollateralDepositedEvent( ); } + const amountRaw = toBigInt(amount, "amount", event.id, nodeEnv); + validateCollateralScale(amountRaw, event.id); + return { eventId: event.id, ledger: event.ledger, @@ -116,7 +180,7 @@ export function parseCollateralDepositedEvent( contractId: event.contractId, account, marketId: String(marketId), - amountRaw: toBigInt(amount, "amount", event.id), + amountRaw, }; } @@ -125,7 +189,7 @@ export function parseCollateralDepositedEvent( */ export function parseCollateralDepositedEvents( events: RawChainEvent[], - options?: { telemetry?: Telemetry } + options?: ParseCollateralDepositedOptions ): { deposits: NormalizedCollateralDeposit[]; errors: CollateralDepositedParseError[]; @@ -133,6 +197,7 @@ export function parseCollateralDepositedEvents( const deposits: NormalizedCollateralDeposit[] = []; const errors: CollateralDepositedParseError[] = []; const telemetry = options?.telemetry; + const nodeEnv = options?.nodeEnv ?? process.env.NODE_ENV ?? "development"; for (const event of events) { if (!isCollateralDepositedEvent(event.topicsXdr)) { @@ -145,8 +210,14 @@ export function parseCollateralDepositedEvents( continue; } try { - deposits.push(parseCollateralDepositedEvent(event)); + deposits.push(parseCollateralDepositedEvent(event, { telemetry, nodeEnv })); } catch (err) { + telemetry?.record("indexer.parser.invalid_collateral_scale", 1, { + parser: "collateral_deposited", + eventId: event.id, + contractId: event.contractId, + ledger: String(event.ledger), + }); errors.push( err instanceof CollateralDepositedParseError ? err diff --git a/docs/indexer-event-mapping.md b/docs/indexer-event-mapping.md index 4e9f264..9e4b505 100644 --- a/docs/indexer-event-mapping.md +++ b/docs/indexer-event-mapping.md @@ -61,6 +61,8 @@ When a parser encounters an event with a topic symbol it does not recognize, it **DB write:** `CollateralDeposit` row via `PrismaBatchWriter`. `amountRaw` is stored as `String` (bigint serialized) to avoid precision loss, matching `IndexedTrade.priceRaw`/`quantityRaw`. Position accounting against `UserPosition` is handled separately by a worker — `batchWriter` only persists the raw deposit for audit/reconciliation. +**Scale validation:** `collateralDepositedParser.ts` now validates every `amountRaw` against the same 7-decimal / `Decimal(20,8)` bounds `decimalUtils.amountRawToDecimal` enforces (see [Decimal/share conversion utilities](#decimalshare-conversion-utilities)), and rejects zero/negative amounts. In `NODE_ENV=production` it additionally rejects an `amount` that decodes as a plain JS `number` instead of a `bigint` — the on-chain `i128` type always decodes to `bigint`, so a `number` signals a wrong-width/wrong-scale decode path rather than a legitimate deposit. A rejection increments `indexer.parser.invalid_collateral_scale` (tags: `parser`, `eventId`, `contractId`, `ledger`). + --- ## 3. `market_resolved` From ef5881a6f39aaf5cbb60ea84d10820b589b6adcb Mon Sep 17 00:00:00 2001 From: janetpius-cmd Date: Mon, 31 Aug 2026 05:05:14 +0100 Subject: [PATCH 4/4] fix(indexer): classify retries as fatal/rate_limited/transient isTransientError() only recognized a fixed set of network error codes/ messages, so a 429 or 5xx from the Stellar RPC (which surfaces as an HTTP-status-bearing error, not a NodeJS.ErrnoException) had no correct retry path, and nothing stopped a re-thrown parse error from being retried indefinitely if its message ever collided with a transient string. Add classifyError() (fatal/rate_limited/transient), route withRetry through it, back off harder (and honor Retry-After) on 429, and never retry ResolutionParseError/TradeParseError/ CollateralDepositedParseError/MarketCreatedParseError/RetryValidationError. Claude-Session: https://claude.ai/code/session_01WR71DfNQdvTTWtuVDGBzD6 --- apps/indexer/src/retry.test.ts | 129 +++++++++++++++++++++++++++++++++ apps/indexer/src/retry.ts | 128 +++++++++++++++++++++++++++++--- docs/event-fetcher.md | 18 ++++- 3 files changed, 262 insertions(+), 13 deletions(-) diff --git a/apps/indexer/src/retry.test.ts b/apps/indexer/src/retry.test.ts index f830a07..9798861 100644 --- a/apps/indexer/src/retry.test.ts +++ b/apps/indexer/src/retry.test.ts @@ -4,7 +4,9 @@ import { withRetry, RetryValidationError, jitteredBackoffMs, + classifyError, } from "./retry.js"; +import { ResolutionParseError, TradeParseError } from "./types.js"; afterEach(() => vi.restoreAllMocks()); @@ -48,6 +50,68 @@ describe("isTransientError", () => { }); }); +// ─── classifyError ─────────────────────────────────────────────────────────── +// +// Gap this covers: previously the only classification was a boolean +// (isTransientError), with no distinction between "safe to retry +// immediately" (network blip, 5xx), "safe to retry but back off harder" +// (429), and "never retry" (parse/validation errors). A parse error +// re-thrown by a transport wrapper with a network-looking `.code`, or an +// HTTP 429/5xx from the Stellar RPC, had no correct classification path. +// These tests fail against the pre-fix module, which has no classifyError. + +describe("classifyError", () => { + it("classifies a 429 response as rate_limited", () => { + const err = Object.assign(new Error("Too Many Requests"), { + response: { status: 429 }, + }); + expect(classifyError(err)).toBe("rate_limited"); + }); + + it("classifies a bare status: 429 error as rate_limited", () => { + expect(classifyError({ status: 429 })).toBe("rate_limited"); + }); + + it("classifies a 500/502/503 response as transient", () => { + for (const status of [500, 502, 503]) { + expect(classifyError({ response: { status } })).toBe("transient"); + } + }); + + it("classifies a non-429 4xx response as fatal", () => { + expect(classifyError({ response: { status: 400 } })).toBe("fatal"); + expect(classifyError({ response: { status: 404 } })).toBe("fatal"); + }); + + it("classifies parser errors as fatal even if the message looks network-y", () => { + const err = new ResolutionParseError("socket hang up", "evt-1"); + expect(classifyError(err)).toBe("fatal"); + }); + + it("classifies TradeParseError as fatal", () => { + expect(classifyError(new TradeParseError("bad payload", "evt-1"))).toBe( + "fatal" + ); + }); + + it("classifies RetryValidationError as fatal", () => { + expect(classifyError(new RetryValidationError("bad options"))).toBe( + "fatal" + ); + }); + + it("classifies known network error codes as transient", () => { + expect( + classifyError(Object.assign(new Error("x"), { code: "ECONNRESET" })) + ).toBe("transient"); + }); + + it("classifies unknown errors as fatal", () => { + expect(classifyError(new Error("bad request"))).toBe("fatal"); + expect(classifyError("not an error")).toBe("fatal"); + }); +}); + // ─── jitteredBackoffMs ───────────────────────────────────────────────────────── describe("jitteredBackoffMs", () => { @@ -144,6 +208,71 @@ describe("withRetry", () => { expect(fn).toHaveBeenCalledTimes(1); }); + it("never retries a parse error, even with retries remaining", async () => { + const fn = vi + .fn() + .mockRejectedValue(new ResolutionParseError("bad payload", "evt-1")); + + await expect( + withRetry(fn, { maxRetries: 5, retryDelayMs: 0 }) + ).rejects.toBeInstanceOf(ResolutionParseError); + + expect(fn).toHaveBeenCalledTimes(1); + }); + + it("retries a 429 with a longer backoff than a plain transient error", async () => { + vi.spyOn(Math, "random").mockReturnValue(0); + const rateLimited = { response: { status: 429 } }; + const fn = vi.fn().mockRejectedValueOnce(rateLimited).mockResolvedValue("ok"); + + const onRetry = vi.fn(); + const result = await withRetry(fn, { + maxRetries: 1, + retryDelayMs: 100, + onRetry, + }); + + expect(result).toBe("ok"); + expect(onRetry).toHaveBeenCalledWith( + expect.objectContaining({ classification: "rate_limited", delayMs: 200 }) + ); + }); + + it("honors a Retry-After header on a 429 instead of computing backoff", async () => { + const rateLimited = { + response: { status: 429, headers: { "retry-after": "2" } }, + }; + const fn = vi.fn().mockRejectedValueOnce(rateLimited).mockResolvedValue("ok"); + const onRetry = vi.fn(); + + await withRetry(fn, { maxRetries: 1, retryDelayMs: 100, onRetry }); + + expect(onRetry).toHaveBeenCalledWith( + expect.objectContaining({ classification: "rate_limited", delayMs: 2000 }) + ); + }); + + it("retries a 503 as a plain transient failure", async () => { + const fn = vi + .fn() + .mockRejectedValueOnce({ response: { status: 503 } }) + .mockResolvedValue("ok"); + + await expect( + withRetry(fn, { maxRetries: 1, retryDelayMs: 0 }) + ).resolves.toBe("ok"); + expect(fn).toHaveBeenCalledTimes(2); + }); + + it("does not retry a non-429 4xx response", async () => { + const fn = vi.fn().mockRejectedValue({ response: { status: 400 } }); + + await expect( + withRetry(fn, { maxRetries: 3, retryDelayMs: 0 }) + ).rejects.toMatchObject({ response: { status: 400 } }); + expect(fn).toHaveBeenCalledTimes(1); + }); + it("respects maxRetries: 0 (no retries)", async () => { const fn = vi .fn() diff --git a/apps/indexer/src/retry.ts b/apps/indexer/src/retry.ts index a9649e4..89c91da 100644 --- a/apps/indexer/src/retry.ts +++ b/apps/indexer/src/retry.ts @@ -11,14 +11,88 @@ const TRANSIENT_CODES = new Set([ "socket hang up", ]); +/** + * Names of error classes that represent a *parse* failure — the payload + * itself is malformed, not the transport. Retrying these forever (the gap + * this module previously had no protection against: any Error whose + * `.code`/message happened not to be network-shaped fell through + * `isTransientError` as `false`, but nothing stopped a *future* transport + * wrapper from re-throwing a parse error with a network-looking `.code`) + * is always wrong — the bytes won't parse any differently on retry #50. + */ +const FATAL_ERROR_NAMES = new Set([ + "ResolutionParseError", + "TradeParseError", + "CollateralDepositedParseError", + "MarketCreatedParseError", + "RetryValidationError", +]); + +/** Shape of an HTTP-client error carrying a status code (axios/fetch-wrapper style). */ +interface HttpLikeError { + status?: unknown; + statusCode?: unknown; + response?: { status?: unknown; headers?: Record }; +} + +function httpStatusOf(err: unknown): number | undefined { + if (typeof err !== "object" || err === null) return undefined; + const e = err as HttpLikeError; + const candidate = e.status ?? e.statusCode ?? e.response?.status; + return typeof candidate === "number" ? candidate : undefined; +} + +export type RetryClassification = "rate_limited" | "transient" | "fatal"; + +/** + * Classify an error for retry purposes: + * - "fatal": never retry — parse errors, validation errors, 4xx (other + * than 429) responses. The request/payload is wrong; retrying can't fix it. + * - "rate_limited": HTTP 429 — retryable, but should back off more + * aggressively (and honor `Retry-After` when present) than a plain + * transient failure. + * - "transient": network-level failures and 5xx responses — safe to + * retry with standard exponential backoff. + */ +export function classifyError(err: unknown): RetryClassification { + if (err instanceof Error && FATAL_ERROR_NAMES.has(err.name)) { + return "fatal"; + } + + const status = httpStatusOf(err); + if (status === 429) return "rate_limited"; + if (typeof status === "number") { + return status >= 500 ? "transient" : "fatal"; + } + + if (!(err instanceof Error)) return "fatal"; + const code = (err as NodeJS.ErrnoException).code ?? ""; + return TRANSIENT_CODES.has(code) || TRANSIENT_CODES.has(err.message) + ? "transient" + : "fatal"; +} + /** * Returns true when the error looks like a transient network failure - * that is safe to retry. + * that is safe to retry. Retained for backwards compatibility with + * existing callers; prefer `classifyError` for new code since it also + * distinguishes rate limiting (429) from other transient failures and + * never classifies a parse/validation error as retryable. */ export function isTransientError(err: unknown): boolean { - if (!(err instanceof Error)) return false; - const code = (err as NodeJS.ErrnoException).code ?? ""; - return TRANSIENT_CODES.has(code) || TRANSIENT_CODES.has(err.message); + const classification = classifyError(err); + return classification === "transient" || classification === "rate_limited"; +} + +/** Extract a `Retry-After` (seconds) header value from an HTTP-like error, if present. */ +function retryAfterMs(err: unknown): number | undefined { + if (typeof err !== "object" || err === null) return undefined; + const headers = (err as HttpLikeError).response?.headers; + const raw = headers?.["retry-after"] ?? headers?.["Retry-After"]; + const seconds = typeof raw === "string" ? Number(raw) : undefined; + return seconds !== undefined && Number.isFinite(seconds) + ? seconds * 1000 + : undefined; } /** @@ -50,6 +124,23 @@ export interface RetryOptions { maxRetries: number; /** Base delay in ms; doubles on each attempt (exponential backoff). */ retryDelayMs: number; + /** + * Multiplier applied to the base backoff for "rate_limited" (429) + * classifications when the response carries no `Retry-After` header. + * Rate limiting is a signal to slow down more than a bare network blip. + */ + rateLimitBackoffMultiplier?: number; + /** + * Optional callback invoked before each retry sleep, for + * metrics/correlation-id logging. Never receives the error's message — + * only the classification and attempt number — so callers can log + * safely without risking secrets leaking through error text. + */ + onRetry?: (info: { + attempt: number; + classification: RetryClassification; + delayMs: number; + }) => void; } export class RetryValidationError extends Error { @@ -72,27 +163,46 @@ function validateRetryOptions(options: RetryOptions): void { } /** - * Execute `fn` with bounded retries on transient errors. + * Execute `fn` with bounded retries, classifying failures via + * `classifyError` instead of a single transient/non-transient split: + * + * - "fatal" (parse errors, validation errors, non-429 4xx) never retries, + * regardless of remaining attempts — this is what stops a malformed + * payload from being retried forever. + * - "rate_limited" (429) retries with a longer backoff (honoring + * `Retry-After` when the response provides it). + * - "transient" (network failures, 5xx) retries with standard + * exponential backoff, as before. * * @throws {RetryValidationError} When options are invalid (statusCode 400). - * @throws The last error when retries are exhausted or the error is non-transient. + * @throws The last error when retries are exhausted or the error is fatal. */ export async function withRetry( fn: () => Promise, options: RetryOptions ): Promise { validateRetryOptions(options); - const { maxRetries, retryDelayMs } = options; + const { maxRetries, retryDelayMs, rateLimitBackoffMultiplier = 4, onRetry } = + options; for (let attempt = 0; attempt <= maxRetries; attempt++) { try { return await fn(); } catch (err) { const isLast = attempt === maxRetries; - if (isLast || !isTransientError(err)) { + const classification = classifyError(err); + if (isLast || classification === "fatal") { throw err; } - await sleep(jitteredBackoffMs(retryDelayMs, attempt)); + + const delayMs = + classification === "rate_limited" + ? retryAfterMs(err) ?? + jitteredBackoffMs(retryDelayMs * rateLimitBackoffMultiplier, attempt) + : jitteredBackoffMs(retryDelayMs, attempt); + + onRetry?.({ attempt, classification, delayMs }); + await sleep(delayMs); } } diff --git a/docs/event-fetcher.md b/docs/event-fetcher.md index 157182d..0b050ab 100644 --- a/docs/event-fetcher.md +++ b/docs/event-fetcher.md @@ -30,10 +30,20 @@ events it returns. ## Retry strategy -Retries use exponential back-off: `retryDelayMs * 2^attempt`. Only errors identified as -transient by `isTransientError()` (from `retry.ts`) trigger a retry; all other errors are -thrown immediately. After `maxRetries` consecutive transient failures the last error is -re-thrown. +`retry.ts` classifies every failure via `classifyError()` into one of three buckets instead +of a single transient/non-transient split: + +| Classification | Examples | Behavior | +| --------------- | ------------------------------------------- | ---------------------------------------------------------------------- | +| `fatal` | Parse errors (`*ParseError`), non-429 4xx | Never retried, even with attempts remaining — the payload is wrong. | +| `rate_limited` | HTTP 429 | Retried with `Retry-After` (if present) or `retryDelayMs * rateLimitBackoffMultiplier` (default `4`) exponential backoff. | +| `transient` | Network error codes (`ECONNRESET`, ...), 5xx | Retried with standard `retryDelayMs * 2^attempt` exponential backoff. | + +`isTransientError()` is kept for backwards compatibility (`true` for `transient` or +`rate_limited`) but new callers should use `classifyError()` directly. This split exists +specifically so a parse error is never retried forever — regardless of what its `.code` or +message happens to look like — while a genuine 429/5xx from the Stellar RPC still backs off +and recovers. After `maxRetries` consecutive retryable failures the last error is re-thrown. ## Telemetry