Repository navigation
fix: report provider dependency health in /status - #253
Miracle656 merged 13 commits into
Conversation
|
@Nwapu-TrustJah 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! 🚀 |
|
Rebase needed — #261 landed, consolidating the two Linq HTTP clients into a single What changed, so the rebase is predictable:
Also worth knowing: Push and I will pick it up. |
|
Rebase needed — #252 just landed and also rewrites the What #252 put there, so your rebase lands on top rather than fighting it:
Your PR reports provider dependency health — Linq — which is the piece that is still missing, so the two compose rather than overlap: #252 answers "is our own indexer alive", yours answers "is the thing we depend on alive". Worth making sure the two end up as one coherent status shape rather than two bolted-on sections. Also: if you add fields to the status schema, run
|
|
Reconciled |
Miracle656
left a comment
There was a problem hiding this comment.
Needs a rebase, and the reason is worth spelling out because it is not a textual conflict — it merges "cleanly" and produces something broken.
Two LinqError classes
#261 landed after you branched and consolidated the two Linq HTTP clients into a single src/linq/http.ts. Your changes re-populate src/linq/client.ts (+190 lines), so after a merge the repo has:
src/linq/client.ts:22:export class LinqError extends Error {
src/linq/http.ts:59:export class LinqError extends Error {
Two classes, same name, different identity. Every err instanceof LinqError then depends on which module the thrower imported:
● submitCustomerKyc — the NIN never escapes the request › does not leak the NIN through a thrown error
Expected constructor: LinqError
Received constructor: LinqError
That error message is the shape of the whole problem — it looks like a tautology and is not. 31 tests fail, and the modules are already split down the middle: onramp.ts and bills.ts import from ./http, while api/ngn.ts, api/offramp.ts and coinPrice.ts still import from ./client.
This is not cosmetic. An instanceof check that silently stops matching takes the error code with it, and the code is what the retry logic, the rate-limit handling and yesterday's existingCustomerRef adoption all branch on. A failure would stop being classified and start being generic — which is exactly the state those changes were made to get out of.
#261 added guard tests for precisely this (only http.ts names the LINQ_BASE_URL variable, has no other call() or fetch() in the Linq modules), so a rebase that puts your work on top of http.ts will tell you immediately if anything is still split.
The feature itself is one I want
/status reporting provider dependency health is the missing half of what #252 added. #252 answers "is our own indexer alive"; yours answers "is the thing we depend on alive". A service that reports itself healthy while Linq is unreachable is reporting the wrong thing — every naira operation is going to fail and /status is green.
Worth coordinating the shape: #252 put per-network indexer state and a reason on /status. Two bolted-on sections would be worse than one coherent object, so have a look at what is there before you rebase.
One more thing: if you add fields to the status schema, run npm run docs:openapi before pushing. The committed openapi.json is checked against the generator and that was the only red on #252.
Integration tests is red on main itself (Postgres read path, since at least 10-16) and is not yours.
40b1cf5 to
12e1017
Compare
|
Hi @Miracle656, I've resolved the merge conflicts on |
Miracle656
left a comment
There was a problem hiding this comment.
You rebased and the LinqError duplication is genuinely fixed — src/linq/http.ts:59 is now the only declaration, which was the thing I asked for. But the rebase went one step too far and the branch no longer builds.
src/linq/client.ts is deleted, and it was not just a shim
My previous review said to rebase on top of http.ts. I think that read as "remove client.ts", and it isn't — on main that file is the offramp provider client: getRate, verifyBank, checkStellarTrustline, createOfframpOrder, getOfframpStatus. What #261 moved into http.ts was only the shared transport and the error class; client.ts kept the offramp calls and re-exports LinqError from ./http so existing importers keep working.
Deleting it takes the offramp's provider calls with it. createOfframpOrder now exists nowhere under src/linq/.
Verified on your branch on its own, not on a merge:
$ npx tsc --noEmit # 25 errors
src/api/ngn.ts(45,27): error TS2307: Cannot find module '../linq/client'
src/__tests__/linq/bills.test.ts(10,27): error TS2307: Cannot find module '../../linq/client'
src/__tests__/linq/client.test.ts(12,8): error TS2307: Cannot find module '../../linq/client'
src/__tests__/linq/httpClient.test.ts(40,25): ...
src/__tests__/linq/onramp.test.ts(19,27): ...
src/__tests__/linq/rateLimit.test.ts(14,27): ...
src/__tests__/routes/ngn.test.ts(47,27): ...
src/api/offramp.ts imports from it too. So this is not a test-only break — the cash-out route loses the functions that actually create the order.
The other 18 errors are a knock-on: with the module unresolved, LinqError is any, so every err instanceof LinqError branch in src/api/ngn.ts degrades to TS18046: 'err' is of type 'unknown'. Which is, in a roundabout way, the same failure my last review was about — an instanceof that stops classifying errors, so a provider failure stops carrying its code.
What to do
Restore src/linq/client.ts from main as-is and put your dependency-health work on top of it. You do not need to touch it at all for this feature. If you'd rather finish #261's consolidation and move the offramp calls into their own module, that's a reasonable change — but it's a separate PR, it has to update every importer including the six test files, and it shouldn't ride along with a /status change.
npm run typecheck catches the whole thing in one command.
The feature is still the one I want
src/dependencyHealth.ts plus src/linq/dependencyHealth.ts is a sensible split — a generic notion of dependency health, and the Linq-specific probe that feeds it — and reporting it on /status is the missing half of #252. A service that says it is healthy while the provider behind every naira operation is unreachable is answering the wrong question.
I can see openapi.json in the diff, so it looks like you ran npm run docs:openapi as asked. Good.
One thing to check when you re-push: whether your /status shape sits inside #252's object or beside it. Two parallel health sections would be worse than one, and #252 is on main now so you can see exactly what it emits.
Integration tests is red on main itself (Postgres read path, since at least 10-16) and is still not yours.
Re-request review when it typechecks and I'll come straight back to it — the hard part, the rebase onto http.ts, you've already done.
1f5908c to
51a146e
Compare
|
Merged to
The dependency-health half was unwired. I wired it rather than send it back, since there was one obvious place: Two fixes while wiring it:
I also deleted Worth saying explicitly, because this repo has had the outage: none of this gates startup. One provider being down is not a reason to refuse to serve. wraith: 79 suites, 1,214 tests passing, typecheck clean. |
Every fee GET /fees quotes is recorded with the policy that produced it, so a fee can be explained after the fact rather than merely logged, and a quote outside the configured range raises an alarm. GET /fees/report reads it back by day and asset. The migration I asked for last time is here, and it is additive — two new tables, so startup's `db push` creates them without a manual step. What makes this design good, and why I checked rather than assumed: - FeePolicy is keyed by a hash of the canonical policy JSON, so one row per distinct configuration however many quotes it priced, and any change to percent, floor, cap, flat or destination becomes a new row with its own firstSeenAt. A historical fee stays explainable with no versioning scheme to maintain. - FeeQuote.policyId points at it without a foreign key, so a quote is never refused because its policy row lost a race. - The write is fire-and-forget and recordFeeQuotes cannot throw, so a ledger outage degrades to "no fee recorded" rather than a failed payment. - Nothing recorded identifies a person: no IP, no user agent, no sender address. The only address is the fee destination, which /fees already publishes to anyone who asks. On a rail under the NDPA that distinction is worth having written down. - `basis` (percent / floor / cap / flat / no_amount) is stored alongside the number, so the fee can be explained without re-deriving it. Two fixes on merge. - The openapi schemas conflicted with #253, which landed first and added ngnCustomerStatusQuerySchema and ngnCustomerStatusResponseSchema. A plain union left each declared twice, where the last declaration silently wins. Dropped this branch's copies and kept main's; feeReportQuerySchema and the fee report schemas are genuinely new and stay. - docs/openapi.json was stale: the branch updated the root openapi.json but not the generated copy. Regenerated — the only new path is /fees/report. wraith: 80 suites, 1,267 tests passing. Typecheck clean, openapi coverage guard green. Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Overview
/statusreported"ok": true, "status": "healthy"throughout an outage where no user could complete identity verification, because the provider's KYC endpoint was returning 5xx and nothing watched it. This PR adds adependenciessection to/statusthat reports the last observed outcome of the provider endpoints the product actually needs, distinguishes "this service is up" from "the rails are usable", and surfaces a failing dependency asdegradedrather thanhealthy.Related Issue
Changes
🔌 Provider Dependency Reporting
[MODIFY]
src/linq/statusForClients.tsGET /b2b/rate) is unauthenticated and side-effect free, so it is probed directly.degraded(notunhealthy): the process is up, but the rails are not usable. The response states which.[MODIFY]
src/linq/client.ts/statusreflects observed reality without adding any new outbound call.[MODIFY]
src/api.ts/statusresponse.[MODIFY]
src/openapi/schemas.ts,openapi.jsondependenciesfield and thedegradedstatus value.[ADD]
src/__tests__/linq/statusForClients.test.tsglobal.fetchmock pattern. No test reaches the live provider.Verification Results
/statusdistinguishes "this service is up" from "the rails are usable"status: "degraded"with adependenciessection separate from service healthGET /b2b/rateis probed; KYC outcome is reported from the last real call/statuswithin a bounded time, and the bound is statedobservedAtper dependency; rate probe on each/statuscall)src/__tests__/linq/statusForClients.test.tsCloses #241