Skip to content

coins: runtime numeric guardrails + spec compliance test suite #11802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
waynebruce0x wants to merge 3 commits into
base: master
Choose a base branch
from
Open

coins: runtime numeric guardrails + spec compliance test suite #11802

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14909c7
coins: runtime numeric guardrails + spec compliance test suite
waynebruce0x Apr 22, 2026
6b3b1c9
Merge master into coins-numeric-guardrails; address review comments
waynebruce0x May 6, 2026
0ed5550
comments
waynebruce0x May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 8 additions & 18 deletions coins/src/adapters/rwa/backed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,24 +107,14 @@ async function getTokenPrices(chain: string, timestamp: number) {
})),
);

writes.map((w: Write) => {
writes.push({
...w,
PK: `asset#avax:${w.PK.substring(w.PK.indexOf(":") + 1)}`,
});
writes.push({
...w,
PK: `asset#base:${w.PK.substring(w.PK.indexOf(":") + 1)}`,
});
writes.push({
...w,
PK: `asset#polygon:${w.PK.substring(w.PK.indexOf(":") + 1)}`,
});
writes.push({
...w,
PK: `asset#xdai:${w.PK.substring(w.PK.indexOf(":") + 1)}`,
});
});
const extraChains = ["avax", "base", "polygon", "xdai"];
const originals = writes.slice();
for (const w of originals) {
const addressPart = w.PK.substring(w.PK.indexOf(":") + 1);
for (const extraChain of extraChains) {
writes.push({ ...w, PK: `asset#${extraChain}:${addressPart}` });

@g1nt0ki g1nt0ki May 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not map the token on the other chains instead of writing the price everytime?

}
}
return writes;
}

Expand Down
168 changes: 168 additions & 0 deletions coins/src/adapters/utils/database.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
jest.mock("../../../../defi/src/utils/discord", () => ({
sendMessage: jest.fn(() => Promise.resolve()),
}));

import { addToDBWritesList, __resetNumericWarningsForTests } from "./database";
import { sendMessage } from "../../../../defi/src/utils/discord";
import type { Write } from "./dbInterfaces";

const mockSendMessage = sendMessage as jest.Mock;

beforeEach(() => {
mockSendMessage.mockClear();
__resetNumericWarningsForTests();
process.env.STALE_COINS_ADAPTERS_WEBHOOK = "https://discord.test/webhook";
});

afterAll(() => {
delete process.env.STALE_COINS_ADAPTERS_WEBHOOK;
});

const TS = 1700000000;

describe("addToDBWritesList numeric guardrail", () => {
test("valid number inputs: write goes through with exact types, no warning", () => {
const writes: Write[] = [];
addToDBWritesList(writes, "ethereum", "0xAAA", 1.23, 18, "AAA", TS, "ok-adapter", 0.99);
expect(writes).toHaveLength(1);
expect(typeof writes[0].price).toBe("number");
expect(writes[0].price).toBe(1.23);
expect(writes[0].confidence).toBe(0.99);
expect(mockSendMessage).not.toHaveBeenCalled();
});

test("string price: coerced to number and written (does not throw)", () => {
const writes: Write[] = [];
addToDBWritesList(
writes,
"ethereum",
"0xBBB",
"42.5" as any,
18,
"BBB",
TS,
"str-price-adapter",
0.99,
);
expect(writes).toHaveLength(1);
expect(typeof writes[0].price).toBe("number");
expect(writes[0].price).toBe(42.5);
// String-but-numeric isn't a "non-finite" case, so no warn is expected.
expect(mockSendMessage).not.toHaveBeenCalled();
});

test("unparseable price: write proceeds with NaN, Discord warn fires once", () => {
const writes: Write[] = [];
addToDBWritesList(
writes,
"ethereum",
"0xCCC",
"not-a-number" as any,
18,
"CCC",
TS,
"bad-price-adapter",
0.99,
);
// Write happened (no throw) — preserves pre-PR behaviour.
expect(writes).toHaveLength(1);
expect(Number.isNaN(writes[0].price)).toBe(true);
// Warn fired to Discord.
expect(mockSendMessage).toHaveBeenCalledTimes(1);
expect(mockSendMessage.mock.calls[0][0]).toMatch(/bad-price-adapter/);
expect(mockSendMessage.mock.calls[0][0]).toMatch(/price/);
});

test("NaN decimals: write proceeds, warn fires", () => {
const writes: Write[] = [];
addToDBWritesList(
writes,
"ethereum",
"0xDDD",
1.0,
NaN,
"DDD",
TS,
"nan-decimals-adapter",
0.99,
);
expect(writes).toHaveLength(1);
expect(mockSendMessage).toHaveBeenCalledTimes(1);
expect(mockSendMessage.mock.calls[0][0]).toMatch(/decimals/);
});

test("undefined confidence: write proceeds with NaN confidence, warn fires", () => {
const writes: Write[] = [];
addToDBWritesList(
writes,
"ethereum",
"0xEEE",
1.0,
18,
"EEE",
TS,
"undef-conf-adapter",
undefined as any,
);
expect(writes).toHaveLength(1);
expect(Number.isNaN(writes[0].confidence)).toBe(true);
expect(mockSendMessage).toHaveBeenCalledTimes(1);
expect(mockSendMessage.mock.calls[0][0]).toMatch(/confidence/);
});

test("dedup: same adapter+field+reason emits on threshold boundaries (1, 10, ...)", () => {
const writes: Write[] = [];
// 5 calls — only the first crosses a threshold, so exactly one Discord msg.
for (let i = 0; i < 5; i++) {
addToDBWritesList(
writes,
"ethereum",
`0xF${i}`,
"garbage" as any,
18,
"X",
TS,
"dedup-adapter",
0.99,
);
}
expect(writes).toHaveLength(5);
expect(mockSendMessage).toHaveBeenCalledTimes(1);

// 5 more calls bring the count to 10 — second threshold, second Discord msg.
for (let i = 5; i < 10; i++) {
addToDBWritesList(
writes,
"ethereum",
`0xF${i}`,
"garbage" as any,
18,
"X",
TS,
"dedup-adapter",
0.99,
);
}
expect(writes).toHaveLength(10);
expect(mockSendMessage).toHaveBeenCalledTimes(2);
expect(mockSendMessage.mock.calls[1][0]).toMatch(/seen 10 time/);
});

test("webhook unset: console.error still fires but no Discord call", () => {
delete process.env.STALE_COINS_ADAPTERS_WEBHOOK;
const writes: Write[] = [];
addToDBWritesList(
writes,
"ethereum",
"0xF99",
1.0,
18,
"X",
TS,
"no-webhook-adapter",
undefined as any,
);
expect(writes).toHaveLength(1);
expect(mockSendMessage).not.toHaveBeenCalled();
});
});
69 changes: 62 additions & 7 deletions coins/src/adapters/utils/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,52 @@ export async function getTokenAndRedirectDataMap(
return map;
}

// Tracks how many times each (adapter, field, reason) tuple has fired.
// We emit on threshold boundaries (1, 10, 100, ...) so a chronically broken
// adapter doesn't go silent for the rest of the pod's lifetime after the first
// warn, while still avoiding one-message-per-row spam.
const numericWarningCounts = new Map<string, number>();
const WARN_THRESHOLDS = [1, 10, 100, 1000, 10000];

export function __resetNumericWarningsForTests(): void {
numericWarningCounts.clear();
}

function warnInvalidNumericField(
value: unknown,
field: string,
adapter: string,
reason: string,
): void {
const key = `${adapter}:${field}:${reason}`;
const next = (numericWarningCounts.get(key) ?? 0) + 1;
numericWarningCounts.set(key, next);
if (!WARN_THRESHOLDS.includes(next)) return;
const msg = `coins: addToDBWritesList[${adapter}] invalid ${field} (${reason}): ${JSON.stringify(value)} (${typeof value}); seen ${next} time(s) since startup. Write proceeds with coerced value; please fix the adapter to pass a finite number.`;
console.error(msg);
if (process.env.STALE_COINS_ADAPTERS_WEBHOOK) {
sendMessage(msg, process.env.STALE_COINS_ADAPTERS_WEBHOOK, false).catch(() => {});
}
}

function coerceNumericField(value: unknown, field: string, adapter: string, allowUndefined: true): number | undefined;
function coerceNumericField(value: unknown, field: string, adapter: string, allowUndefined: false): number;
function coerceNumericField(
value: unknown,
field: string,
adapter: string,
allowUndefined: boolean,
): number | undefined {
if (value === undefined || value === null) {
if (allowUndefined) return undefined;
warnInvalidNumericField(value, field, adapter, "missing");
return NaN;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const n = typeof value === "number" ? value : Number(value);
if (!Number.isFinite(n)) warnInvalidNumericField(value, field, adapter, "non-finite");
return n;
}
Comment on lines +163 to +177

@coderabbitai coderabbitai Bot Apr 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

NaN still propagates to write records for decimals and confidence.

When value can't be coerced, coerceNumericField returns NaN (or Number(null) === 0). The PR is intentionally warn-only for price, and the downstream guard in batchWriteWithAlerts (Line 473: isFinite(i.price) || i.redirect) filters NaN-priced items without a redirect — but:

  • NaN decimals is not filtered anywhere I can see and will reach DynamoDB/Kafka/ClickHouse.
  • NaN confidence does get dropped indirectly by filterWritesWithLowConfidence (NaN > threshold is false), but only if that filter runs upstream of batchWriteWithAlerts for the given adapter — not all adapter paths are guaranteed to call it.
  • A write carrying a redirect with a NaN price will still slip through the isFinite || redirect filter.

Since the stated goal is "prevent string-price regressions", consider extending the batchWriteWithAlerts filter at Line 473 to also require Number.isFinite(i.decimals ?? 0) (or drop the attribute when non-finite before write) and to check price finiteness even when a redirect is present. If that's intentionally deferred to keep this PR warn-only, please add a TODO referencing the follow-up.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@coins/src/adapters/utils/database.ts` around lines 152 - 165,
coerceNumericField currently allows NaN (and coerces null to 0) for numeric
fields like decimals and confidence, so update the downstream write filter in
batchWriteWithAlerts (and/or sanitize outputs from coerceNumericField) to reject
or strip non-finite decimals and confidence before writing: ensure
batchWriteWithAlerts requires Number.isFinite(i.price) regardless of i.redirect
and add checks like Number.isFinite(i.decimals ?? 0) (or delete i.decimals when
non-finite) and similarly handle i.confidence, or if you intend to keep this PR
warn-only, add a TODO referencing a follow-up to enforce finite checks and strip
non-finite numeric fields; reference functions coerceNumericField,
batchWriteWithAlerts, and filterWritesWithLowConfidence when making the change.


export function addToDBWritesList(
writes: Write[],
chain: string,
Expand All @@ -142,22 +188,31 @@ export function addToDBWritesList(
confidence: number,
redirect: string | undefined = undefined,
) {
// NOTE: warn-only by design — coerceNumericField may return NaN for
// decimals/confidence, which propagates into the write record. The downstream
// `batchWriteWithAlerts` filter (search for `isFinite(i.price)`) rejects
// non-finite-priced rows without a redirect; non-finite decimals/confidence
// are not yet filtered, matching pre-PR behaviour. Tightening that filter
// (and dropping non-finite numeric fields rather than writing them) is a
// follow-up tracked in the PR description for #11802.
const priceNum = coerceNumericField(price, "price", adapter, true);
const decimalsNum = coerceNumericField(decimals, "decimals", adapter, true);
const confidenceNum = coerceNumericField(confidence, "confidence", adapter, false);
const PK: string =
chain == "coingecko"
? `coingecko#${token.toLowerCase()}`
: `asset#${chain}:${lowercase(token, chain)}`;
const priceNum = price == null ? undefined : Number(price);
if (redirect && timestamp == 0) {
writes.push({
SK: 0,
PK,
price: priceNum,
symbol,
decimals: Number(decimals),
decimals: decimalsNum,
redirect,
timestamp: getCurrentUnixTimestamp(),
adapter,
confidence: Number(confidence),
confidence: confidenceNum,
});
} else if (timestamp == 0) {
writes.push(
Expand All @@ -167,18 +222,18 @@ export function addToDBWritesList(
PK,
price: priceNum,
adapter,
confidence: Number(confidence),
confidence: confidenceNum,
},
{
SK: 0,
PK,
price: priceNum,
symbol,
decimals: Number(decimals),
decimals: decimalsNum,
redirect,
timestamp: getCurrentUnixTimestamp(),
adapter,
confidence: Number(confidence),
confidence: confidenceNum,
},
],
);
Expand All @@ -192,7 +247,7 @@ export function addToDBWritesList(
redirect,
price: priceNum,
adapter,
confidence: Number(confidence),
confidence: confidenceNum,
});
}
}
Expand Down
Loading