Docs/openapi route coverage - #213
JemimahEkong wants to merge 1 commit into
Conversation
…p routes (W085) Every route the app serves must appear in the generated spec. Adds zod schemas and registry entries for GET /tokens, GET /transfers.csv, GET /transfers.parquet, GET /accounts/:address/balance and POST /webhooks/linq, and documents the five /offramp/* routes as internal (deprecated: true with a rationale). The walker-based test in openapiCoverage.test.ts fails on main, guards against vacuous passes, and asserts both committed spec copies match the generator output. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
@JemimahEkong 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.
Strong PR — the route-walker guardrail is exactly the right shape for #186, the descriptions are careful, and I verified the big part is honest work rather than a hand-edit (see below). Two red checks are genuinely yours though, and both are small.
Diff size, for the record: 2191 additions, of which openapi.json is 1600 generated lines. The human diff is 591 added / 9 deleted across four files: src/openapi/build.ts (+203/-9), src/openapi/schemas.ts (+158), src/__tests__/openapiCoverage.test.ts (+162), README.md (+68).
openapi.json is genuinely generated, not hand-edited. I ran npx prisma generate && npm run docs:openapi on your branch and the result is byte-identical to the committed file (5803 lines both sides, no diff once my Windows checkout's CRLF is normalised). Good — that's the thing I most expected to be wrong on a PR this shape, and it isn't.
1. Blocker — src/__tests__/openapiCoverage.test.ts:156 reads a gitignored file
const docsCopy = readFileSync(path.resolve(process.cwd(), "docs", "openapi.json"), "utf8");docs/openapi.json is in .gitignore, with a comment saying so explicitly:
# Generated docs output. docs/ itself is tracked (hand-written guides live
# there); only the generated artefacts inside it are ignored.
docs/openapi.json
So on any clean clone — CI included — that file does not exist and the test throws ENOENT before it can assert anything. Reproduced locally after rm -f docs/openapi.json:
FAIL src/__tests__/openapiCoverage.test.ts
● the committed openapi.json copies match the generator output
ENOENT: ... open '...\docs\openapi.json'
Test Suites: 1 failed, 39 passed, 40 total
Tests: 1 failed, 448 passed, 449 total
That is exactly the CI failure on "Typecheck & build", same counts. Fix: drop the docsCopy read and assert only on the tracked openapi.json. The docs/ copy is a build artefact for the published docs site — it can't drift independently, because the same loop in build.ts writes both from one document, so there's nothing for a second assertion to catch.
2. Blocker — clients/react-query/src/schema.d.ts is stale
This is the other red check ("Generate, typecheck & test"). .github/workflows/react-query-sdk.yml triggers on any change to openapi.json, runs npm run generate in clients/react-query, then git diff --exit-code src/schema.d.ts. Adding ten paths to the spec means that generated client type has to be regenerated and committed in the same PR:
cd clients/react-query && npm install && npm run generate
then commit the resulting src/schema.d.ts. Easy to miss — the coupling isn't mentioned anywhere near src/openapi/, and it might be worth a line in the README section you're already touching.
3. src/__tests__/openapiCoverage.test.ts:148 — deprecated: true is the wrong marker for "internal"
I like that you made internality explicit and testable rather than leaving the /offramp/* routes silently absent. But in OpenAPI deprecated means "still works, will be withdrawn, stop calling it" — generators act on it. The react-query client above will emit @deprecated on five endpoints the wallet depends on today, and anyone reading the published spec will reasonably conclude cash-out is being retired. Please use a vendor extension instead — "x-internal": true alongside the existing rationale in description — and assert on that in the test. Same enforcement, accurate semantics, and it's the conventional hook for filtering routes out of a published spec later.
4. README.md:556 — a tool artifact got committed
<arg_value><b88a6f17>Export the **entire matching transfer set** as a CSV or Apache Parquet download —
Leading <arg_value><b88a6f17> needs deleting. Harmless but it renders.
5. Worth a thought, not a blocker: naming the provider in a published artifact
src/openapi/schemas.ts documents source: z.enum(["linq", "cache"]) and build.ts titles the callback "Linq payout webhook". To be clear, that's not something you introduced — src/api/offramp.ts:268/282 already returns source: "linq" on the wire, so your schema is accurate. But src/api/offramp.ts also carries a deliberate withoutProviderName() helper whose comment says a user-facing error "is the one place it still leaked", and openapi.json is published to the docs site. Documenting these routes moves the provider's name from an internal response field into public reference docs. My call, not yours to fix here — but if you'd rather not make that decision inside this PR, dropping the /offramp/* and /webhooks/linq registrations and leaving the coverage test's allow-list to cover them would be a reasonable scope cut. Happy either way; just flagging it so it's a choice rather than an accident.
Smaller notes
- The branch is cut from
07e6f72;mainis now at 47 jest suites / 562 tests while your branch runs 40 / 449. Nothing in your diff conflicts (GitHub still reportsMERGEABLE), but a rebase will make the CI numbers comparable and is worth doing before the next push. collectRoutesreadsapp._router, which Express 5 removed. You're onexpress@^4.18.3so it's correct today, and your "guards the guard" test is the right defence — just worth a one-line comment pinning the assumption to express 4, next to thepath-to-regexp@0.1.13note you already wrote.README.md's"contractId": "CB64D3G7SM2RTH6ISYIG4P2IYYD6J2OFR6B"in the new/tokensexample is 35 characters, not a valid 56-char contract id — but it's copied from six existing occurrences onmain, so it's pre-existing and not yours to fix. Mentioning it only so you don't propagate it further.
What I ran: npx prisma generate; npx tsc --noEmit and npx tsc --noEmit -p tsconfig.test.json (both clean, exit 0); npm run docs:openapi and a byte-compare against the committed spec (identical); npx jest --runInBand (1 failed / 448 passed / 449 total, 40 suites — the one failure being item 1 above). Integration tests I could not run: the Docker daemon isn't up on this machine.
Fix 1, 2 and 4 and I'll merge this; 3 I'd like too, and 5 is a conversation.
Closes #186
Summary
openapi.jsonlisted only 20 paths while the application serves 30 routes. This PR documents the missing public endpoints, explicitly marks internal routes, and adds a coverage test to prevent the OpenAPI specification from drifting behind the live route table.What Changed
Documented public endpoints
Added Zod schemas and registered the following routes in
src/openapi/build.ts:GET /tokensGET /transfers.csvGET /transfers.parquetGET /accounts/:address/balancePOST /webhooks/linqInternal
/offramp/*routesThe five
/offramp/*routes are explicitly marked withdeprecated: trueand a rationale explaining that they are the wallet's internal cash-out surface in front of a server-side provider API, rather than part of the public data API.Their response schemas remain documented so the specification accurately describes the responses callers receive.
OpenAPI Coverage Guardrail
Added
src/__tests__/openapiCoverage.test.tsto walk the Express route table and verify that every registered route is represented in the generated OpenAPI specification.The test:
:paramsyntax to OpenAPI{param}syntax.openapi.jsoncopies match the generated document.The coverage test fails against
mainwith the 10 currently undocumented routes and passes with this change.Also:
buildOpenApiDocument()for test coverage.require.main === module.Acceptance Criteria
/tokensand the two export routes.src/openapi/build.ts./offramp/*routes explicitly marked internal with a rationale.npm run docs:openapiregenerates deterministically.Verification
tsc --noEmit— cleannpm test— 40 suites / 474 tests passed