Conversation
|
@Wang1rrr Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
A lot here is genuinely careful and I want to credit it before the objections. The overlap guard in startOhlcRefreshWorker (the running flag) and the switch to Promise.allSettled so a failed bucket can't leave queries pending into the next tick are both the right instincts. The interval validation is unusually thorough — rejecting non-integers, zero and blank, and explicitly rejecting > 2^31-1 because Node clamps an oversized delay to 1 ms is the kind of thing most people find out in production. Disabled by default, wired into the shutdown path, and SKIP_INDEXER respected. And the $queryRawUnsafe parameter fix in queryCandlesFromAggregate is a real bug caught: main passed [contractId, limit, offset] as a single positional argument, so $1/$2/$3 never bound. The ${table} interpolation is safe because bucket comes out of a z.enum before it's used — I checked.
Verified locally on the merge with main: tsc --noEmit and tsc -p tsconfig.test.json both clean, and all three new suites pass — 41 tests across ohlcRefreshWorker, ohlcStartup and candles-route.
Three things I can't merge past.
1. POST /candles/refresh is mounted with no authentication (src/api/candles.ts:98, mounted at src/api.ts:199)
This is a new, anonymous, mutating endpoint that runs ohlc.refresh_candles_1m/1h/1d() — three full aggregate rebuilds over the transfers table — against the production database. The only thing in front of it is the global rate limiter at src/api.ts:65, which is 60 requests/minute per IP. Sixty full aggregate rebuilds a minute per IP, from anyone, on a Postgres instance this deployment shares. openapi.json now advertises it too.
The irony is that this PR makes the endpoint unnecessary: startOhlcRefreshWorker already does exactly this on a timer. Either drop the public route entirely and let the worker own refreshes, or put it behind the same bearer-token check #216 just added to /offramp. Dropping it is cleaner.
2. The read route answers for a network it then ignores
networkMiddleware is mounted app-wide at src/api.ts:155, so GET /candles/1h/C…?network=mainnet validates and resolves the network — and then queryCandlesFromAggregate never uses it. Your own README text says why: "These legacy aggregates do not separate networks, so they must not be used with a database containing transfers from multiple networks." Thank you for writing that down honestly, but a README note doesn't stop the route from returning 200 with mainnet and testnet candles blended together while the caller believes they asked for one. That's the failure mode src/middleware/network.ts is written against in so many words — "silently conflating them is how a dashboard ends up confidently showing zero."
Since the tables also aren't installed by any current startup path (again, your README), the route 500s on every request on every deployment today. Gate the mount behind the same opt-in that gates the worker: if OHLC_REFRESH_INTERVAL_MS is unset, don't mount /candles and don't register it in openapi.json. Then an operator who installs the aggregates and accepts the single-network constraint opts into both together, and nobody else gets a documented endpoint that only ever 500s.
3. src/db.ts:39 — log: [] silences Prisma for the whole application
Going from [query, warn, error] in development and [warn, error] in production to [] everywhere is a global observability change that has nothing to do with mounting candles routes. The motivation is right — this repo's rule is that nothing logs a raw database error or connection string — but it's worth its own commit and its own reasoning, because as written it also drops Prisma's connection-pool and deprecation warnings, which don't carry query text. log: ["warn"] would satisfy the rule and keep those. Either way, please split it out.
One smaller note, no action required: catch { console.error("[candles] Query failed"); } now discards the error object completely. Not leaking the message is right; losing the error class means a missing relation and a connection timeout produce byte-identical logs. Something like err instanceof Error ? err.name : "unknown" keeps it diagnosable without leaking anything.
Solid work — the worker is in good shape. It's the public surface that needs tightening.
Description
The candles router and refresh worker existed but were never connected to the application. Mount the router at
/candles, document its GET and refresh endpoints in the generated OpenAPI spec, and start the worker fromsrc/index.tswhenOHLC_REFRESH_INTERVAL_MSis a valid positive integer. An unset value or0leaves it disabled;SKIP_INDEXER=truealways skips it.The startup hook retains the worker's stop callback for shutdown. A running refresh skips later timer ticks, including when one bucket fails while others are still pending.
The newly reachable route also needed three small corrections: pass Prisma's SQL placeholders as separate arguments, use the existing Zod pagination/parameter schemas, and keep raw database errors out of HTTP responses and logs. Prisma's automatic query/error stdout logging is disabled because it otherwise prints those errors before the application can sanitize them. The OHLC handlers and worker retain fixed diagnostic messages.
Closes #190
Validation
main(07e6f723):npx tsc --noEmitand all 39 Jest suites / 443 tests pass.createApp()and failed with 404. They pass after mounting the router. The tests include realPrisma.Decimalserialization, separate SQL values, invalid input, and sanitized GET/POST failures.npm run typecheck,npx tsc --noEmit, andnpm run buildpass.npm test -- --silent: 42 suites / 484 tests pass, including the configured coverage gate.npm run docs:openapisucceeds. Both generated copies match and contain/candles/{bucket}/{contractId}plus/candles/refresh; the tracked rootopenapi.jsonis included.git diff --checkpasses.Existing database prerequisite
Scheduled refresh is opt-in because the OHLC tables/functions are not installed by the current Prisma startup path. The existing
sql/001_ohlc_aggregates.sqlalso needs separate repair: an isolated PostgreSQL/PGlite execution rejectsSTRINGas a type, and an in-memory diagnostic replacement withTEXTthen exposes invalidARRAY_AGG ... ORDERsyntax. The legacy aggregates also lack a network dimension. The README and env example document the prerequisite; this PR does not execute or deploy that script.Docker integration tests were not run locally because no Docker daemon is running. The base commit's actual GitHub CI run (36567791274) has passing build and integration jobs; the issue's earlier statement that integration is red is outdated. The separate existing load-test failure stops before k6 because
STAGING_BASE_URLis absent.