refactor(indexer): replace axios with native fetch API - #3553
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe indexer replaces Axios and ChangesIndexer HTTP migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
62d011d to
1823182
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3553 +/- ##
==========================================
- Coverage 75.08% 75.05% -0.03%
==========================================
Files 1162 1162
Lines 30165 30165
Branches 7503 7509 +6
==========================================
- Hits 22649 22640 -9
- Misses 6638 6651 +13
+ Partials 878 874 -4
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@apps/indexer/src/chain/nodeInfo.ts`:
- Around line 179-181: Update the non-OK response handling around
HttpResponseError to read the body once with response.text(), parse it only when
valid JSON, and preserve the original text otherwise. Do not catch body-read
failures so aborts and other read errors propagate; add a regression test
covering a non-JSON HTTP error response.
- Around line 110-112: Update the non-OK response branch in updateStatus to
cancel or fully consume response.body before throwing HttpResponseError, while
preserving response.status for the exception. Keep the existing
successful-response handling and retry behavior unchanged.
In `@apps/indexer/src/db/priceHistoryProvider.ts`:
- Line 22: Update the response handling around the PriceHistoryProvider request
to check response.ok before invoking response.json(). When the response is
non-2xx, throw an error that includes the HTTP status, and preserve the existing
JSON parsing flow for successful responses.
In `@apps/indexer/src/providers/ipLocationProvider.ts`:
- Around line 19-20: Update the fetch flow in the IP location provider to check
response.ok immediately after fetch and throw for non-2xx responses before
invoking response.json(). Preserve JSON parsing and provider updates only for
successful responses.
🪄 Autofix
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: CHILL
Plan: Pro
Run ID: 5e96dd37-e870-40f0-a18b-d13c3c186907
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
apps/indexer/package.jsonapps/indexer/src/chain/nodeInfo.tsapps/indexer/src/db/keybaseProvider.tsapps/indexer/src/db/priceHistoryProvider.tsapps/indexer/src/providers/ipLocationProvider.tsapps/indexer/src/providers/providerStatusProvider.tsapps/indexer/src/providers/statusEndpointHandlers/rest.tsapps/indexer/src/scheduler.tsapps/indexer/src/shared/utils/fetch.ts
💤 Files with no reviewable changes (1)
- apps/indexer/src/scheduler.ts
There was a problem hiding this comment.
Beyond the two previously flagged findings on this PR, this run also examined whether skipping the response body drain on the updateStatus() non-ok path in apps/indexer/src/chain/nodeInfo.ts (unlike the query() path, which reads the body via response.json().catch()) could exhaust the undici connection pool. Given the low request volume and periodic polling nature of node-status checks, this doesn't rise to a real issue and needs no action.
Extended reasoning...
This run re-examined the PR (no new commits since the prior bug-hunting pass) and found no new bugs beyond the two already posted as inline comments (the DOMException.message mutation regression in nodeInfo.ts/nodeAccessor.ts, and the missing ok-check in ipLocationProvider.ts). One additional candidate was investigated and ruled out: an unconsumed response body on the non-2xx path of updateStatus() in nodeInfo.ts potentially exhausting the undici connection pool. Given the low, periodic call volume of node status polling, this isn't a practical concern. Deferring rather than approving since the existing critical inline finding (DOMException message mutation) is still unresolved.
There was a problem hiding this comment.
No new issues from this run. This PR needed several correction rounds already (timeout-error handling, non-2xx JSON parsing, response body draining) to get the fetch migration behaviorally correct, so I'd like a human to confirm the final state before merge rather than rely on my read alone.
What was reviewed:
- Confirmed the previously flagged issues are now fixed in the current commit: the DOMException message-mutation bug in nodeAccessor.ts (now wraps in a new Error with cause), the missing response.ok checks in ipLocationProvider.ts and priceHistoryProvider.ts, and non-JSON error body handling in nodeInfo.ts (now reads the body once via response.text() and keeps raw text when it isn't JSON).
- Checked the non-2xx / timeout error paths across nodeInfo.ts, nodeAccessor.ts, providerStatusProvider.ts, rest.ts against the PR's stated invariant that non-2xx responses still throw.
- Investigated claims that unread response bodies on non-2xx paths in providerStatusProvider.ts/rest.ts leak undici connections — ruled out this run.
Extended reasoning...
This PR replaces axios and node-fetch with native fetch across the indexer's HTTP call sites: blockchain node RPC/status queries (nodeInfo.ts, nodeAccessor.ts), provider version/status checks including a self-signed-cert dispatcher (providerStatusProvider.ts, rest.ts, shared/utils/fetch.ts), and two external data providers (keybaseProvider.ts, priceHistoryProvider.ts). It also drops axios/node-fetch and adds undici as a direct dependency.
Security risks
No new security-sensitive surface is introduced. The self-signed-cert bypass (rejectUnauthorized: false) is carried over from the prior axios httpsAgent configuration, not newly introduced, and is scoped to provider status/version calls as before. No auth, crypto, or permission logic is touched.
Level of scrutiny
This warrants more than a rubber stamp. Native fetch has different failure semantics than axios (no automatic reject on non-2xx, DOMException instead of a plain Error on timeout, one-shot body streams), and this PR's own history shows those differences bit it in practice: my prior bug-hunting pass caught a genuine regression (timeout errors getting swapped for an opaque TypeError due to mutating a getter-only DOMException.message), and CodeRabbit caught missing ok-checks and body-draining/preservation issues. All of those have since been fixed in commits 0d02834 and 5847644, and this run's bug hunter found nothing further, including ruling out a connection-pool-exhaustion theory on the remaining non-2xx paths. Given the code now touches blockchain sync reliability (nodeAccessor/nodeInfo feed chainSync's downloadBlock/downloadBlocks) and required multiple correction rounds to get right, a human sanity-check of the final diff is warranted before merge.
Other factors
Codecov reports full coverage of modified/coverable lines, but there are no dedicated unit tests for the new HttpResponseError/toHttpResponseError error-shaping logic or the timeout/ok-check branches added to nodeInfo.ts, nodeAccessor.ts, ipLocationProvider.ts, or priceHistoryProvider.ts — the coverage is presumably incidental from existing higher-level tests rather than targeted regression tests for the fetch-migration edge cases that were just fixed.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/indexer/src/db/priceHistoryProvider.ts`:
- Around line 22-25: Update the non-OK response branches in
apps/indexer/src/db/priceHistoryProvider.ts lines 22-25 and
apps/indexer/src/providers/ipLocationProvider.ts lines 20-23 to consume the
response body before throwing, include a bounded text representation alongside
the endpoint or IP and status, and preserve the existing error behavior
otherwise.
In `@apps/indexer/src/providers/ipLocationProvider.ts`:
- Around line 18-23: Update the fetch endpoint in getIpLocation to use the HTTPS
scheme for the IP-location request, preserving the existing response validation
and error handling.
🪄 Autofix
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: CHILL
Plan: Pro
Run ID: cf1e03fa-88f8-475b-93c1-8986c5f79530
📒 Files selected for processing (4)
apps/indexer/src/chain/nodeAccessor.tsapps/indexer/src/chain/nodeInfo.tsapps/indexer/src/db/priceHistoryProvider.tsapps/indexer/src/providers/ipLocationProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/indexer/src/chain/nodeInfo.ts
10c5fe0 to
19dabb7
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Swap the axios HTTP client for Node's native fetch across the indexer. Behavior is preserved: non-2xx responses still throw (via an explicit ok check or a small HttpResponseError carrying status and body), timeouts use AbortSignal.timeout, and provider status/version calls keep bypassing self-signed cert verification through an undici Agent dispatcher. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Response.json() resolves to Promise<unknown> under undici-types (IDE) but Promise<any> under DOM lib (CLI tsc). Cast/annotate the parsed bodies so both resolutions type-check identically instead of relying on implicit any. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
node-fetch was not imported anywhere in the source; Node's global fetch covers all HTTP calls. Remove it along with its now-orphaned @types/node-fetch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eduler Drop the remaining node-fetch imports in favour of Node's global fetch. Behavior is unchanged (node-fetch v2 and global fetch both leave non-2xx responses unthrown, follow redirects and apply no timeout here); the parsed json() bodies are cast explicitly so they type-check under both undici-types and DOM lib resolutions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Node's fetch rejects timeouts with a getter-only DOMException, so mutating err.message in NodeAccessor swallowed the real error with a TypeError. Wrap instead, and read non-2xx bodies once as text so plain-text errors survive and the connection returns to the undici pool. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…errors Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
19dabb7 to
15cd25f
Compare
2 participants
Why
Closes CON-169
What
Swap the axios HTTP client and node-fetch for Node's native fetch across the indexer. Behavior is preserved: non-2xx responses still throw (via an explicit ok check or a small HttpResponseError carrying status and body), timeouts use AbortSignal.timeout, and provider status/version calls keep bypassing self-signed cert verification through an undici Agent dispatcher.
Summary by CodeRabbit