Apply phantom-OI suppression to /api/stats protocol open interest - #2483
Apply phantom-OI suppression to /api/stats protocol open interest#24830x-SquidSol wants to merge 1 commit into
Conversation
/api/stats' shared-loader path (computeStatsFromMarketsApi) summed total_open_interest_usd directly, without the phantom-OI suppression /api/markets applies (isPhantomOpenInterest + computeDisplayOiUsd). A market whose OI is suppressed to $0 in the market list therefore still inflated the protocol-wide totalOpenInterest stat — the exact drift the shared predicate exists to prevent (markets/route.ts documents it as the single source of truth for both routes). Compute per-market display OI with the shared helpers before summing, mirroring /api/markets exactly. - stats/route: isPhantomOpenInterest + computeDisplayOiUsd per row in the merged-rows OI sum - add a regression test (raw sum over-counts a phantom market; the helper-based sum excludes it) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@0x-SquidSol is attempting to deploy a commit to the Khubair Nasir's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughChangesThe Stats aggregation and regression coverage
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/__tests__/api/stats-phantom-oi.test.ts`:
- Around line 27-45: Replace the local helper-only assertions in the “stats
phantom-OI suppression” tests with an integration-style test of the `/api/stats`
route: mock `loadMergedMarketRows` with the phantom and real markets, invoke the
exported `GET`, and assert the response’s `totalOpenInterest` is 10,000. Keep
coverage for phantom suppression through the route so regressions in
`computeStatsFromMarketsApi` are detected.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e111a4ff-0393-425d-a01c-a62e5e3b21e1
📒 Files selected for processing (2)
app/__tests__/api/stats-phantom-oi.test.tsapp/app/api/stats/route.ts
| describe("stats phantom-OI suppression", () => { | ||
| it("raw summation over-counts a phantom market (the bug)", () => { | ||
| const rawSum = [phantom, real].reduce((s, m) => s + (m.total_open_interest_usd ?? 0), 0); | ||
| expect(rawSum).toBe(15_000); // phantom's stale $5k is wrongly included | ||
| }); | ||
|
|
||
| it("applying isPhantomOpenInterest + computeDisplayOiUsd excludes it (the fix)", () => { | ||
| const fixedSum = [phantom, real].reduce((s, m) => { | ||
| const isPhantom = isPhantomOpenInterest(m.total_accounts, m.vault_balance); | ||
| const oi = computeDisplayOiUsd(m.total_open_interest_usd, isPhantom, m.total_open_interest); | ||
| return s + (oi ?? 0); | ||
| }, 0); | ||
| expect(fixedSum).toBe(10_000); // phantom suppressed to 0, only the real market counts | ||
| }); | ||
|
|
||
| it("the phantom market is individually suppressed to 0, the real one is unchanged", () => { | ||
| expect(computeDisplayOiUsd(phantom.total_open_interest_usd, isPhantomOpenInterest(phantom.total_accounts, phantom.vault_balance), phantom.total_open_interest)).toBe(0); | ||
| expect(computeDisplayOiUsd(real.total_open_interest_usd, isPhantomOpenInterest(real.total_accounts, real.vault_balance), real.total_open_interest)).toBe(10_000); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test the /api/stats route instead of a local copy of its logic.
These tests do not import app/app/api/stats/route.ts or invoke GET. They would pass before this PR because the local reduce already uses the shared helpers.
Mock loadMergedMarketRows, call GET, and assert that totalOpenInterest is 10_000 for these two markets. This makes the test fail if computeStatsFromMarketsApi again sums total_open_interest_usd directly.
🤖 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 `@app/__tests__/api/stats-phantom-oi.test.ts` around lines 27 - 45, Replace the
local helper-only assertions in the “stats phantom-OI suppression” tests with an
integration-style test of the `/api/stats` route: mock `loadMergedMarketRows`
with the phantom and real markets, invoke the exported `GET`, and assert the
response’s `totalOpenInterest` is 10,000. Keep coverage for phantom suppression
through the route so regressions in `computeStatsFromMarketsApi` are detected.
dcccrypto
left a comment
dcccrypto
left a comment
There was a problem hiding this comment.
Reviewed on head e7d0eaff. Fix is correct. Suite clean: 2922 passed / 0 failed (282 files).
I chased one specific hazard and it doesn't apply — worth recording so nobody re-chases it
The new line coerces an unsupplied column to zero:
const isPhantomOI = isPhantomOpenInterest(numericOrNull(row.total_accounts), numericOrNull(row.vault_balance) ?? 0);Two things made that look dangerous:
computeStatsFromMarketsApidefinesasSupplied()eight lines above precisely
to distinguish "column not supplied" (undefined) from a value, and the zombie
filter directly above usesasSupplied(row, "vault_balance")— so the new code
bypasses the file's own discipline for the same field.isPhantomOpenInterest's doc comment warns about exactly this on the other
parameter: "Unknown is not zero … an unknown account count simply abstains
instead of voting phantom." Coercing an unknown vault to0makes
0 < MIN_VAULT_FOR_OItrue, i.e. votes phantom, andmarket-registry.ts:27-28
records thatvault_balancewas dropped from the reduced schema — so a
registry-only row genuinely hasundefinedthere.
That would zero protocol OI whenever the live RPC read degrades — which
loadMergedMarketRows explicitly supports (if (!live) return m;, documented as
"partial RPC results degrade to the pre-merge behaviour rather than zeroing a
market out").
It's a non-issue, for two independent reasons, and I checked both before saying
anything:
/api/marketsreads from the sameloadMergedMarketRows()(:726) and applies
the same coercion (numericOrNull(m.vault_balance)at :784 →?? 0at :826).
So the PR mirrors it faithfully. Any change here would have made the two
surfaces disagree again — the opposite of the goal.- On a registry-only row the outcome is unchanged anyway.
total_open_interest_usd
was dropped in the same schema reduction, so the old code's
if (oiUsd != null …)skipped the row (contributing 0) and the new code returns
0via the phantom branch. Identical.
So the behaviour change is confined to rows that do have live data, which is
exactly the intended fix. isSaneMarketValue moving from the raw to the displayed
value is also fine — the only new value it sees is 0.
Test binding
stats-phantom-oi.test.ts imports the two helpers and re-applies them to local
fixtures; it never imports app/api/stats/route. Deleting the new block from the
route leaves it green, so it documents the intended arithmetic rather than pinning
the route to it.
Your own issue text makes this point better than I can — it's why the bug survived
in the first place:
The existing
stats-phantom-oi-guardtests pass because they exercise a
mirror of the intended logic, not the route's actual summation.
That is precisely what the new test also does, so the next drift in this path will
be just as invisible. This is the sixth PR in the run with that shape (#2467,
#2470, #2472, #2475, #2480, and now this one) — and having diagnosed it explicitly
here, this seems like the right one to break the pattern on: assert the route's
summation, not a re-derivation of it.
Minor: the issue title got mangled
#2482's title reads C:/Program Files/Git/api/stats over-counts … — Git Bash
MSYS path conversion rewrote the leading /api/stats. Prefixing the command with
MSYS_NO_PATHCONV=1 (or writing //api/stats) avoids it. Cosmetic, but it makes
the issue hard to find by name.
2 participants
What
Apply the phantom-OI suppression to
/api/stats' protocol-wide open-interest sum soit matches
/api/markets.Closes #2482.
Why
computeStatsFromMarketsApisummedtotal_open_interest_usddirectly, skipping theisPhantomOpenInterest+computeDisplayOiUsdsuppression/api/marketsapplies(markets/route.ts:827-833). A market suppressed to $0 OI in the list (no accounts /
dust vault) therefore still inflated the protocol-wide
totalOpenInterest. The/api/marketscomment names these helpers the single source of truth for bothroutes; the stats shared-loader path just wasn't using them. (The existing
stats-phantom-oi-guardtests kept passing because they test a mirror of theintended logic, not the route's actual summation.)
Changes
isPhantomOpenInterest+computeDisplayOiUsd(fromtotal_accounts,vault_balance,total_open_interest,total_open_interest_usd) before summing, mirroring/api/marketsexactly.sum excludes it (and the phantom market is individually suppressed to 0).
Testing
npx tsc --noEmit— clean.stats-phantom-oi-guard,stats-active-total-consistency,ProtocolStatsBar).Notes
definitionally consistent.
Summary by CodeRabbit
Bug Fixes
Tests