fix: harden HTTP/RPC response handling and gas-fee fallback - #218
Merged
Conversation
…ling Three related failure-masking defects: - BaseRpcTransport.parseResponse used a key-presence check, so a successful response carrying 'error': null (sent by some non-strict servers) crashed destructuring null instead of returning the result. - HttpTransport.send never checked the HTTP status and unconditionally parsed JSON, so a 401/429/502 with an HTML or plain-text body surfaced as an opaque SyntaxError with the status — the actual diagnostic — lost. Non-JSON and non-envelope error bodies now throw a TransportRpcError carrying the status; proper JSON-RPC error envelopes still report the server's error. - sendJsonRpcRequest's URL path had the same gap plus a TypeError when the body had neither result nor error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
getFeeData's fallback ladder dropped a successfully fetched maxPriorityFeePerGas into the 1-gwei-floor branch whenever eth_gasPrice was unsupported, pinning both fees below the chain's actual required tip (and contradicting the documented fallback order). The priority fee is now used, with the existing clamp raising maxFeePerGas to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A scalar or null JSON body slipped past the envelope key checks and
threw a raw TypeError ('in' operator on a primitive) instead of a
malformed-response TransportRpcError.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughTransport handling now parses response bodies defensively, accepts ChangesTransport response robustness
Fee fallback calculation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HttpTransport
participant fetch
participant JSON.parse
participant TransportRpcError
HttpTransport->>fetch: Send request
fetch-->>HttpTransport: Return status and text body
HttpTransport->>JSON.parse: Parse text body
JSON.parse-->>HttpTransport: Return payload or parse failure
HttpTransport->>TransportRpcError: Report malformed or HTTP failure
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/transport/JsonRpcNode.ts (1)
299-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the priority-only fallback.
The supplied
test/transport/JsonRpcNode.test.jscoverage exercises both methods being supported and neither being supported, but noteth_gasPricereturning-32601whileeth_maxPriorityFeePerGassucceeds. Add cases where the priority fee is below and above the scaled 1 gwei floor to lock down gas-level scaling and themaxFeePerGas >= priorityFeeclamp.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transport/JsonRpcNode.ts` around lines 299 - 304, The existing JsonRpcNode tests lack coverage for the priority-only fallback when eth_gasPrice returns -32601 and eth_maxPriorityFeePerGas succeeds. Extend test/transport/JsonRpcNode.test.js with cases where the returned priority fee is below and above the gas-level-scaled 1 gwei floor, asserting scaling and that maxFeePerGas is at least priorityFee.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/transport/BaseRpcTransport.ts`:
- Around line 101-103: Validate the JSON-RPC error envelope before destructuring
in BaseRpcTransport.ts lines 101-103: require error to be an object with numeric
code and string message, otherwise return the existing malformed-response error.
Extend the object guard in src/utils.ts lines 572-580 to validate err.code and
err.message, and add transport tests covering string and incomplete error
payloads.
---
Nitpick comments:
In `@src/transport/JsonRpcNode.ts`:
- Around line 299-304: The existing JsonRpcNode tests lack coverage for the
priority-only fallback when eth_gasPrice returns -32601 and
eth_maxPriorityFeePerGas succeeds. Extend test/transport/JsonRpcNode.test.js
with cases where the returned priority fee is below and above the
gas-level-scaled 1 gwei floor, asserting scaling and that maxFeePerGas is at
least priorityFee.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74989980-3c36-43ac-aa2d-73e6a13ef110
📒 Files selected for processing (7)
src/transport/BaseRpcTransport.tssrc/transport/HttpTransport.tssrc/transport/JsonRpcNode.tssrc/utils.tstest/safe/isDeployed.test.jstest/transport/HttpTransport.test.jstest/transport/httpErrorHandling.test.js
Comment on lines
+101
to
+103
| // null check, not key presence — some servers include "error": null | ||
| // on successful responses | ||
| if (response.error != null) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate JSON-RPC error fields at runtime.
Untrusted payloads such as {"error":"bad"} or {"error":{}} can produce a TransportRpcError with undefined code/message rather than the required malformed-response error.
src/transport/BaseRpcTransport.ts#L101-L103: validate thaterroris an object with numericcodeand stringmessagebefore destructuring.src/utils.ts#L572-L580: extend the existing object guard to validateerr.codeanderr.message; add malformed-error-envelope cases to the transport tests.
📍 Affects 2 files
src/transport/BaseRpcTransport.ts#L101-L103(this comment)src/utils.ts#L572-L580
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/transport/BaseRpcTransport.ts` around lines 101 - 103, Validate the
JSON-RPC error envelope before destructuring in BaseRpcTransport.ts lines
101-103: require error to be an object with numeric code and string message,
otherwise return the existing malformed-response error. Extend the object guard
in src/utils.ts lines 572-580 to validate err.code and err.message, and add
transport tests covering string and incomplete error payloads.
Sednaoui
requested changes
Jul 23, 2026
Comment on lines
+101
to
+103
| // null check, not key presence — some servers include "error": null | ||
| // on successful responses | ||
| if (response.error != null) { |
A non-2xx status with a "result" body is contradictory (e.g. a gateway returning 401 with a stub payload) and was resolved as a success by both HttpTransport and sendJsonRpcRequest. On HTTP failure, only a non-null JSON-RPC error envelope now falls through (preserving server errors like 429 rate limits); everything else surfaces the HTTP status. Also keeps the status for 401 + error:null bodies instead of a generic malformed error. Reported by @Sednaoui in #218 review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…port A non-object error field (e.g. a bare string) previously destructured into TransportRpcError(undefined, undefined), losing the payload. It now falls through to the malformed-response error carrying the raw response. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/transport/HttpTransport.test.js`:
- Around line 98-109: Add an array-valued error payload regression test
alongside the existing malformed-response test, then update BaseRpcTransport’s
JSON-RPC error parsing validation to require a non-null, non-array object before
destructuring or creating TransportRpcError; arrays must follow the
malformed-response error path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: accbffee-56d0-4b21-8d78-749f2b91d71d
📒 Files selected for processing (2)
src/transport/BaseRpcTransport.tstest/transport/HttpTransport.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/transport/BaseRpcTransport.ts
Comment on lines
+98
to
+109
| test("treats a non-object error payload as a malformed response", async () => { | ||
| const fetch = makeMockFetch(() => | ||
| jsonResponse({ jsonrpc: "2.0", id: 1, error: "rate limited" }), | ||
| ); | ||
| const t = new ak.HttpTransport("https://example.test/rpc", { fetch }); | ||
|
|
||
| await expect(t.request({ method: "eth_chainId" })).rejects.toMatchObject({ | ||
| name: "TransportRpcError", | ||
| code: -32603, | ||
| data: { error: "rate limited" }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover array-valued error payloads.
typeof [] === "object", so BaseRpcTransport can still destructure an array as an error and produce a TransportRpcError with undefined fields instead of the malformed-response error. Add an array regression case and tighten the parser’s JSON-RPC error-object validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/transport/HttpTransport.test.js` around lines 98 - 109, Add an
array-valued error payload regression test alongside the existing
malformed-response test, then update BaseRpcTransport’s JSON-RPC error parsing
validation to require a non-null, non-array object before destructuring or
creating TransportRpcError; arrays must follow the malformed-response error
path.
Sednaoui
approved these changes
Jul 26, 2026
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
2 participants
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.
Fixes a set of failure-masking defects in the transport layer, found during the audit sweep.
SyntaxErrors:HttpTransport.sendnever checked the HTTP status and unconditionally parsed JSON, so a 401/429/502 with an HTML or plain-text body lost the status — the actual diagnostic. Non-JSON and non-envelope error bodies now throw aTransportRpcErrorcarrying the status; proper JSON-RPC error envelopes still report the server's error.sendJsonRpcRequest's URL path had the same gap.error: nullcrashed parsing:BaseRpcTransport.parseResponseused a key-presence check, so a successful response carryingerror: null(sent by some non-strict servers) crashed destructuring null instead of returning the result.TypeError:sendJsonRpcRequestapplied theinoperator to primitive bodies; they now produce a malformed-responseTransportRpcError.eth_gasPriceis unsupported:getFeeData's fallback ladder discarded a successfully fetchedmaxPriorityFeePerGasand pinned both fees to the 1-gwei floor, below the chain's actual required tip. The fetched priority fee is now kept, with the existing clamp raisingmaxFeePerGasto match.Tests added in
test/transport/httpErrorHandling.test.jsandtest/transport/HttpTransport.test.js; full offline suite passes.Summary by CodeRabbit
Bug Fixes
error: null, preventing incorrect failure detection.result.code/messagefrom valid error envelopes and improved robustness for unexpectederrorshapes.Tests
error: null, malformed payloads, and HTTP/non-envelope failure combinations.