-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| jest.mock("../../../../defi/src/utils/discord", () => ({ | ||
| sendMessage: jest.fn(() => Promise.resolve()), | ||
| })); | ||
|
|
||
| import { addToDBWritesList } from "./database"; | ||
| import { sendMessage } from "../../../../defi/src/utils/discord"; | ||
| import type { Write } from "./dbInterfaces"; | ||
|
|
||
| const mockSendMessage = sendMessage as jest.Mock; | ||
|
|
||
| beforeEach(() => { | ||
| mockSendMessage.mockClear(); | ||
| 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 only warns once per process", () => { | ||
| const writes: Write[] = []; | ||
| for (let i = 0; i < 5; i++) { | ||
| addToDBWritesList( | ||
| writes, | ||
| "ethereum", | ||
| `0xF${i}`, | ||
| "garbage" as any, | ||
| 18, | ||
| "X", | ||
| TS, | ||
| "dedup-adapter", | ||
| 0.99, | ||
| ); | ||
| } | ||
| // 5 writes happened; only 1 warn was sent. | ||
| expect(writes).toHaveLength(5); | ||
| expect(mockSendMessage).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,39 @@ export async function getTokenAndRedirectDataMap( | |
| return map; | ||
| } | ||
|
|
||
| const warnedNumericKeys = new Set<string>(); | ||
|
|
||
| function warnInvalidNumericField( | ||
| value: unknown, | ||
| field: string, | ||
| adapter: string, | ||
| reason: string, | ||
| ): void { | ||
| const key = `${adapter}:${field}:${reason}`; | ||
| if (warnedNumericKeys.has(key)) return; | ||
| warnedNumericKeys.add(key); | ||
| const msg = `coins: addToDBWritesList[${adapter}] invalid ${field} (${reason}): ${JSON.stringify(value)} (${typeof value}). 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: boolean, | ||
| ): number | undefined { | ||
| if (value === undefined || value === null) { | ||
| if (!allowUndefined) warnInvalidNumericField(value, field, adapter, "missing"); | ||
| return allowUndefined ? undefined : (Number(value) as number); | ||
| } | ||
|
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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NaN still propagates to write records for When
Since the stated goal is "prevent string-price regressions", consider extending the 🤖 Prompt for AI Agents |
||
|
|
||
| export function addToDBWritesList( | ||
| writes: Write[], | ||
| chain: string, | ||
|
|
@@ -143,6 +176,9 @@ export function addToDBWritesList( | |
| confidence: number, | ||
| redirect: string | undefined = undefined, | ||
| ) { | ||
| 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()}` | ||
|
|
@@ -151,34 +187,34 @@ export function addToDBWritesList( | |
| writes.push({ | ||
| SK: 0, | ||
| PK, | ||
| price, | ||
| price: priceNum, | ||
| symbol, | ||
| decimals: Number(decimals), | ||
| decimals: decimalsNum, | ||
| redirect, | ||
| timestamp: getCurrentUnixTimestamp(), | ||
| adapter, | ||
| confidence: Number(confidence), | ||
| confidence: confidenceNum, | ||
| }); | ||
| } else if (timestamp == 0) { | ||
| writes.push( | ||
| ...[ | ||
| { | ||
| SK: getCurrentUnixTimestamp(), | ||
| PK, | ||
| price, | ||
| price: priceNum, | ||
| adapter, | ||
| confidence: Number(confidence), | ||
| confidence: confidenceNum, | ||
| }, | ||
| { | ||
| SK: 0, | ||
| PK, | ||
| price, | ||
| price: priceNum, | ||
| symbol, | ||
| decimals: Number(decimals), | ||
| decimals: decimalsNum, | ||
| redirect, | ||
| timestamp: getCurrentUnixTimestamp(), | ||
| adapter, | ||
| confidence: Number(confidence), | ||
| confidence: confidenceNum, | ||
| }, | ||
| ], | ||
| ); | ||
|
|
@@ -190,9 +226,9 @@ export function addToDBWritesList( | |
| SK: timestamp, | ||
| PK, | ||
| redirect, | ||
| price, | ||
| price: priceNum, | ||
| adapter, | ||
| confidence: Number(confidence), | ||
| confidence: confidenceNum, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
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?