fix: improve faucet error handling #3118
Conversation
WalkthroughThis PR improves faucet error handling by refactoring response processing logic in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR improves error handling in the faucet funding functionality by refactoring how error responses are processed when funding wallet requests fail.
- Moved body parsing into the error handling function to avoid premature consumption of the response stream
- Added proper error data structure with type safety
- Enhanced error handling to gracefully fallback to text when JSON parsing fails
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/xrpl/src/Wallet/fundWallet.ts | Refactored error handling by moving response body parsing into processError, added ErrorData interface, and implemented fallback to text response on JSON parse failure |
| packages/xrpl/HISTORY.md | Added changelog entry documenting the improved faucet error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| async function processError(response: Response): Promise<never> { | ||
| const errorData: ErrorData = { | ||
| contentType: response.headers.get('Content-Type') ?? undefined, |
There was a problem hiding this comment.
The ?? undefined is redundant since response.headers.get('Content-Type') already returns string | null. When assigned to an optional property (contentType?: string), null will be automatically converted to undefined without the explicit coalescing operator.
| contentType: response.headers.get('Content-Type') ?? undefined, | |
| contentType: response.headers.get('Content-Type'), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xrpl/src/Wallet/fundWallet.ts (1)
160-169: Guard JSON shape to prevent TypeError on malformed success bodies.Accessing
account.classicAddresswithout checks can throw if the faucet returns unexpected JSON (still 2xx). Use optional chaining so the existing undefined check handles it gracefully.Apply:
- const body: unknown = await response.json() - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- It's a FaucetWallet - const classicAddress = (body as FaucetWallet).account.classicAddress + const body: unknown = await response.json() + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Best‑effort extraction with guards + const classicAddress = (body as FaucetWallet | undefined)?.account?.classicAddress
🧹 Nitpick comments (2)
packages/xrpl/test/faucet/fundWallet.test.ts (1)
126-126: Make error assertion less brittle.Matching the entire serialized string (incl. exact header value and field order) risks flakes. Prefer checking type and key parts (statusCode, error/detail) or parsing the JSON substring from error.message.
Apply one option:
- expect(error).toEqual( - new XRPLFaucetError( - 'Request failed: {"contentType":"application/json; charset=utf-8","statusCode":400,"body":{"error":"Invalid amount","detail":"Must be an integer"}}', - ), - ) + expect(error).toBeInstanceOf(XRPLFaucetError) + const msg = (error as XRPLFaucetError).message + expect(msg).toContain('"statusCode":400') + expect(msg).toContain('"Invalid amount"') + expect(msg).toContain('"Must be an integer"')packages/xrpl/src/Wallet/fundWallet.ts (1)
208-229: Enrich error context; optionally cap huge bodies.Add
urlto aid debugging which faucet endpoint failed, and (optionally) truncate very large text bodies to avoid noisy logs while preserving JSON bodies intact.Apply:
interface ErrorData { body?: unknown contentType?: string statusCode: number + url?: string } async function processError(response: Response): Promise<never> { const errorData: ErrorData = { contentType: response.headers.get('Content-Type') ?? undefined, statusCode: response.status, + url: response.url, } const clone = response.clone() try { const body: unknown = await response.json() errorData.body = body } catch { - errorData.body = await clone.text() + const text = await clone.text() + errorData.body = text.length > 2000 ? `${text.slice(0, 2000)}… [truncated]` : text } return Promise.reject( new XRPLFaucetError(`Request failed: ${JSON.stringify(errorData)}`), ) }Note: If you adopt truncation, update the test to assert key substrings rather than the full string.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/Wallet/fundWallet.ts(3 hunks)packages/xrpl/test/faucet/fundWallet.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xrpl/src/Wallet/fundWallet.ts (1)
packages/xrpl/src/errors.ts (1)
XRPLFaucetError(158-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: browser (24.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (22.x)
- GitHub Check: integration (24.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (22.x)
🔇 Additional comments (3)
packages/xrpl/HISTORY.md (1)
12-12: Changelog entry reads well.Clear and scoped; matches the PR intent.
packages/xrpl/test/faucet/fundWallet.test.ts (1)
129-131: Finally block for disconnect is correct.Ensures cleanup on both success and error paths.
packages/xrpl/src/Wallet/fundWallet.ts (1)
170-170: Centralized error handling LGTM.Routing all non-JSON/!ok responses through
processErrorimproves diagnosability.
High Level Overview of Change
This PR improves faucet error handling.
When the faucet fails in such a way that it does not deliver a JSON response, you get an incredibly opaque error:
This PR changes that so instead of getting that error, we instead get an HTML output:
Context of Change
The WASM Devnet faucet was down and the error was tricky to debug
Type of Change
Did you update HISTORY.md?
Test Plan
Tested locally with the WASM Devnet faucet, which is currently down. I couldn't figure out a good way to write a repeatable test, though.