Closed
Conversation
this supports objects with sendPayment and payInvoice functions. The reason is that webln and nwc use different method names here.
📝 WalkthroughWalkthroughThe Wallet interface now accepts either Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/l402/l402.ts (2)
60-61: Good fallback selection and binding; add branch tests forpayInvoice.The dynamic selection and
.bind(wallet)usage look correct. Please add tests for (1)payInvoice-only wallets and (2) wallets with both methods to lock precedence behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/l402/l402.ts` around lines 60 - 61, Add unit tests covering both branches around the dynamic payment selection: (1) create a wallet stub exposing only payInvoice and assert that calling the code path which uses payFn invokes wallet.payInvoice (verify the method was called and that its `this` is the wallet instance) and (2) create a wallet stub exposing both sendPayment and payInvoice and assert sendPayment takes precedence (verify sendPayment was called, payInvoice was not, and `this` binding is the wallet). Target the symbols payFn, wallet.sendPayment, wallet.payInvoice, and the invocation that produces invResp to ensure correct method selection and binding.
8-9: Prefer a type that enforces at least one payment method at compile time.Current optional/optional shape allows invalid
Walletobjects in TS ({}), and pushes all safety to runtime. Consider a union type so TS callers must providesendPaymentorpayInvoice.Proposed type refactor
-interface Wallet { - sendPayment?(paymentRequest: string): Promise<{ preimage: string }>; - payInvoice?(paymentRequest: string): Promise<{ preimage: string }>; -} +type PaymentResult = { preimage: string }; + +type Wallet = + | { + sendPayment(paymentRequest: string): Promise<PaymentResult>; + payInvoice?: (paymentRequest: string) => Promise<PaymentResult>; + } + | { + payInvoice(paymentRequest: string): Promise<PaymentResult>; + sendPayment?: (paymentRequest: string) => Promise<PaymentResult>; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/l402/l402.ts` around lines 8 - 9, Replace the two optional methods with a union type that enforces at least one payment method at compile time: define two specific shapes (e.g., WalletWithSend { sendPayment(paymentRequest: string): Promise<{ preimage: string }> } and WalletWithPay { payInvoice(paymentRequest: string): Promise<{ preimage: string }> }) and set Wallet = WalletWithSend | WalletWithPay (allowing both implicitly via structural typing); update any references to the original optional signatures (sendPayment, payInvoice) to use the new Wallet type so callers must provide at least one implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/l402/l402.ts`:
- Around line 29-31: The current validation checks only truthiness for
wallet.sendPayment and wallet.payInvoice, which allows non-function values to
pass and later cause a crash when calling .bind; update the check in the l402
module so you verify callability (e.g., typeof wallet.sendPayment === 'function'
or typeof wallet.payInvoice === 'function') and throw an Error if neither is a
function; then ensure the subsequent usage that calls .bind on the chosen method
(the sendPayment/payInvoice reference used later) only happens after selecting a
verified function.
---
Nitpick comments:
In `@src/l402/l402.ts`:
- Around line 60-61: Add unit tests covering both branches around the dynamic
payment selection: (1) create a wallet stub exposing only payInvoice and assert
that calling the code path which uses payFn invokes wallet.payInvoice (verify
the method was called and that its `this` is the wallet instance) and (2) create
a wallet stub exposing both sendPayment and payInvoice and assert sendPayment
takes precedence (verify sendPayment was called, payInvoice was not, and `this`
binding is the wallet). Target the symbols payFn, wallet.sendPayment,
wallet.payInvoice, and the invocation that produces invResp to ensure correct
method selection and binding.
- Around line 8-9: Replace the two optional methods with a union type that
enforces at least one payment method at compile time: define two specific shapes
(e.g., WalletWithSend { sendPayment(paymentRequest: string): Promise<{ preimage:
string }> } and WalletWithPay { payInvoice(paymentRequest: string): Promise<{
preimage: string }> }) and set Wallet = WalletWithSend | WalletWithPay (allowing
both implicitly via structural typing); update any references to the original
optional signatures (sendPayment, payInvoice) to use the new Wallet type so
callers must provide at least one implementation.
Comment on lines
+29
to
+31
| if (!wallet.sendPayment && !wallet.payInvoice) { | ||
| throw new Error("wallet must have a sendPayment or payInvoice function"); | ||
| } |
There was a problem hiding this comment.
Harden method validation to avoid runtime bind crashes.
Line 29 currently validates truthiness, not callability. A non-function truthy value would pass and fail at Line 60 when calling .bind(...).
Proposed guard fix
- if (!wallet.sendPayment && !wallet.payInvoice) {
+ if (
+ typeof wallet.sendPayment !== "function" &&
+ typeof wallet.payInvoice !== "function"
+ ) {
throw new Error("wallet must have a sendPayment or payInvoice function");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/l402/l402.ts` around lines 29 - 31, The current validation checks only
truthiness for wallet.sendPayment and wallet.payInvoice, which allows
non-function values to pass and later cause a crash when calling .bind; update
the check in the l402 module so you verify callability (e.g., typeof
wallet.sendPayment === 'function' or typeof wallet.payInvoice === 'function')
and throw an Error if neither is a function; then ensure the subsequent usage
that calls .bind on the chosen method (the sendPayment/payInvoice reference used
later) only happens after selecting a verified function.
rolznz
reviewed
Mar 5, 2026
|
|
||
| interface Wallet { | ||
| sendPayment(paymentRequest: string): Promise<{ preimage: string }>; | ||
| sendPayment?(paymentRequest: string): Promise<{ preimage: string }>; |
Contributor
There was a problem hiding this comment.
to keep type safety we can do:
type Wallet =
| { sendPayment(paymentRequest: string): Promise<{ preimage: string }> }
| { payInvoice(paymentRequest: string): Promise<{ preimage: string }> };
Contributor
|
My bot says it is more confusing. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/l402.js (1)
16-26:⚠️ Potential issue | 🟡 MinorWrap the NWC lifecycle in
try/finally.
nwc.close()only runs after the happy-path JSON parse. Ifenable(),fetchWithL402(), orresponse.json()throws, the websocket stays open and the example can hang.Proposed fix
-await nwc.enable(); -nwc.on("sendPayment", (response) => { - console.info(`payment response:`, response); -}); - -fetchWithL402(url, {}, { wallet: nwc }) - .then((response) => response.json()) - .then((data) => { - console.info(data); - nwc.close(); - }); +try { + await nwc.enable(); + nwc.on("sendPayment", (response) => { + console.info(`payment response:`, response); + }); + + const response = await fetchWithL402(url, {}, { wallet: nwc }); + const data = await response.json(); + console.info(data); +} finally { + nwc.close(); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/l402.js` around lines 16 - 26, The NWC websocket may remain open on errors because nwc.close() is only called on the happy path; wrap the lifecycle in a try/finally: call await nwc.enable() then run fetchWithL402(...) and response.json() inside the try block, and ensure nwc.close() is invoked in the finally block (rethrow any caught errors if needed) so enable(), fetchWithL402(), or response.json() failures still close the connection.
♻️ Duplicate comments (1)
src/l402/l402.ts (1)
29-31:⚠️ Potential issue | 🟠 MajorValidate callable methods, not just truthy properties.
{ sendPayment: true }or{ payInvoice: {} }passes this guard, then Line 61 or Line 62 crashes with... is not a function. Check callability before accepting the wallet.Proposed fix
- if (!wallet.sendPayment && !wallet.payInvoice) { + if ( + typeof wallet.sendPayment !== "function" && + typeof wallet.payInvoice !== "function" + ) { throw new Error("wallet must have a sendPayment or payInvoice function"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/l402/l402.ts` around lines 29 - 31, The guard currently only checks truthiness of wallet.sendPayment and wallet.payInvoice; instead ensure at least one is callable by checking typeof wallet.sendPayment === "function" or typeof wallet.payInvoice === "function" (or using a small isFunction helper) and throw the existing Error if neither is a function; update the check around the wallet validation (the block referencing wallet.sendPayment and wallet.payInvoice) so subsequent calls to sendPayment/payInvoice won't crash when non-function values are passed.
🧹 Nitpick comments (1)
src/l402/l402.ts (1)
60-62: Add coverage for thepayInvoicefallback.
src/l402/l402.test.ts:16-20still only builds wallets withsendPayment, so the new branch at Line 62 can regress unnoticed. Please add a case with a wallet that only implementspayInvoiceand assert its returned preimage is used for the retry request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/l402/l402.ts` around lines 60 - 62, Add a unit test in src/l402/l402.test.ts that builds a wallet object which does NOT implement sendPayment but only implements payInvoice, then call the same flow that triggers the code path using wallet.payInvoice (the branch where invResp is assigned from payInvoice) and assert the returned preimage from payInvoice is included in the subsequent retry request; target the wallet variable used in the test and the invResp behavior so the branch at the ternary (sendPayment ? ... : await wallet.payInvoice!({ invoice })) is covered and the retry request payload uses the payInvoice preimage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@examples/l402.js`:
- Around line 16-26: The NWC websocket may remain open on errors because
nwc.close() is only called on the happy path; wrap the lifecycle in a
try/finally: call await nwc.enable() then run fetchWithL402(...) and
response.json() inside the try block, and ensure nwc.close() is invoked in the
finally block (rethrow any caught errors if needed) so enable(),
fetchWithL402(), or response.json() failures still close the connection.
---
Duplicate comments:
In `@src/l402/l402.ts`:
- Around line 29-31: The guard currently only checks truthiness of
wallet.sendPayment and wallet.payInvoice; instead ensure at least one is
callable by checking typeof wallet.sendPayment === "function" or typeof
wallet.payInvoice === "function" (or using a small isFunction helper) and throw
the existing Error if neither is a function; update the check around the wallet
validation (the block referencing wallet.sendPayment and wallet.payInvoice) so
subsequent calls to sendPayment/payInvoice won't crash when non-function values
are passed.
---
Nitpick comments:
In `@src/l402/l402.ts`:
- Around line 60-62: Add a unit test in src/l402/l402.test.ts that builds a
wallet object which does NOT implement sendPayment but only implements
payInvoice, then call the same flow that triggers the code path using
wallet.payInvoice (the branch where invResp is assigned from payInvoice) and
assert the returned preimage from payInvoice is included in the subsequent retry
request; target the wallet variable used in the test and the invResp behavior so
the branch at the ternary (sendPayment ? ... : await wallet.payInvoice!({
invoice })) is covered and the retry request payload uses the payInvoice
preimage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad029792-1364-4791-b20e-99253724fd73
📒 Files selected for processing (2)
examples/l402.jssrc/l402/l402.ts
Contributor
Author
|
we decided to keep it simple and just follow the NWC spec |
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.
this supports objects with sendPayment and payInvoice functions.
The reason is that webln and nwc use different method names here.
I am not sure if it makes sense.
Summary by CodeRabbit
New Features
Improvements