error mapping normalization#45
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughWalkthroughThis PR standardizes provider error handling across the TypeScript bindings and Rust core crate. It adds shared normalization helpers, operation-aware request plumbing, richer ChangesTypeScript bindings error normalization
Rust core crate error normalization
Error normalization documentation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Adapter as Node adapter
participant Helper as normalizeProviderError
participant Error as NwcError
Adapter->>Helper: throwNormalizedProviderError(error, options)
Helper->>Helper: extract provider info and map code/message/status
Helper->>Error: construct normalized NwcError
Error-->>Adapter: throw provider metadata on error
sequenceDiagram
participant Api as Provider API module
participant Core as error_normalization
participant ApiError as ApiError::Nwc
Api->>Core: transport_error / provider_error_from_response
Core->>Core: map provider code, message, and HTTP status
Core->>ApiError: build normalized ApiError::Nwc
ApiError-->>Api: return structured error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f44e00950b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
🧹 Nitpick comments (6)
crates/lni/phoenixd/api.rs (1)
309-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant double
map_err; first mapping is dead.The first
.map_err(|e| ApiError::Json { ... })produces a value that the second.map_err(|_| ...)immediately discards, so theFailed to parse pay_invoice responsemessage is never used. Collapse to a single conversion (matching thematchstyle used inpay_offer).♻️ Proposed simplification
- let pay_invoice_resp: PhoenixPayInvoiceResp = serde_json::from_str(&response_text) - .map_err(|e| ApiError::Json { - reason: format!("Failed to parse pay_invoice response: {}", e), - }) - .map_err(|_| phoenixd_error_from_body(None, response_text.clone()))?; + let pay_invoice_resp: PhoenixPayInvoiceResp = match serde_json::from_str(&response_text) { + Ok(resp) => resp, + Err(_) => return Err(phoenixd_error_from_body(None, response_text.clone())), + };🤖 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 `@crates/lni/phoenixd/api.rs` around lines 309 - 313, The `pay_invoice_resp` parsing path has a redundant double `map_err`, where the `ApiError::Json` from `serde_json::from_str` is immediately discarded by the next `map_err`. In `phoenixd/api.rs`, simplify the `pay_invoice_resp` conversion to a single error-handling step, following the same `match`-based pattern used in `pay_offer`, so the parse failure is handled in one place and no dead error mapping remains.crates/lni/strike/api.rs (1)
87-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffStrike duplicates the shared
error_normalizationhelpers.Every other adapter in this PR consumes
crate::error_normalization(ProviderErrorInfo,provider_info_from_body,map_http_status,provider_error_from_response,transport_error), but Strike re-implements equivalents (StrikeProviderErrorInfo,json_string_field,json_u16_field,map_provider_http_status,strike_nwc_error_from_transport). Consider reusing the shared module with a Strike-specificmap_provider_code, matching the CLN/LND/Phoenixd/Speed pattern, to reduce divergence.🤖 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 `@crates/lni/strike/api.rs` around lines 87 - 196, Strike is duplicating the shared error-normalization logic instead of using crate::error_normalization. Refactor strike_nwc_error_from_response and strike_nwc_error_from_transport to build on ProviderErrorInfo, provider_info_from_body, map_http_status, provider_error_from_response, and transport_error, and keep only a Strike-specific map_provider_code equivalent for any provider code translation. Remove the local StrikeProviderErrorInfo/json_* helpers so Strike follows the same CLN/LND/Phoenixd/Speed pattern and stays consistent with the shared adapter behavior.bindings/typescript/src/internal/error-normalization.ts (1)
188-223: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRewrapping an already-normalized
NwcErroris a bit wasteful.When
error instanceof NwcError(Line 192), a brand-newNwcErroris constructed withcause: error, purely to potentially overrideoperation/provider. Since all other fields are copied verbatim, in the common case whereoptions.operation/options.providermatch the existing values this just adds an extra wrapping layer/allocation. Not incorrect, just avoidable churn.🤖 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 `@bindings/typescript/src/internal/error-normalization.ts` around lines 188 - 223, In normalizeProviderError, avoid rewrapping an input that is already an NwcError unless you actually need to change operation or provider. Check the existing error’s operation/provider against options.operation and options.provider, and return the original error unchanged when they already match; only construct a new NwcError in the NwcError branch when those fields need overriding, keeping the rest of the copied fields and cause behavior as-is.bindings/typescript/src/nodes/blink.ts (2)
421-431: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider
NOT_FOUNDinstead ofOTHERfor missing BTC wallet.
getBtcWalletthrowsblinkNwcError('OTHER', ...)when no BTC wallet exists in the account.NOT_FOUNDwould better convey the failure mode to NWC clients and is consistent with how other adapters signal missing resources.🤖 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 `@bindings/typescript/src/nodes/blink.ts` around lines 421 - 431, The getBtcWallet method currently throws blinkNwcError with OTHER when no BTC wallet is found; change that error to use NOT_FOUND so the missing-resource failure is signaled correctly to NWC clients. Update the throw in getBtcWallet while keeping the existing message and context, and make sure the BlinkWallet lookup logic and cachedWalletId assignment remain unchanged.
256-262: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueOnly the first GraphQL error's
codeis used for classification.
blinkProviderInfoFromErrorsjoins all error messages but only readscodefromerrors[0]. If Blink returns multiple errors with different codes (e.g., a benign validation error first and anUNAUTHORIZEDerror second), the more actionable code could be missed.🤖 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 `@bindings/typescript/src/nodes/blink.ts` around lines 256 - 262, The error aggregation in blinkProviderInfoFromErrors only classifies by the first GraphQLError, so it can miss a more relevant code when Blink returns multiple errors. Update blinkProviderInfoFromErrors in blink.ts to inspect all errors and derive the ProviderErrorInfo.code from the most actionable error rather than errors[0], while still joining all messages for ProviderErrorInfo.message.bindings/typescript/src/nodes/strike.ts (1)
531-534: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBolt12 "not implemented" paths still use generic
LniError, unlike Blink's equivalent.
createInvoice's Bolt12 guard andcreateOffer/getOffer/listOffers/payOfferstillthrow new LniError('Api', 'Bolt12 is not implemented for StrikeNode.'), whereas the equivalent Blink methods in this same PR were migrated toblinkNwcError('NOT_IMPLEMENTED', ...)with properoperationtagging. Worth aligning for consistency across adapters, though low urgency since the feature itself is unimplemented either way.Also applies to: 762-776
🤖 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 `@bindings/typescript/src/nodes/strike.ts` around lines 531 - 534, The Bolt12 unsupported paths in StrikeNode still throw the generic LniError, unlike the Blink adapter’s NOT_IMPLEMENTED handling. Update StrikeNode’s createInvoice and the related Bolt12 methods createOffer, getOffer, listOffers, and payOffer to use the same not-implemented error helper/pattern as Blink, including the correct operation tagging and a consistent NOT_IMPLEMENTED-style error shape instead of LniError.
🤖 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 `@bindings/typescript/src/internal/error-normalization.ts`:
- Around line 34-105: The recursive search in findStringProperty and
findNumberProperty can walk arbitrarily deep provider JSON and overflow the
stack. Add a bounded traversal to both helpers, either by threading a
max-depth/step limit through providerInfoFromJsonErrorBody or by switching to an
iterative queue-based search with a visited/bound cap. Keep
normalizeProviderError calling the guarded helpers so malformed or
pathologically nested provider payloads still normalize into NwcError instead of
throwing.
In `@bindings/typescript/src/nodes/lnd.ts`:
- Around line 358-361: The FAILED branch in lnd.ts is mapping missing or
unrecognized failure_reason values through mapLndFailureReason, which can
incorrectly label a definite payment failure as OTHER. Update the
wrapped.result.status === 'FAILED' handling in the pay_invoice flow to default
to PAYMENT_FAILED when the status is FAILED, and only use mapLndFailureReason
when a specific known failure reason is present. Keep the existing lndNwcError
call and reason handling, but ensure the status-to-NWC error mapping in this
branch always surfaces PAYMENT_FAILED for failed payments.
In `@bindings/typescript/src/nodes/strike.ts`:
- Around line 225-319: The Strike error parsing code is duplicating shared
normalization logic and has drifted from
`bindings/typescript/src/internal/error-normalization.ts`. Update
`findNumberProperty`/`strikeProviderErrorInfo` in `strike.ts` to reuse the
shared helpers instead of reimplementing them, so numeric-string status values
are handled consistently and parse failures preserve the raw body text as the
message. Align the JSON parsing and deep-search flow with
`providerInfoFromJsonErrorBody`, and keep `throwStrikeError` wired through
`throwNormalizedProviderError` with the shared extraction behavior.
In `@crates/lni/blink/api.rs`:
- Line 164: The response body read in api.rs should not panic via
response.text().await.unwrap(); instead, route the body read through the same
structured error handling used in the surrounding code. Update the logic around
the response_text assignment in the relevant response-processing function to
capture the text read failure and convert it into the existing error type with
map_err or equivalent, so the request path fails gracefully rather than
unwinding on a mid-stream disconnect.
---
Nitpick comments:
In `@bindings/typescript/src/internal/error-normalization.ts`:
- Around line 188-223: In normalizeProviderError, avoid rewrapping an input that
is already an NwcError unless you actually need to change operation or provider.
Check the existing error’s operation/provider against options.operation and
options.provider, and return the original error unchanged when they already
match; only construct a new NwcError in the NwcError branch when those fields
need overriding, keeping the rest of the copied fields and cause behavior as-is.
In `@bindings/typescript/src/nodes/blink.ts`:
- Around line 421-431: The getBtcWallet method currently throws blinkNwcError
with OTHER when no BTC wallet is found; change that error to use NOT_FOUND so
the missing-resource failure is signaled correctly to NWC clients. Update the
throw in getBtcWallet while keeping the existing message and context, and make
sure the BlinkWallet lookup logic and cachedWalletId assignment remain
unchanged.
- Around line 256-262: The error aggregation in blinkProviderInfoFromErrors only
classifies by the first GraphQLError, so it can miss a more relevant code when
Blink returns multiple errors. Update blinkProviderInfoFromErrors in blink.ts to
inspect all errors and derive the ProviderErrorInfo.code from the most
actionable error rather than errors[0], while still joining all messages for
ProviderErrorInfo.message.
In `@bindings/typescript/src/nodes/strike.ts`:
- Around line 531-534: The Bolt12 unsupported paths in StrikeNode still throw
the generic LniError, unlike the Blink adapter’s NOT_IMPLEMENTED handling.
Update StrikeNode’s createInvoice and the related Bolt12 methods createOffer,
getOffer, listOffers, and payOffer to use the same not-implemented error
helper/pattern as Blink, including the correct operation tagging and a
consistent NOT_IMPLEMENTED-style error shape instead of LniError.
In `@crates/lni/phoenixd/api.rs`:
- Around line 309-313: The `pay_invoice_resp` parsing path has a redundant
double `map_err`, where the `ApiError::Json` from `serde_json::from_str` is
immediately discarded by the next `map_err`. In `phoenixd/api.rs`, simplify the
`pay_invoice_resp` conversion to a single error-handling step, following the
same `match`-based pattern used in `pay_offer`, so the parse failure is handled
in one place and no dead error mapping remains.
In `@crates/lni/strike/api.rs`:
- Around line 87-196: Strike is duplicating the shared error-normalization logic
instead of using crate::error_normalization. Refactor
strike_nwc_error_from_response and strike_nwc_error_from_transport to build on
ProviderErrorInfo, provider_info_from_body, map_http_status,
provider_error_from_response, and transport_error, and keep only a
Strike-specific map_provider_code equivalent for any provider code translation.
Remove the local StrikeProviderErrorInfo/json_* helpers so Strike follows the
same CLN/LND/Phoenixd/Speed pattern and stays consistent with the shared adapter
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a55aa942-9d9c-44fd-9635-b08645937e96
📒 Files selected for processing (20)
bindings/typescript/src/__tests__/provider-error-normalization.test.tsbindings/typescript/src/__tests__/strike.test.tsbindings/typescript/src/errors.tsbindings/typescript/src/internal/error-normalization.tsbindings/typescript/src/nodes/blink.tsbindings/typescript/src/nodes/cln.tsbindings/typescript/src/nodes/lnd.tsbindings/typescript/src/nodes/phoenixd.tsbindings/typescript/src/nodes/speed.tsbindings/typescript/src/nodes/strike.tsbindings/typescript/sunnyln-lni-0.2.13.tgzcrates/lni/blink/api.rscrates/lni/cln/api.rscrates/lni/error_normalization.rscrates/lni/lib.rscrates/lni/lnd/api.rscrates/lni/phoenixd/api.rscrates/lni/speed/api.rscrates/lni/strike/api.rsdocs/error-normalization.md
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 `@bindings/typescript/src/__tests__/provider-error-normalization.test.ts`:
- Around line 22-47: The pathological payload test in normalizeProviderError is
not actually exercising the bounded traversal because providerInfo is never
extracted from the JSON body. Update the test to pass extractProviderError using
providerInfoFromJsonErrorBody so the deep body is parsed and findProperty
performs the BFS traversal capped by MAX_PROVIDER_ERROR_SEARCH_STEPS; keep the
existing assertions on the NwcError result.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9408996-3c68-4a5a-9efe-47d0ddc60863
📒 Files selected for processing (11)
bindings/typescript/.gitignorebindings/typescript/src/__tests__/provider-error-normalization.test.tsbindings/typescript/src/__tests__/strike.test.tsbindings/typescript/src/internal/error-normalization.tsbindings/typescript/src/nodes/blink.tsbindings/typescript/src/nodes/lnd.tsbindings/typescript/src/nodes/strike.tscrates/lni/blink/api.rscrates/lni/error_normalization.rscrates/lni/phoenixd/api.rscrates/lni/strike/api.rs
✅ Files skipped from review due to trivial changes (1)
- bindings/typescript/.gitignore
🚧 Files skipped from review as they are similar to previous changes (7)
- bindings/typescript/src/tests/strike.test.ts
- crates/lni/error_normalization.rs
- bindings/typescript/src/internal/error-normalization.ts
- crates/lni/phoenixd/api.rs
- bindings/typescript/src/nodes/lnd.ts
- crates/lni/blink/api.rs
- bindings/typescript/src/nodes/blink.ts
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.
Summary by CodeRabbit