fix(kit): accept JSON-RPC result: null and add context to postRouterRpc errors - #206
Open
nicoeft wants to merge 14 commits into
Open
fix(kit): accept JSON-RPC result: null and add context to postRouterRpc errors#206nicoeft wants to merge 14 commits into
nicoeft wants to merge 14 commits into
Conversation
`Connection.getLatestBlockhashForTransaction` and the router-detection probe both discarded JSON-RPC `error` bodies, so a misconfigured `delegationValidator` surfaced as `Invalid RPC response: undefined` instead of `-32604 "account has been delegated to unknown ER node: <id>"`. Consumers had to patch `node_modules` to see the real cause. Extracts a shared `postRouterRpc<T>` helper plus a typed `RouterRpcError` (carrying `method`, `code`, `data`, `httpStatus`, `cause`) so callers can classify failures without string-matching the message. Parses JSON-RPC error bodies on both 2xx AND non-2xx responses — some providers (notably Helius) return HTTP 404 with a JSON-RPC error body for unsupported methods, and short-circuiting on `!res.ok` would force callers to classify by HTTP status instead of JSON-RPC code. When the error body rode on a non-2xx response, the thrown `RouterRpcError` carries the status in `httpStatus`. The `isRouter` probe classifies an endpoint as non-router when the error looks semantically like "method not found": either the standard code `-32601`, or any code whose message matches `/method not found/i` (covers Helius's non-standard `-32603`). All other failures — transport errors, HTTP failures whose body isn't a parseable JSON-RPC error, unexpected JSON-RPC codes without the method-not-found message — rethrow, preventing a transient failure during construction from permanently mis-classifying a `Connection` as non-router. Also bounds requests with a 10-second `AbortSignal.timeout` so `Connection.create` cannot hang on a router that accepts TCP but never responds. `RouterRpcError` is exported from the public barrel; `postRouterRpc` stays internal. No type signatures on existing APIs change. Adds 19 tests (`router-errors.test.ts`) exercising the fix end-to-end by stubbing `global.fetch` rather than mocking internal helpers; covers the Helius non-2xx + -32603 shape, the httpStatus field, and the message-regex fallback.
…uctor Move the Number.isFinite(code) check into the RouterRpcError constructor so the invariant lives with the type instead of at each call site, and widen the 2xx body.error shape to unknown to match JSON.parse reality. Add regression tests for the 10s AbortSignal timeout, transport errors, the constructor invariant, and lenient-parse branches on both 2xx and non-2xx paths.
…lt handling - Narrow the RouterRpcError constructor to `code: number, message: string` and delete the runtime finite-code throw. Every production call site has already validated these at the boundary (via parseJsonRpcErrorFromObject), so the runtime check duplicated a compile-time guarantee and swallowed signal from upstream construction. - Make the 2xx and non-2xx JSON-RPC error paths symmetric: both now route through parseJsonRpcErrorFromObject, which is strict on `code` and lenient on `message`. A malformed 2xx error (non-finite code) falls through to the generic "returned no result" error rather than surfacing as a typed RouterRpcError with an invalid code. - Accept `result: null` as a valid 2xx success. JSON-RPC spec permits it; the previous `body.result == null` check rejected it. Switched to an explicit own-property check (`hasOwnProperty.call(body, "result")`). - Delete the three RouterRpcError constructor-invariant tests that are now TS-enforced. Replace with a compile-time guarantee plus a runtime test that a malformed 2xx error body falls through to the generic path. - Add a test for `result: null` acceptance and for `.cause` preservation on 2xx non-JSON bodies to lock in the existing cause-forwarding contract.
- Wrap fetch in try/catch and rewrite AbortError/TimeoutError into a method-scoped "Magic Router <method> timed out after 10s" with the original error attached as `.cause`. Other transport errors (e.g. ECONNREFUSED) pass through unchanged. - Attach the body-read failure as `.cause` on the thrown HTTP error when `res.text()` throws, instead of silently dropping it. Mirrors the cause-forwarding pattern already used on the JSON parse path.
- Collapse the RouterRpcError class JSDoc and parseJsonRpcError block to one-liners; the httpStatus field comment already carries the non-obvious invariant, and the lenient-message/strict-code policy fits on one line. - Collapse the postRouterRpc block to one line on the 2xx/non-2xx rationale. The in-body AbortError/TimeoutError comment is now one line. - Collapse isRouter to two one-liners: the latch invariant and the `/method not found/i` rationale for Helius-style `-32603`. - Preserve all "surprising invariant" inline comments per the plan: cause forwarding, prototype restore, and the connection.ts asymmetric null check (untouched).
…omment Rewrite the postRouterRpc JSDoc to lead with the WHY (providers return RPC error bodies on non-2xx) rather than narrating the branch behavior. Remove the AbortError/TimeoutError inline comment — the two-string check is self-evident from the condition.
The `cause` parameter has no caller — both `new RouterRpcError(...)` sites in postRouterRpc construct without it. The Object.setPrototypeOf workaround is also inert at the kit's tsconfig target (es2020 with native class emit), and the package ships compiled JS only, so downstream consumers cannot reach the source at a lower target.
Adds "es2022.error" to the kit's tsconfig lib so the standard
`new Error(message, { cause })` constructor option is typed.
Replaces the three `(err as Error & { cause?: unknown }).cause = ...`
post-assignment workarounds in postRouterRpc with the option form,
and narrows the unsound `(err as Error).message` cast in the
non-JSON catch to `err instanceof Error ? err.message : String(err)`.
Tightens three test assertions: asserts code on the -32601
non-swallow test, asserts httpStatus on the no-message non-2xx
test, and replaces the weak `/non-json/` substring match with a
`/returned non-JSON body/` regex plus a cause-chain assertion.
Runtime support for `Error.cause` is Node 16.9+, satisfied by the
project's vitest 3.x toolchain (which already requires Node 18+).
The web3js sibling has no equivalent code and is intentionally
not touched.
Removes the duplicate `"wraps JSON parse failure with cause on 2xx non-JSON body"` case from the postRouterRpc lenient parsing block. The same wrapper-message + cause-identity contract is asserted by the tightened `"rejects when the 200 body is not JSON"` test in the Magic Router branch describe, which is the more discoverable home for it.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request introduces structured error handling for Magic Router JSON-RPC calls. It adds a new ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ts/kit/src/__test__/router-errors.test.ts`:
- Around line 111-123: Replace the manual await call.catch(...) property checks
with a single Jest rejection matcher: for the test using
conn.getLatestBlockhashForTransaction(txMessage) and RouterRpcError, remove the
.catch block and instead assert await expect(call).rejects.toMatchObject({ code:
-32604, method: "getBlockhashForAccounts", data: { node: "mAGic..." } });; apply
the same change to the other occurrences noted (the tests around lines with
similar patterns) to collapse the two-step rejects + catch pattern into one
robust rejects.toMatchObject assertion.
In `@ts/kit/src/router-rpc.ts`:
- Around line 78-94: Extract the hard-coded 10_000ms timeout into a named
constant (e.g., ROUTER_RPC_TIMEOUT_MS) and use it in both the fetch call's
AbortSignal.timeout(...) and the error message string in the catch branch that
throws `new Error(\`Magic Router ${method} timed out after 10s\`, { cause: err
})`; update the message to derive the seconds from the constant (or include the
ms constant) so the AbortSignal.timeout usage and the `Magic Router ${method}
timed out...` text remain in sync; adjust references around the fetch invocation
and the catch handling where `AbortSignal.timeout(10_000)`, the timeout text,
and the thrown error occur.
In `@ts/kit/src/utils.ts`:
- Around line 33-49: Keep the [[]] probe parameter as-is in isRouter and add an
inline comment explaining this is intentional (transient probe failures must
propagate), reference the postRouterRpc call and RouterRpcError classification
so future maintainers don't change it, and ensure router-errors.test.ts covers
the rethrow behavior for non -32601 errors; do not alter the probe parameter or
error-handling logic unless a future decision explicitly widens classification
(e.g., to include -32602).
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: f4ec7b74-ecca-4677-a827-ea766504b00d
📒 Files selected for processing (6)
ts/kit/src/__test__/router-errors.test.tsts/kit/src/connection.tsts/kit/src/index.tsts/kit/src/router-rpc.tsts/kit/src/utils.tsts/kit/tsconfig.json
Replace `await call.catch(err => expect(...))` with idiomatic `rejects.toMatchObject` / `rejects.toHaveProperty` / `rejects.not.toBeInstanceOf` where the assertions reduce cleanly. Sites with colocated message-regex checks are left as-is, since `toMatchObject` silently skips the non-enumerable `Error.message` and splitting them across multiple `rejects.*` awaits fragments one logical assertion. `toHaveProperty(key, value)` uses `Object.is`, preserving the reference identity that the previous `expect(...).toBe(ref)` checks asserted on `cause`.
Couple `AbortSignal.timeout` and the user-facing "timed out after Ns" message to a single source. The two literals previously had to be kept in sync by hand — bumping the timeout without updating the message would silently produce a lying error string.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
postRouterRpc(the kit's internal Magic Router HTTP helper) has four error-path gaps: valid JSON-RPCnullresults are rejected, timeouts lose method context,res.text()failures are silently dropped, and malformed 2xx error bodies surface through the wrong error class. Net effect: any future binding whose RPC method returnsnullfails silently against the SDK, and debugging the failure paths in production is harder than it needs to be. This PR fixes each gap and tightensRouterRpcError's constructor.Changes
result: nullinpostRouterRpc<T>. Previously rejected asreturned no result: .... JSON-RPC permitsnullas a validresult, so any future binding whose method returns
nullwould have failed silently against the SDK.Magic Router ${method} timed out after 10s, with the originalDOMExceptionattached ascause. Previously surfaced as a bareDOMExceptionwith no method name. Catches bothAbortErrorandTimeoutError.res.text()read failures ascauseon non-2xx responses. Previously the read error was silently dropped.error.code) now fall through toreturned no result: ...instead of throwingRouterRpcError requires a finite number code.... Matches the!res.okpath. Net effect: malformed JSON-RPC error bodies are nevertyped as
RouterRpcErrorregardless of HTTP status.RouterRpcErrorconstructor argcode:unknown → number. The class field was alreadynumber, soerr.code === -32601isunchanged. The runtime finiteness check moved into the parser.
causeconstructor parameter onRouterRpcError. Not yet published —@magicblock-labs/ephemeral-rollups-kit@0.11.2does not export the class.
"es2022.error"tots/kit/tsconfig.jsonlibfornew Error(msg, { cause }). Node 16.9+, well below the existing floor.RouterRpcErrorandisRouter. No semantic change.Tests
npm testints/kit/— 155/155.isRouter's Heliusclassification (HTTP 404 +
-32603+ "Method not found" → non-router). 20/20 ok, p95 ~2.0s, no RPC errors.Scope
ts/web3jsis unchanged — no equivalent code path.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores