Add bounded batch PPS endpoint - #457
Conversation
Add a Redis-backed endpoint for fetching bounded PPS histories across multiple chains. - Validate shared date ranges and chain-qualified vault addresses - Preserve recent-over-historical precedence and predecessor anchors - Bound request and response sizes and document the contract
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| !Number.isSafeInteger(chainId) || | ||
| chainId <= 0 || | ||
| chainId > MAX_CHAIN_ID || | ||
| !ADDRESS_PATTERN.test(address) |
There was a problem hiding this comment.
https://viem.sh/docs/utilities/getAddress can help with this
| } | ||
| } | ||
|
|
||
| export function parsePpsBatchRequest(input: unknown): ParsePpsBatchRequestResult { |
There was a problem hiding this comment.
wdyt of using zod here? will make your life easier!
| const contentLength = Number(request.headers.get('content-length')) | ||
| if (Number.isFinite(contentLength) && contentLength > MAX_REQUEST_BYTES) { | ||
| return errorResponse('Request body is too large', 413) | ||
| } |
There was a problem hiding this comment.
maybe next already handles that for us? I dont remember if does.
matheus1lva
left a comment
matheus1lva
left a comment
There was a problem hiding this comment.
No linked issue; graded against the PR description. All described behaviors (validation, recent-over-historical precedence with predecessor anchor, size bounds, docs) are implemented.
Issues
1. One malformed cached row 500s the whole batch — service.ts:46
toPpsCandidate/toPpsPoint throw on a bad row inside mergeAndSlicePps; route.ts catches and returns 500 for all up-to-50 requested series. v1 tolerates the same data by filtering. If per-series blast radius matters, catch per address in readPpsBatch and return an empty array for the corrupt series; keep precedence and anchor semantics unchanged. Done when a batch with one malformed cached row still returns 200 with the valid series intact — or confirm fail-whole-batch is the intended contract (docs do document 500 on cache read failure).
2. 16KB request cap bypassable without content-length — route.ts:27
Number(request.headers.get('content-length')) is 0 for chunked/absent header, so the guard passes; request.text() then buffers the full body before the byte check. Stream with an incremental cap (abort past MAX_REQUEST_BYTES) or reject requests lacking a valid content-length. Done when an oversized chunked POST gets 413 without full buffering; valid <=16KB bodies still return 200.
3. Second Redis client instead of the shared one — service.ts:12
Module-level createKeyv(process.env.REST_CACHE_REDIS_URL || ...) duplicates cache.ts's getKeyvClient() config; every other REST route reuses the shared client. Reuse it — but v2 relies on { throwOnErrors: true } for its 500 path, which the shared client doesn't set, so export a throwing variant deliberately rather than silently dropping error propagation. Keep the injectable cacheGet seam. Done when no createKeyv call remains under pps/v2 and Redis read failures still 500.
4. No tests despite explicit test seams — service.ts:66
mergeAndSlicePps is exported and readPpsBatch takes injectable cacheGet, yet the PR ships zero specs; the package has vitest and the .spec.ts convention next door (cache.spec.ts). Add service.spec.ts + contract.spec.ts covering anchor at/before start, recent-over-historical tie-break, empty series, address dedup/normalization, range validation, and key ordering via stubbed cacheGet. Done when bun --filter web test exercises all three and passes.
Suggestions (non-blocking)
service.ts:26— cached-row validation is split betweentoPpsCandidateandtoPpsPoint; consolidating it in one place would make the throw paths obvious.- Coverage this review did not reach: the producer-to-consumer cache contract (whether
refresh.ts/refresh-historical.tswriter output actually satisfies the new strict validators — pg numeric strings, day alignment); thecache-control: no-storedivergence from v1's 900s public caching; the 4MB response cap firing only after the full Redis read + stringify; and docs/rest.md accuracy vs implementation (405 behavior, address dedup, exact-start anchor, no-store are undocumented).
How This Was Reviewed
Reviewed with the review-pr-workflow skill —
5 review lenses, each finding independently verified by claude. 1 candidate finding was refuted and dropped.
3 participants
Summary
Add a Redis-backed endpoint for fetching bounded PPS histories across multiple chains. This lets consumers request one shared date range for several vaults without downloading each vault’s complete timeline.
How to review
Review the implementation in this order:
contract.ts— request validation, address normalization, and limitsservice.ts— Redis reads, historical/recent merging, and range selectionroute.ts— HTTP handling, CORS, and payload-size protectiondocs/rest.md— public request and response contractThe existing REST timeseries endpoint, GraphQL API, ingestion flow, and database schema are intentionally unchanged.
For a local smoke test: