coins: runtime numeric guardrails + spec compliance test suite#11802
Open
waynebruce0x wants to merge 3 commits into
Open
coins: runtime numeric guardrails + spec compliance test suite#11802waynebruce0x wants to merge 3 commits into
waynebruce0x wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughNormalizes and coerces numeric price fields across adapters, storage, and serving; adds warning/throttling for invalid numeric inputs with tests; expands token mappings; refactors RWA cross-chain write generation; and adds conditional live API integration tests. ChangesPrice Validation & Mappings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Follow-up to #11797. That PR coerces price at the DB-write helper; the gaps addressed here are (a) other write/read paths, (b) no runtime guard against future regressions the TypeScript types cannot catch (adapter input data is typed as `any`), and (c) no live test coverage against the published OpenAPI spec. - addToDBWritesList: new `coerceNumericField` helper runs on every call and throws loudly when price/decimals/confidence are not finite numbers. Single chokepoint so adapters cannot reintroduce the Ondo string-price bug by bypassing Number(). - servingLayer + bootstrapRedis: coerce price on the Redis-cache write path and skip rows whose price is not a finite number. Before this, stale string-typed rows in ClickHouse would propagate to clients through the cache even after write-time fixes. - servingLayer: coerce price on the ClickHouse fallback read path for parity with the Redis read path (already used parseFloat). - rwa/backed.ts: replace writes.map that mutated writes during its own iteration with an explicit `writes.slice()` + nested loop over extra chains. Same behaviour (Array#map snapshots length), obviously correct at a glance. - coins/src/api-spec.test.ts: new 33-test live integration suite that validates responses on all seven spec'd coins endpoints against the OpenAPI spec. Configurable via COINS_API_BASE (default https://coins.llama.fi) and SKIP_LIVE=1. Covers happy paths, type assertions, unknown/malformed coins, mixed-case, timestamp boundaries, batchHistorical duplicate-casing regression, and explicit regression guards for the string-price bug in #11797. Companion spec changes in DefiLlama/api-docs#15 (mark decimals optional, document confidence) — required for this suite to pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
waynebruce0x
force-pushed
the
waynebruce0x/coins-numeric-guardrails
branch
from
39146f1 to
14909c7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
coins/src/adapters/utils/database.test.ts (2)
11-18: Test isolation is implicitly coupled to the module-level dedup set.
warnedNumericKeysindatabase.tsis module-scoped and is not reset inbeforeEach— every test here passes only because each uses a unique adapter name (ok-adapter,bad-price-adapter,nan-decimals-adapter, …). A future maintainer adding a test that reuses one of these adapter strings will see zero warnings and a false green test.Two low-friction ways to harden this:
jest.isolateModules(...)around the import so each test gets a fresh module instance, or- Export a
__resetNumericWarningsForTests()helper fromdatabase.tsand call it inbeforeEach.Option 2 sketch
// database.ts +export function __resetNumericWarningsForTests() { + warnedNumericKeys.clear(); +}// database.test.ts beforeEach(() => { mockSendMessage.mockClear(); + __resetNumericWarningsForTests(); process.env.STALE_COINS_ADAPTERS_WEBHOOK = "https://discord.test/webhook"; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coins/src/adapters/utils/database.test.ts` around lines 11 - 18, Tests share module-scoped warnedNumericKeys in database.ts causing implicit coupling; fix by either wrapping the import that uses database.ts in jest.isolateModules(...) inside each test so each test gets a fresh module instance, or add and export a test-only function named __resetNumericWarningsForTests (or similar) from database.ts that clears warnedNumericKeys and call that function in beforeEach of database.test.ts; reference the module symbol warnedNumericKeys and the new helper __resetNumericWarningsForTests (or the jest.isolateModules wrapper) when making the change so future tests are isolated.
93-110: Consider also asserting the "webhook unset" branch.
warnInvalidNumericFieldonly callssendMessagewhenprocess.env.STALE_COINS_ADAPTERS_WEBHOOKis truthy. None of the tests cover the env-unset path, so a future regression that unconditionally callssendMessage(e.g. someone removes the env guard) wouldn't be caught. A one-line test that unsets the env and assertsmockSendMessagewas not called would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coins/src/adapters/utils/database.test.ts` around lines 93 - 110, Add a test that verifies the "webhook unset" branch by clearing process.env.STALE_COINS_ADAPTERS_WEBHOOK, calling addToDBWritesList (the same way as the existing "undefined confidence" test) and asserting mockSendMessage was not called; ensure you use the same inputs as the existing test (e.g., adapter "undef-conf-adapter") so warnInvalidNumericField path is exercised, and restore or isolate the env var after the test to avoid cross-test pollution.coins/src/adapters/utils/database.ts (2)
179-181: Non-null assertion masks the fact thatconfidenceNumcan beNaN.
coerceNumericField(confidence, "confidence", adapter, false)!— the!eliminatesundefinedfrom the type, which is accurate (givenallowUndefined=false), butconfidenceNumcan still beNaN. Since the type is nownumber, downstream consumers won't be warned by the type system that a finiteness check is still required. Consider either:
- Returning a discriminated result (
{ ok: true, value } | { ok: false }) and letting the caller decide, or- Replacing the
!with an explicitNumber.isFiniteguard and eitherreturn-ing or substituting a sentinel when invalid.Lower priority since the PR is explicitly warn-only, but it would harden the function signature for future callers.
🤖 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 179 - 181, The current non-null assertion on confidenceNum (const confidenceNum = coerceNumericField(confidence, "confidence", adapter, false)!) hides that the value can be NaN; update the callsite or the coerceNumericField API so callers see when the numeric value is invalid: either change coerceNumericField to return a discriminated result type (e.g., { ok: true, value: number } | { ok: false, reason? }) and handle the failure before using confidenceNum, or remove the `!` and add an explicit Number.isFinite(confidenceNum) guard after the call (and return/substitute a sentinel or throw/log) so downstream code using confidenceNum (priceNum/decimalsNum consumers) cannot assume a finite number.
134-150: Dedup is per-process-lifetime and uncapped — fine operationally, but silences long-running breakage.
warnedNumericKeysnever resets, so once an adapter has warned for(field, reason)you get no further signal for the rest of the pod's uptime. In practice pods get rotated often enough that this is fine, but a broken adapter that first trips on startup warns once and then disappears from Discord for the pod's remaining N hours — ops will underestimate frequency.Two low-cost improvements to consider:
- Periodically reset the set (e.g. on a daily interval) so chronic failures keep surfacing.
- Include a counter in the message (
seen 147 times since startup) by tracking aMap<string, number>instead of aSet<string>and only re-emitting on threshold boundaries (1, 10, 100, 1000…).Also note the set has no upper bound; size is bounded by
adapters × fields × reasonswhich is small, so not a memory concern — mentioning only for future-proofing.🤖 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 134 - 150, warnedNumericKeys is an unbounded Set used by warnInvalidNumericField so each unique adapter:field:reason only logs once per process lifetime; change it to a Map<string, number> (keep the name) and increment the count on each occurrence, include the count in the console and webhook message (e.g., "seen N times since startup"), and only emit logs when the count hits threshold boundaries (1, 10, 100, 1000, …) to avoid noisy repeats; additionally add a periodic reset (e.g., setInterval clearing the Map once per day) so chronic failures continue to resurface across long-lived processes while preserving the existing sendMessage/webhook behavior and the function signature of warnInvalidNumericField.coins/src/utils/servingLayer.ts (1)
155-160: Good — symmetric with the write-path guard.Coercing
price.priceand dropping non-finite entries on the CH fallback prevents legacy string rows from leaking through the read path. Consider applying the same guard to the Redis read path at Line 103 for consistency — it still usesparseFloat(price.price)without aNumber.isFinitecheck, so any pre-existing cache entry with a bad value (e.g. a serializednull/"NaN"price, or a cache row written before this PR deployed) would still produce a NaNpricefield in the response.Suggested defensive change at Line 103
- response[coin] = { - decimals: mapping.decimals ?? undefined, - symbol: mapping.symbol ?? "", - price: parseFloat(price.price), - timestamp: typeof price.timestamp === "string" ? Math.floor(new Date(price.timestamp).getTime() / 1000) : price.timestamp, - confidence: price.confidence ?? undefined, - }; - hits++; + const priceNum = typeof price.price === "number" ? price.price : Number(price.price); + if (!Number.isFinite(priceNum)) return; + response[coin] = { + decimals: mapping.decimals ?? undefined, + symbol: mapping.symbol ?? "", + price: priceNum, + timestamp: typeof price.timestamp === "string" ? Math.floor(new Date(price.timestamp).getTime() / 1000) : price.timestamp, + confidence: price.confidence ?? undefined, + }; + hits++;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coins/src/utils/servingLayer.ts` around lines 155 - 160, The Redis read path currently uses parseFloat(price.price) and can produce NaN for bad cached values; mirror the CH fallback by coercing price.price to a Number (e.g., typeof check or Number()) and then guard with Number.isFinite before assigning to response[coin]; if the coerced value is not finite, skip the entry (same behavior as the CH branch that sets decimals/symbol and price only when priceNum is finite). Target the code that calls parseFloat(price.price) and the response[coin] assignment to apply this defensive check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coins/src/adapters/utils/database.ts`:
- Around line 158-161: The code path that handles value === undefined || value
=== null conflates both cases and then returns allowUndefined ? undefined :
Number(value), which makes null become 0 and undefined become NaN inconsistently
and also changes behavior when allowUndefined=true; change the branch to handle
null and undefined explicitly: call warnInvalidNumericField(value, field,
adapter, "missing") as currently done, but then if value === undefined return
allowUndefined ? undefined : Number(undefined) (preserving NaN), and if value
=== null return allowUndefined ? undefined : Number(null) (preserving 0) — i.e.,
split the combined condition into two branches using the existing symbols value,
allowUndefined, field, adapter and warnInvalidNumericField so behaviour is
deterministic and explicit.
- Around line 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.
In `@coins/src/api-spec.test.ts`:
- Around line 126-137: The test "SPYon (Ondo equity) — price must be a number,
not a string" silently passes because of the early return if (!coin) return; —
make the guard deterministic by ensuring the coin is queried over a wider window
or by asserting presence: modify the GET request parameters used by
get(`${P}/${SPYON}`) to include a larger searchWidth (e.g. "30d") or replace the
early return with an explicit expect(coin).toBeDefined() so the test fails when
SPYON is absent; update references in the test to P, SPYON, coin, and the get
call accordingly.
- Around line 213-224: The test "same coin in two casings — regression for
duplicate-entry bug (PR test C)" currently only checks each returned coin has ≤1
price entry but doesn't ensure the two casings collapsed; update the test to
also assert that the server returned a single canonical key (inspect
Object.keys(r.data.coins).length) or verify the union of timestamps across all
entries has no duplicates (collect timestamps from r.data.coins values and
assert uniqueness) — reference variables/checkpoints: checksum, lower (USDC),
and r.data.coins to locate where to add the extra assertion.
- Around line 243-257: The test "happy path — prices[] with numeric fields"
currently calls assertFiniteNumber on coin.decimals and coin.confidence
unconditionally; change it to only call assertFiniteNumber for each field when
that property exists on coin (use the "in" check like assertPriceEntry does),
i.e., guard assertFiniteNumber(coin.decimals, "coin.decimals") and
assertFiniteNumber(coin.confidence, "coin.confidence") with checks such as "if
('decimals' in coin) ..." and "if ('confidence' in coin) ...", leaving the rest
of the price array assertions (assertPriceEntry and checks of coin.prices)
intact.
---
Nitpick comments:
In `@coins/src/adapters/utils/database.test.ts`:
- Around line 11-18: Tests share module-scoped warnedNumericKeys in database.ts
causing implicit coupling; fix by either wrapping the import that uses
database.ts in jest.isolateModules(...) inside each test so each test gets a
fresh module instance, or add and export a test-only function named
__resetNumericWarningsForTests (or similar) from database.ts that clears
warnedNumericKeys and call that function in beforeEach of database.test.ts;
reference the module symbol warnedNumericKeys and the new helper
__resetNumericWarningsForTests (or the jest.isolateModules wrapper) when making
the change so future tests are isolated.
- Around line 93-110: Add a test that verifies the "webhook unset" branch by
clearing process.env.STALE_COINS_ADAPTERS_WEBHOOK, calling addToDBWritesList
(the same way as the existing "undefined confidence" test) and asserting
mockSendMessage was not called; ensure you use the same inputs as the existing
test (e.g., adapter "undef-conf-adapter") so warnInvalidNumericField path is
exercised, and restore or isolate the env var after the test to avoid cross-test
pollution.
In `@coins/src/adapters/utils/database.ts`:
- Around line 179-181: The current non-null assertion on confidenceNum (const
confidenceNum = coerceNumericField(confidence, "confidence", adapter, false)!)
hides that the value can be NaN; update the callsite or the coerceNumericField
API so callers see when the numeric value is invalid: either change
coerceNumericField to return a discriminated result type (e.g., { ok: true,
value: number } | { ok: false, reason? }) and handle the failure before using
confidenceNum, or remove the `!` and add an explicit
Number.isFinite(confidenceNum) guard after the call (and return/substitute a
sentinel or throw/log) so downstream code using confidenceNum
(priceNum/decimalsNum consumers) cannot assume a finite number.
- Around line 134-150: warnedNumericKeys is an unbounded Set used by
warnInvalidNumericField so each unique adapter:field:reason only logs once per
process lifetime; change it to a Map<string, number> (keep the name) and
increment the count on each occurrence, include the count in the console and
webhook message (e.g., "seen N times since startup"), and only emit logs when
the count hits threshold boundaries (1, 10, 100, 1000, …) to avoid noisy
repeats; additionally add a periodic reset (e.g., setInterval clearing the Map
once per day) so chronic failures continue to resurface across long-lived
processes while preserving the existing sendMessage/webhook behavior and the
function signature of warnInvalidNumericField.
In `@coins/src/utils/servingLayer.ts`:
- Around line 155-160: The Redis read path currently uses
parseFloat(price.price) and can produce NaN for bad cached values; mirror the CH
fallback by coercing price.price to a Number (e.g., typeof check or Number())
and then guard with Number.isFinite before assigning to response[coin]; if the
coerced value is not finite, skip the entry (same behavior as the CH branch that
sets decimals/symbol and price only when priceNum is finite). Target the code
that calls parseFloat(price.price) and the response[coin] assignment to apply
this defensive check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b37a90e-849b-402f-9025-b6d8e51048e7
📒 Files selected for processing (6)
coins/src/adapters/rwa/backed.tscoins/src/adapters/utils/database.test.tscoins/src/adapters/utils/database.tscoins/src/api-spec.test.tscoins/src/scripts/bootstrapRedis.tscoins/src/utils/servingLayer.ts
Comment on lines
+152
to
+165
| 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); | ||
| } | ||
| const n = typeof value === "number" ? value : Number(value); | ||
| if (!Number.isFinite(n)) warnInvalidNumericField(value, field, adapter, "non-finite"); | ||
| return n; | ||
| } |
Contributor
There was a problem hiding this comment.
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
decimalsis not filtered anywhere I can see and will reach DynamoDB/Kafka/ClickHouse. - NaN
confidencedoes get dropped indirectly byfilterWritesWithLowConfidence(NaN > threshold is false), but only if that filter runs upstream ofbatchWriteWithAlertsfor the given adapter — not all adapter paths are guaranteed to call it. - A write carrying a
redirectwith a NaN price will still slip through theisFinite || redirectfilter.
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.
- Resolve conflict in coins/src/utils/servingLayer.ts (keep coerced priceNum; apply same Number.isFinite guard to the Redis read path for parity). - Drop merge-introduced duplicate `priceNum` in addToDBWritesList. - coerceNumericField: split null vs undefined branches so behaviour is explicit (undefined -> NaN, null -> NaN when allowUndefined=false; both -> undefined when allowUndefined=true). Add TS overloads so callers don't need a non-null assertion when allowUndefined=false. - Replace per-process dedup Set with a Map<string, number> that emits on threshold boundaries (1, 10, 100, 1000, 10000) so a chronically broken adapter keeps surfacing instead of going silent for the rest of the pod's lifetime. - Export __resetNumericWarningsForTests for test isolation. - Document NaN-on-decimals/confidence as warn-only-by-design with a TODO pointing at the follow-up filter tightening in batchWriteWithAlerts. - database.test.ts: reset warnings in beforeEach; assert second threshold fires; add webhook-unset path test. - api-spec.test.ts: SPYon guard now uses searchWidth=30d + expects coin to be defined (was silently passing when the token aged out); duplicate- casing test now asserts canonical-key collapse and timestamp uniqueness; /chart happy-path gates decimals/confidence asserts on `in` checks since both are optional in the spec.
g1nt0ki
reviewed
May 7, 2026
| 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}` }); |
Member
There was a problem hiding this comment.
why not map the token on the other chains instead of writing the price everytime?
g1nt0ki
reviewed
May 7, 2026
| if (p.price && p.price !== "0") { | ||
| pipeline.set(`price:${p.canonical_id}`, JSON.stringify({ price: p.price, confidence: p.confidence ? parseFloat(p.confidence) : null, source: p.adapter || null, timestamp: p.latest_ts || null }), "EX", PRICE_TTL); | ||
| } | ||
| if (!p.price || p.price === "0") continue; |
Member
There was a problem hiding this comment.
sometimes we set the price as 0 for blacklisted tokens?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
coins/src/api-spec.test.ts (1)
187-196:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame silent-pass pattern flagged on SPYon survives in three other happy-path tests.
The earlier review fixed
if (!coin) return;→expect(coin).toBeDefined()for the SPYon regression guard, but identical guards still exist in:
- Line 188 —
batchHistoricalhappy path (USDC at1700000000)- Line 255 —
/chart/{coins}happy path- Line 335 —
/prices/first/{coins}happy pathThese are billed as "happy path" assertions, so if the live API ever omits the canonical USDC entry (transient outage, key-casing drift, response-shape change) all the assertions below the early-return are skipped and the test reports green. For the canonical-USDC happy paths, it’s safe to assert presence and let the test fail loudly:
📝 Proposed fixes
@@ batchHistorical happy path (around line 187-188) const coin = r.data.coins[USDC]; - if (!coin) return; + expect(coin).toBeDefined(); expect(typeof coin.symbol).toBe("string");@@ /chart happy path (around line 254-255) const coin = r.data.coins[USDC]; - if (!coin) return; + expect(coin).toBeDefined(); expect(typeof coin.symbol).toBe("string");@@ /prices/first happy path (around line 334-335) const coin = r.data.coins[USDC]; - if (!coin) return; + expect(coin).toBeDefined(); assertFiniteNumber(coin.price, "coin.price");The
if (r.data.coins[USDC])guard at line 271 in thespan=1test has the same shape — happy to leave that one if it’s deliberately tolerant of edge cases, but worth a sanity check.Also applies to: 254-266, 334-339
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coins/src/api-spec.test.ts` around lines 187 - 196, Replace the silent early-return guards that skip assertions when the canonical USDC entry is missing by asserting presence instead: for each test block that does "const coin = r.data.coins[USDC]; if (!coin) return;" (e.g., the batchHistorical happy path, the /chart/{coins} happy path, and the /prices/first/{coins} happy path), change the guard to expect(coin).toBeDefined() and then continue with the existing type/array/assertFiniteNumber checks so failures surface loudly; leave the one at the span=1 test only if its tolerant behavior is intentional after a quick sanity check.
🧹 Nitpick comments (1)
coins/src/adapters/utils/database.test.ts (1)
151-167: 💤 Low valueTest name promises a
console.errorassertion that isn’t made.The test is titled
webhook unset: console.error still fires but no Discord call, but only asserts thatmockSendMessagewas not called — there’s no spy onconsole.error, so the “still fires” half of the contract isn’t actually verified. Either drop the claim from the title or add ajest.spyOn(console, "error")and assert it was invoked.♻️ Proposed assertion
test("webhook unset: console.error still fires but no Discord call", () => { + const errSpy = jest.spyOn(console, "error").mockImplementation(() => {}); 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(); + expect(errSpy).toHaveBeenCalled(); + errSpy.mockRestore(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coins/src/adapters/utils/database.test.ts` around lines 151 - 167, The test "webhook unset: console.error still fires but no Discord call" currently only checks mockSendMessage and doesn't verify console.error; update the test that calls addToDBWritesList by either renaming the test to remove "console.error still fires" or (preferred) add a spy via jest.spyOn(console, "error") before invoking addToDBWritesList and assert that console.error was called (and optionally restore the spy after); keep the existing assertion that mockSendMessage was not called to ensure no Discord call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coins/src/scripts/bootstrapRedis.ts`:
- Around line 89-94: Decide whether price 0 is a valid market price or a
blacklist sentinel; if it is a sentinel, change the guard in the loop that
processes page (the p.price check before Number(p.price) and subsequent
pipeline.set with PRICE_TTL in bootstrapRedis.ts) to also skip when priceNum ===
0, and make the identical change in coins/src/utils/servingLayer.ts (the code
path around the price parsing/assignment currently at lines ~222–225) so both
places consistently drop zero prices; otherwise (if zero is valid) add a brief
inline comment near the p.price check in bootstrapRedis.ts and next to the
servingLayer price handling explaining that price=0 is intentionally accepted.
---
Duplicate comments:
In `@coins/src/api-spec.test.ts`:
- Around line 187-196: Replace the silent early-return guards that skip
assertions when the canonical USDC entry is missing by asserting presence
instead: for each test block that does "const coin = r.data.coins[USDC]; if
(!coin) return;" (e.g., the batchHistorical happy path, the /chart/{coins} happy
path, and the /prices/first/{coins} happy path), change the guard to
expect(coin).toBeDefined() and then continue with the existing
type/array/assertFiniteNumber checks so failures surface loudly; leave the one
at the span=1 test only if its tolerant behavior is intentional after a quick
sanity check.
---
Nitpick comments:
In `@coins/src/adapters/utils/database.test.ts`:
- Around line 151-167: The test "webhook unset: console.error still fires but no
Discord call" currently only checks mockSendMessage and doesn't verify
console.error; update the test that calls addToDBWritesList by either renaming
the test to remove "console.error still fires" or (preferred) add a spy via
jest.spyOn(console, "error") before invoking addToDBWritesList and assert that
console.error was called (and optionally restore the spy after); keep the
existing assertion that mockSendMessage was not called to ensure no Discord
call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a32abe9-e9ce-42ad-bcb8-46b97eb05f10
📒 Files selected for processing (7)
coins/src/adapters/rwa/backed.tscoins/src/adapters/tokenMapping.jsoncoins/src/adapters/utils/database.test.tscoins/src/adapters/utils/database.tscoins/src/api-spec.test.tscoins/src/scripts/bootstrapRedis.tscoins/src/utils/servingLayer.ts
💤 Files with no reviewable changes (1)
- coins/src/adapters/rwa/backed.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- coins/src/adapters/utils/database.ts
- coins/src/utils/servingLayer.ts
👮 Files not reviewed due to content moderation or server errors (1)
- coins/src/adapters/tokenMapping.json
Comment on lines
89
to
94
| for (const p of page) { | ||
| if (p.price && p.price !== "0") { | ||
| pipeline.set(`price:${p.canonical_id}`, JSON.stringify({ price: p.price, confidence: p.confidence ? parseFloat(p.confidence) : null, source: p.adapter || null, timestamp: p.latest_ts || null }), "EX", PRICE_TTL); | ||
| } | ||
| if (!p.price) continue; | ||
| const priceNum = Number(p.price); | ||
| if (!Number.isFinite(priceNum)) continue; | ||
| pipeline.set(`price:${p.canonical_id}`, JSON.stringify({ price: priceNum, confidence: p.confidence ? parseFloat(p.confidence) : null, source: p.adapter || null, timestamp: p.latest_ts || null }), "EX", PRICE_TTL); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether '0' is used as a blacklist sentinel and how the serving layer / consumers treat price=0 vs missing key.
rg -nP -C3 "blacklist" coins/src
rg -nP -C3 'price\s*===?\s*0\b|price\s*===?\s*"0"' coins/src
rg -nP -C3 "price:\$\{" coins/src
# Find the matching write path in servingLayer.ts referenced by the PR summary.
rg -nP -C5 'priceNum|Number\(p\.price\)|Number\(.*price' coins/src/utils/servingLayer.tsRepository: DefiLlama/defillama-server
Length of output: 9187
🏁 Script executed:
# Check if there are any explicit zero-price handling in consumers or downstream code
rg -nP "price.*===?\s*0|price.*===?\s*['\"]0['\"]" coins/src --type ts --type js -A 2 -B 2
# Look for how the price key from Redis is consumed downstream
rg -nP "price:\\\$|price:" coins/src --type ts --type js -C 2 | head -100
# Check if bootstrapRedis.ts actually has a different condition elsewhere (to confirm change)
cat coins/src/scripts/bootstrapRedis.ts | head -100Repository: DefiLlama/defillama-server
Length of output: 12920
Inconsistency: price=0 behavior differs from historical guard but matches existing servingLayer pattern.
The code at lines 89–94 skips empty prices via if (!p.price) but accepts price="0" (string zero is truthy, so !p.price is false). After conversion, Number("0") is finite and gets written to Redis.
This is identical to the pattern already in coins/src/utils/servingLayer.ts:222–225, which also lacks an explicit zero-skip. If this is a change from an older p.price !== "0" guard (per the PR summary), the same change should apply to servingLayer for consistency—or document why the difference exists.
However, consumers in servingLayer don't reject price: 0; they simply skip missing keys. If price zero is intentional (legitimate markets with zero-USD assets exist), the current code is fine and should be documented. If zero remains a sentinel for blacklisted tokens, add || priceNum === 0 to the guard in both locations:
If 0 means "blacklisted", add guard here and in servingLayer.ts
if (!p.price) continue;
const priceNum = Number(p.price);
- if (!Number.isFinite(priceNum)) continue;
+ if (!Number.isFinite(priceNum) || priceNum === 0) continue;
pipeline.set(`price:${p.canonical_id}`, ...Clarify: Is price zero a valid market price or a blacklist sentinel? If sentinel, it should skip here and in servingLayer.ts. Otherwise, document the intent to accept zero-USD prices.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@coins/src/scripts/bootstrapRedis.ts` around lines 89 - 94, Decide whether
price 0 is a valid market price or a blacklist sentinel; if it is a sentinel,
change the guard in the loop that processes page (the p.price check before
Number(p.price) and subsequent pipeline.set with PRICE_TTL in bootstrapRedis.ts)
to also skip when priceNum === 0, and make the identical change in
coins/src/utils/servingLayer.ts (the code path around the price
parsing/assignment currently at lines ~222–225) so both places consistently drop
zero prices; otherwise (if zero is valid) add a brief inline comment near the
p.price check in bootstrapRedis.ts and next to the servingLayer price handling
explaining that price=0 is intentionally accepted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #11797. That PR fixed the string-price bug at the DB-write helper; this PR hardens the surrounding paths and adds test coverage so the bug class cannot silently return.
addToDBWritesListnumeric guardrail (warn-only) — newcoerceNumericFieldhelper runs on every call. Coercesprice/decimals/confidencewithNumber(), then if the result isn't finite (or if the original input was a weird type/string/undefined) fires a deduped Discord warning via the existingSTALE_COINS_ADAPTERS_WEBHOOKand continues the write. Zero runtime blast radius: the write still happens with whateverNumber(...)produces, matching pre-PR behaviour for the existingNumber(decimals)/Number(confidence)coercion. Dedup is per-{adapter, field, reason}tuple per process lifetime — a single misbehaving adapter produces one Discord message, not one per row.servingLayer.ts(Redis rebuild) andbootstrapRedis.tsnowNumber()the price and skip rows where it isn't finite. Before this, legacy string-typed rows in ClickHouse would propagate to clients through the cache even after write-time fixes.servingLayer.tscoerces price on the CH fallback for parity with the Redis path (which already usedparseFloat). If cached price isn't finite, that coin is omitted from the response instead of returning a bad value.rwa/backed.tscleanup — replacedwrites.mapthat mutatedwritesduring its own iteration withwrites.slice()+ nested loop over extra chains. Same behaviour (Array#mapsnapshots length), obviously correct at a glance.coins/src/adapters/utils/database.test.ts— 6 unit tests for the guardrail: valid inputs pass through untouched (no warn), string-numeric prices coerce silently, unparseable/NaN/undefined inputs write + warn once, dedup verified (5 bad calls → 1 Discord message).coins/src/api-spec.test.ts— 33-test live integration suite validating responses on all 7 spec'd coins endpoints against the OpenAPI spec. Configurable viaCOINS_API_BASE(defaulthttps://coins.llama.fi) andSKIP_LIVE=1. Covers happy paths, type assertions, unknown/malformed coins, mixed-case, timestamp boundaries,batchHistoricalduplicate-casing regression, and explicit regression guards for the string-price bug.addToDBWritesListhas ~374 call sites across 162 adapter files, so early versions of this PR thatthrow'd on invalid numerics were a much higher blast-radius change. The committed version is log-only and preserves the exact write behaviour the codebase has today — an adapter that was silently writing NaN before still writes NaN, but now it sends one Discord message so we notice and fix the adapter. No runtime behaviour change for any currently-working call site.Test plan
npx jest src/adapters/utils/database.test.ts→ 6/6 pass (guardrail unit tests)tsc --noEmitclean on all 6 touched files; pre-existing errors elsewhere untouchedorigin/master— not caused by this PRcd coins && npx jest api-spec— 33/33 green once Number Type Coins #11797 merges (2 intentional regression guards for that PR)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Refactor
Chores