fix(trader-stats): aggregate in the database instead of capping at 10k rows (GH#2510) - #2512
Conversation
…k rows (GH#2510)
`GET /api/trader/:wallet/stats` returns fields named `totalTrades`,
`totalVolume`, `totalFees`, `uniqueMarkets`, `firstTradeAt` and `lastTradeAt`.
Both backends fetched at most 10 000 rows and reduced them in JavaScript, so any
wallet past that cap got its PARTIAL history returned as exact totals, with no
flag, cursor or range to say so.
Worth being precise about how wrong it was: both queries order by
`created_at ASC`, so the cap kept the OLDEST 10 000 trades. `lastTradeAt` was
therefore not an approximation of the most recent trade -- it was the timestamp
of the 10 000th oldest one, and it would stop advancing entirely once a wallet
crossed the cap.
Primary path (local indexer) now aggregates in SQL over the full history via
queryTraderStatsAggregate: exact totals, no cap, and one row on the wire instead
of up to 10 000. The arithmetic mirrors the old JS reducer deliberately so the
two cannot drift:
volume += abs(trunc(size)) * round(price * 1e6) / 1e6
fees += round(fee)
`size` is truncated at the decimal point, matching the reducer's
`String(size).split(".")[0]`, and the sums stay in `numeric` so they do not
inherit the float rounding a Number() round-trip would introduce.
Supabase fallback still reads rows, because aggregating there needs a
server-side function -- a schema change, deliberately out of scope here. What it
no longer does is present a partial sum as a total: when the row count reaches
the cap it sets `truncated: true` on the response. The field is optional and
additive, so existing consumers are unaffected; absent means complete.
Tests cover the flag on the fallback (set at the cap, absent below it, absent
when empty) and the mapping of the aggregate row, including the zero-trade case
-- Postgres returns one row of NULLs for an aggregate over an empty set rather
than no row, which is the case most likely to regress into NaN.
One test needed hardening after mutation testing, and says so in a comment: the
"money stays a string" assertion originally used 987654321, which survives a
Number() round-trip intact, so it passed even against an implementation that
parsed the value as a float. It now uses a value above MAX_SAFE_INTEGER, which
fails that mutation.
Mutation-tested, control either side:
control 9 passed
drop the `truncated` flag 1 failed
aggregate returns first for lastTradeAt 1 failed
money mapped via String(Number(...)) 1 failed
control 9 passed
Verified: 303 files, 3056 passed, 17 skipped; tsc --noEmit exit 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughTrader statistics now use PostgreSQL aggregation for indexer data. Supabase fallback queries share a row cap and expose truncation metadata. The trade statistics panel labels capped histories as partial. ChangesTrader statistics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TraderStatsPanel
participant TraderStatsRoute
participant queryTraderStatsAggregate
participant PostgreSQL
TraderStatsPanel->>TraderStatsRoute: request trader statistics
TraderStatsRoute->>queryTraderStatsAggregate: request wallet aggregate
queryTraderStatsAggregate->>PostgreSQL: execute aggregate query
PostgreSQL-->>queryTraderStatsAggregate: return aggregate statistics
queryTraderStatsAggregate-->>TraderStatsRoute: return full-history statistics
TraderStatsRoute-->>TraderStatsPanel: return statistics and optional truncated flag
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
Thanks for picking this up and for the detailed fix. I went through the patch again against the original #2510 report. Moving the primary indexer path to a full database aggregate addresses the main 10k truncation/root-cause cleanly, including the frozen I noticed two details that may be worth checking:
while the SQL currently sums the numeric per-row expressions and casts the final aggregate to Other than those two points, the primary-path change lines up well with the root cause reported in #2510. Thanks for addressing it. |
Both points from @Bayyan16's review on #2512. The first is a real defect in what I pushed, not a nit. 1. VOLUME ARITHMETIC. I claimed the SQL "mirrors the JS reducer exactly". It did not. The reducer accumulates with BigInt division: totalVolume += (absSize * priceE6) / 1_000_000n which TRUNCATES on every trade. The aggregate summed the fractional per-row values and let the final `::bigint` cast round once at the end. Those are different numbers, and the gap is not a rounding error -- it compounds with row count: size=1 price=1.5, x1 reducer 1 sum-then-round 2 size=3 price=0.33 reducer 0 sum-then-round 1 size=1 price=1.9, x10 reducer 10 sum-then-round 19 The reviewer's 1.5 example is exact. Volume is now truncated per row before summing, so the sum is integral and the cast is exact. While fixing it, `round()` became `floor(x + 0.5)` for both price and fee: Postgres `round()` and JS `Math.round` disagree on negative halves (`Math.round(-1.5) === -1`, `round(-1.5) = -2`). The reducer used Math.round, so the SQL should too. 2. UI. #2510's invariant covers the contract AND the display. The API reporting `truncated` is not enough on its own, because the panel is where a reader forms the belief. TradeStatsPanel now renders "Trades (partial)" with "partial history — showing the earliest trades only" instead of "Total Trades" plus a market count when the flag is set. Tests: fractional fixtures pinning per-row parity against the reducer, as the review asked for, including the sub-unit case that truncates to zero and the ten-row case that drifts to 19. One thing worth flagging about those fixtures, since it would otherwise be invisible: they model BOTH implementations in JavaScript, because CI has no Postgres. That documents the contract but does not enforce it -- verified by mutation, reverting the real SQL left all of them green. So the file also asserts the per-row `trunc(` and the `floor(x + 0.5)` forms against the source of `queryTraderStatsAggregate`. That pair binds: the fixtures say why, the source assertions make a revert fail. Mutation-tested, control either side: control 11 passed revert SQL to sum-then-round 1 failed swap floor(x+0.5) back to round() 1 failed control 11 passed Verified: 304 files, 3067 passed, 17 skipped; tsc --noEmit exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both points were right, and the first one was a real defect in what I pushed — thank you for catching it. Fixed in 1. The volume arithmetic. My claim that the SQL "mirrors the JS reducer exactly" was wrong. You identified it precisely. The reducer accumulates with BigInt division — totalVolume += (absSize * priceE6) / 1_000_000n;— which truncates on every trade. My aggregate summed the fractional per-row values and let the final
Your 1.5 example is exact. Volume is now truncated per row before summing, so the sum is integral and the cast is exact. While in there I also changed 2. The UI. You're right that the invariant covers the display, not just the contract.
On the fixtures you asked for — one caveat I want visible rather than buried. I added the fractional fixtures, including the sub-unit case that truncates to zero and the ten-row case that drifts to 19. But they model both implementations in JavaScript, because CI has no Postgres — so they document the contract without enforcing it. I checked that rather than assuming: mutating the real SQL back to sum-then-round left every fixture green. So the file also asserts the per-row
A real DB-level fixture would be better than either, and if the indexer ever gets a test Postgres in CI that is where this belongs. Full suite: 304 files, 3067 passed; |
![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/lib/indexer-db.ts`:
- Around line 519-520: Update the aggregate query against the trades table to
obtain the network via getServerNetwork(), pass it to the aggregate operation,
and add a network = ${network} predicate alongside the existing trader filter.
🪄 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: 6d1ad294-a894-43e4-9d72-e5833a7a3b3a
📒 Files selected for processing (6)
app/__tests__/api/trader-stats.test.tsapp/__tests__/lib/trader-stats-aggregate.test.tsapp/__tests__/lib/trader-stats-sql-parity.test.tsapp/app/api/trader/[wallet]/stats/route.tsapp/components/trade/TradeStatsPanel.tsxapp/lib/indexer-db.ts
| FROM trades | ||
| WHERE trader = ${wallet} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline app/lib/indexer-db.ts --items all
rg -n -C 4 --glob '*.ts' --glob '*.tsx' \
'\bqueryTraderStatsAggregate\s*\(|\bgetServerNetwork\s*\(|INDEXER_DATABASE_URL|\bnetwork\b' app
rg -n -i -C 3 --glob '*.sql' --glob '*.ts' --glob '*.tsx' \
'create table.*trades|alter table.*trades|\bnetwork\b' .Repository: dcccrypto/percolator-launch
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- indexer-db outline ---'
ast-grep outline app/lib/indexer-db.ts --items all | sed -n '1,220p'
printf '%s\n' '--- aggregate and nearby code ---'
rg -n -C 18 'queryTraderStatsAggregate|FROM trades|trader = \$\{wallet\}' app/lib/indexer-db.ts
printf '%s\n' '--- aggregate callers ---'
rg -n -C 12 '\bqueryTraderStatsAggregate\s*\(' app --glob '*.ts' --glob '*.tsx'
printf '%s\n' '--- indexer database configuration references ---'
rg -n -i -C 5 \
'INDEXER_DATABASE_URL|indexer[-_ ]?(database|db)|DATABASE_URL|trades' \
--glob '!**/node_modules/**' \
--glob '!**/.next/**' \
--glob '!**/dist/**' \
--glob '!**/build/**' \
.
printf '%s\n' '--- candidate schema/deployment files ---'
git ls-files | rg -i '(^|/)(schema|migration|migrations|docker-compose|compose|supabase|prisma|drizzle|indexer|database|db)([^/]*)($|/)|\.sql$' | sed -n '1,240p'Repository: dcccrypto/percolator-launch
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- app/lib/indexer-db.ts: imports, config, and aggregate ---'
sed -n '1,180p' app/lib/indexer-db.ts
sed -n '430,560p' app/lib/indexer-db.ts
printf '%s\n' '--- exact aggregate references ---'
rg -n -C 10 \
'queryTraderStatsAggregate|queryTraderStats|trader/\[wallet\]/stats|from ".*indexer-db"' \
app/lib/indexer-db.ts \
app/app/api \
app/__tests__/api/trader-stats.test.ts
printf '%s\n' '--- exact indexer URL references ---'
rg -n -C 6 'INDEXER_DATABASE_URL' \
--glob '!**/node_modules/**' \
--glob '!**/.next/**' \
--glob '!**/dist/**' \
--glob '!**/build/**' \
app .env* Dockerfile* docker-compose*.yml docker-compose*.yaml 2>/dev/null || true
printf '%s\n' '--- tracked SQL and indexer-related files ---'
git ls-files | rg -i '(^|/)([^/]*(schema|migration|migrations|indexer|database|db)[^/]*)$|\.sql$' | sed -n '1,300p'Repository: dcccrypto/percolator-launch
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- indexer-related tracked files ---'
git ls-files | rg -i 'indexer|migration|schema|postgres|database|supabase|docker|compose|railway|render|fly|heroku' | sed -n '1,400p'
printf '%s\n' '--- local indexer network references ---'
rg -n -C 5 \
'network|mainnet|devnet|single[- ]network|multi[- ]network|shared' \
app/lib/indexer-db.ts \
app/app/api/trader/'[wallet]'/stats/route.ts \
app/lib/supabase.ts \
app/lib/config.ts \
README.md PLAYGROUND.md 2>/dev/null || true
printf '%s\n' '--- trader stats fallback ---'
sed -n '114,220p' app/app/api/trader/'[wallet]'/stats/route.ts
printf '%s\n' '--- active server network implementation ---'
rg -n -C 12 \
'export function getServerNetwork|function getServerNetwork|getServerNetwork\s*=' \
app/lib app/app/api --glob '*.ts'Repository: dcccrypto/percolator-launch
Length of output: 48013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Dockerfile.indexer ---'
cat -n Dockerfile.indexer
printf '%s\n' '--- docker-compose indexer/database sections ---'
rg -n -C 12 \
'indexer|postgres|DATABASE|NETWORK|mainnet|devnet|SUPABASE' \
docker-compose.yml
printf '%s\n' '--- mainnet indexer environment example ---'
cat -n docs/env/indexer.env.mainnet.example
printf '%s\n' '--- architecture and deployment network/database references ---'
rg -n -C 8 \
'indexer|database|Postgres|Supabase|network|mainnet|devnet|shared' \
docs/ARCHITECTURE.md \
docs/BACKEND-ARCHITECTURE.md \
docs/MAINNET-READINESS.md \
docs/MAINNET-ROADMAP.md \
packages/indexer/README.md \
scripts/deploy-mainnet-railway.sh 2>/dev/null || true
printf '%s\n' '--- network-column migrations ---'
sed -n '1,220p' supabase/migrations/20260329170000_add_network_column_PERC8192.sql
sed -n '1,220p' supabase/migrations/20260329180000_add_network_column.sqlRepository: dcccrypto/percolator-launch
Length of output: 50385
Scope the local aggregate by network.
The indexer trades table includes network, and INDEXER_DATABASE_URL does not guarantee a single-network database. Pass getServerNetwork() to the aggregate and add AND network = ${network}.
🤖 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/lib/indexer-db.ts` around lines 519 - 520, Update the aggregate query
against the trades table to obtain the network via getServerNetwork(), pass it
to the aggregate operation, and add a network = ${network} predicate alongside
the existing trader filter.
|
On CodeRabbit's finding (missing What I checked:
So fixing it only in the aggregate would make trader stats network-isolated while candles, funding and trade history on the same database stay cross-network. That is a worse state than either consistent option, and it hides the real problem. One thing that makes a blind fix riskier than it looks, and which I checked rather than assumed. The two sides derive the value from different environment variables:
The vocabularies match, which is the good news. But they are different variables, and both default to I have raised it as its own issue covering all of Not making the change here. Happy to be overruled if you would rather take the narrow version now. |
|
Agreed. From my side, I wouldn’t treat the network predicate as a blocker for #2512. Since the behavior is pre-existing across I’m fine treating the network-scoping item from #2510 as a follow-up rather than expanding this truncation fix. Thanks for checking the broader path before applying the narrow change. |
Review: looks good, mergeThe bug is worse than "stats are approximate", and the PR gets that right. The defectThe row cap orders by What I checked hardest: the SQL/JS arithmetic parityThis is where a "just move it into SQL" change goes wrong silently, and the PR documents that it did go wrong on the first attempt. The per-row I mutation-tested the parity guard rather than trusting it:
Both are exactly the failure modes the comment warns about, and both fail the guard. Scoping is honestThe indexer path aggregates over full history. The Supabase fallback can't without a server-side function, so it stays capped — but now sets Verified
One note, not blockingTwo assertions in |
2 participants
Closes #2510.
The bug
GET /api/trader/:wallet/statsreturns fields namedtotalTrades,totalVolume,totalFees,uniqueMarkets,firstTradeAt,lastTradeAt. Both backends fetched at most 10 000 rows and reduced them in JavaScript, so any wallet past that cap had its partial history returned as exact totals — no flag, no cursor, no range.One detail worth adding to the report: both queries order by
created_at ASC, so the cap keeps the oldest 10 000 trades.lastTradeAtwas therefore not an approximation of the most recent trade — it was the timestamp of the 10 000th oldest one, and it stops advancing entirely once a wallet crosses the cap. A trader's "last trade" would freeze at a date in their past and never move again.Verified against
playground@41bb1304:indexer-db.ts:466(LIMIT 10000,ORDER BY created_at ASC) and the two.limit(10_000)calls atroute.ts:140/148.The fix
Primary path (local indexer) — exact. Aggregates in SQL over the full history via a new
queryTraderStatsAggregate: no cap, and one row on the wire instead of up to 10 000.The arithmetic mirrors the old JS reducer deliberately, so the two paths cannot drift:
sizeis truncated at the decimal point to match the reducer'sString(size).split(".")[0], and the sums stay innumeric, so they don't inherit the float rounding aNumber()round-trip would introduce.Supabase fallback — honest. Aggregating there needs a server-side function, i.e. a schema change. I've deliberately kept that out of scope, since a schema-deployment dependency is exactly what made #2504 unschedulable. What this path no longer does is present a partial sum as a total: when the row count reaches the cap it sets
truncated: true.The field is optional and additive (
truncated?: boolean), so existing consumers are unaffected — absent means complete.On scope
I could have made the fallback exact with a Postgres aggregate function in
supabase/schema.sql. I didn't, because it would tie this to a deployment step and the primary path is the one actually serving these stats (hasIndexerDb()is preferred). Happy to follow up with the RPC if you'd rather have both paths exact — it's a small function, it just needs somewhere to land.Tests
NULLs for an aggregate over an empty set rather than no row, which is the case most likely to regress intoNaN.One test needed hardening, found by mutation rather than by reading. The "money stays a string" assertion originally used
987654321, which survives aNumber()round-trip intact — so it passed even against an implementation that parsed the value as a float. It now uses a value aboveMAX_SAFE_INTEGER, and there's a comment recording why, so nobody "simplifies" it back.Mutation-tested
truncatedflagfirstforlastTradeAtString(Number(...))Verification
The 22 pre-existing trader-stats tests (route, hook, rate-limit) all still pass unchanged.
Summary by CodeRabbit
New Features
Bug Fixes
Tests