feat(e2e): add NWC contract tests for get_*, hold, multi, and notifications#538
feat(e2e): add NWC contract tests for get_*, hold, multi, and notifications#538DSanich wants to merge 8 commits intogetAlby:masterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request adds comprehensive end-to-end test coverage for Nostr Wallet Connect (NWC) functionality, including balance retrieval, payment operations (standard and multi-pay), hold invoice lifecycle, message signing, notifications, and transaction listing. It also adjusts an existing faucet test to use a tolerance-based balance assertion. 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 |
|
@rolznz Hi! I continued my work on e2e tests and prepared this PR. I hope it will be as useful as the previous ones. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
e2e/nwc-multi-pay-invoice.test.ts (1)
57-61: Relax stricterrorsexpectation to reduce E2E flakiness.Line 59 requires zero per-item errors, which is brittle in network-backed runs. Prefer contract-shape checks plus success presence.
Proposed assertion adjustment
expect(Array.isArray(multiPayResult.invoices)).toBe(true); expect(multiPayResult.invoices.length).toBe(2); - expect(multiPayResult.errors).toEqual([]); - expect(multiPayResult.invoices[0].preimage).toBeDefined(); - expect(multiPayResult.invoices[1].preimage).toBeDefined(); + expect(Array.isArray(multiPayResult.errors)).toBe(true); + expect( + multiPayResult.invoices.filter((invoice) => invoice.preimage).length, + ).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nwc-multi-pay-invoice.test.ts` around lines 57 - 61, The test is brittle because it asserts multiPayResult.errors strictly equals an empty array; instead, verify the contract shape and presence of success: ensure multiPayResult.errors is an array (use Array.isArray on multiPayResult.errors) and that its length matches multiPayResult.invoices.length to keep the per-item error array shape, while keeping the existing checks that multiPayResult.invoices[0].preimage and multiPayResult.invoices[1].preimage are defined to assert success presence.e2e/nwc-multi-pay-keysend.test.ts (1)
51-55: Use contract-safe assertions for multi-keysend results.Line 53’s
errors === []check is fragile for E2E network variability. Assert shape and successful results instead.Proposed assertion adjustment
expect(Array.isArray(multiPayResult.keysends)).toBe(true); expect(multiPayResult.keysends.length).toBe(2); - expect(multiPayResult.errors).toEqual([]); - expect(multiPayResult.keysends[0].preimage).toBeDefined(); - expect(multiPayResult.keysends[1].preimage).toBeDefined(); + expect(Array.isArray(multiPayResult.errors)).toBe(true); + expect( + multiPayResult.keysends.filter((keysend) => keysend.preimage).length, + ).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nwc-multi-pay-keysend.test.ts` around lines 51 - 55, The test currently asserts multiPayResult.errors equals an empty array which is fragile; instead assert the result shape and per-entry success: verify multiPayResult.keysends is an array of length 2, verify multiPayResult.errors is an array (type check) and that every entry in multiPayResult.keysends has no per-entry error and has a defined preimage (e.g. check each keysend object for absence/falsey of an error field and that preimage is defined). Use the existing symbols multiPayResult, keysends, and errors to locate and update the assertions so they assert type/shape and per-keysend success rather than strict equality to [].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/nwc-get-balance.test.ts`:
- Line 23: The assertion using exact equality on balanceResult.balance vs
BALANCE_MSATS is too strict for a network-backed faucet; change the assertion to
allow a small tolerance or a minimum bound instead of exact match. Update the
test that contains expect(balanceResult.balance).toBe(BALANCE_MSATS) to assert a
range (e.g., greater-than-or-equal to BALANCE_MSATS minus a small delta, or use
toBeGreaterThanOrEqual with a tolerance variable) so BALANCE_MSATS and
balanceResult.balance are compared with a tolerance to avoid flakiness.
In `@e2e/nwc-notifications-payment-received.test.ts`:
- Around line 46-73: The test races because senderClient.payInvoice is called
before the subscription established by receiverClient.subscribeNotifications is
fully active; update subscribeNotifications to provide a readiness signal (e.g.,
return or include a Promise that resolves once the background subscription/loop
has registered the subscription) and then change this test to await that
readiness before calling senderClient.payInvoice; specifically, ensure
receiverClient.subscribeNotifications resolves or exposes a "ready" Promise that
the test awaits prior to invoking senderClient.payInvoice so the notification
cannot be missed.
In `@e2e/nwc-settle-hold-invoice.test.ts`:
- Around line 50-66: The test spawns payPromise via senderClient.payInvoice and
may leave it unawaited on failure, causing unhandled rejections; ensure
payPromise is always awaited or has a rejection handler: keep creating
payPromise, then after calling receiverClient.settleHoldInvoice (or in the catch
block) await payPromise (or await payPromise.catch(() => {})) so the payment
promise is resolved or its rejection is handled in all paths (referencing
payPromise, senderClient.payInvoice, receiverClient.settleHoldInvoice, and the
existing try/catch/finally flow) to avoid leaked unhandled rejections.
---
Nitpick comments:
In `@e2e/nwc-multi-pay-invoice.test.ts`:
- Around line 57-61: The test is brittle because it asserts
multiPayResult.errors strictly equals an empty array; instead, verify the
contract shape and presence of success: ensure multiPayResult.errors is an array
(use Array.isArray on multiPayResult.errors) and that its length matches
multiPayResult.invoices.length to keep the per-item error array shape, while
keeping the existing checks that multiPayResult.invoices[0].preimage and
multiPayResult.invoices[1].preimage are defined to assert success presence.
In `@e2e/nwc-multi-pay-keysend.test.ts`:
- Around line 51-55: The test currently asserts multiPayResult.errors equals an
empty array which is fragile; instead assert the result shape and per-entry
success: verify multiPayResult.keysends is an array of length 2, verify
multiPayResult.errors is an array (type check) and that every entry in
multiPayResult.keysends has no per-entry error and has a defined preimage (e.g.
check each keysend object for absence/falsey of an error field and that preimage
is defined). Use the existing symbols multiPayResult, keysends, and errors to
locate and update the assertions so they assert type/shape and per-keysend
success rather than strict equality to [].
🪄 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: 88e39087-4e38-4eb9-94cf-15750f2374ac
📒 Files selected for processing (9)
e2e/nwc-cancel-hold-invoice.test.tse2e/nwc-get-balance.test.tse2e/nwc-get-budget.test.tse2e/nwc-get-info.test.tse2e/nwc-make-hold-invoice.test.tse2e/nwc-multi-pay-invoice.test.tse2e/nwc-multi-pay-keysend.test.tse2e/nwc-notifications-payment-received.test.tse2e/nwc-settle-hold-invoice.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/nwc/NWCClient.ts`:
- Around line 716-719: The Promise initialSubscriptionReady (backed by
pendingInitial) can hang indefinitely if the resolve path in
pendingInitial.resolve is never reached (e.g., repeated failures in
_checkConnected or _selectEncryptionType); change the logic in the notification
subscription setup so the promise is settled on both success and failure:
implement a timeout fallback and a reject path that calls pendingInitial.resolve
or rejects the promise after a configurable timeout, and ensure any
caller-facing return (the unsubscribe function) is returned even when the
initial readiness times out or when _checkConnected/_selectEncryptionType fail;
update references to initialSubscriptionReady, pendingInitial, and the code
paths in _checkConnected/_selectEncryptionType to wire the timeout/reject and to
clean up resources so callers are not left awaiting forever.
🪄 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: 9d11214c-10ab-4b7b-94d1-754cc9356625
📒 Files selected for processing (4)
e2e/nwc-get-balance.test.tse2e/nwc-notifications-payment-received.test.tse2e/nwc-settle-hold-invoice.test.tssrc/nwc/NWCClient.ts
✅ Files skipped from review due to trivial changes (1)
- e2e/nwc-settle-hold-invoice.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/nwc-get-balance.test.ts
- e2e/nwc-notifications-payment-received.test.ts
|
@rolznz hello! I hope this PR is still relevant 🙏 Could you please take a look at it? I've corrected coderabbitai's comments in this PR and am currently continuing to work on testing coverage for this repository. |
c453c58 to
3ccb8b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/nwc-list-transactions-after-payment.test.ts (1)
75-91: Consider polling before assertingincoming/settledto reduce E2E flakes.
payInvoicecan complete before the receiver’s transaction view is fully indexed. A short retry loop aroundlistTransactionswould make this test more stable in CI/network jitter scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nwc-list-transactions-after-payment.test.ts` around lines 75 - 91, The test currently calls senderClient.payInvoice then immediately calls receiverClient.listTransactions and asserts matchingTx is incoming/settled, causing flakes; wrap the list+assert logic in a short retry/poll loop that re-calls receiverClient.listTransactions (with same params) until a matchingTx with type === "incoming" and state === "settled" and amount === AMOUNT_MSATS is found or a timeout (e.g., few seconds / fixed attempts) is reached; ensure the loop sleeps briefly between attempts and fail the test only after timeout so matchingTx and its fields are validated reliably.e2e/nwc-lookup-invoice.test.ts (1)
61-67: Nice contract coverage; consider a small retry aroundlookupInvoicefor stability.In some relay/backend conditions, immediate post-payment lookup can lag briefly. Retrying lookup for a short window would harden this E2E test against transient timing issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nwc-lookup-invoice.test.ts` around lines 61 - 67, The lookupInvoice call can race with backend propagation; wrap the receiverClient.lookupInvoice({ payment_hash: invoiceResult.payment_hash }) call in a short retry loop that polls until the invoice is returned or a timeout is reached (e.g., max 5 attempts with ~200ms delay or a 2s total timeout). Replace the single await byHash = await receiverClient.lookupInvoice(...) with logic that retries on null/missing invoice or exceptions and then assert byHash.payment_hash and byHash.invoice once a non-empty result is received; reference invoiceResult, senderClient.payInvoice, receiverClient.lookupInvoice, and byHash when locating where to change the test. Ensure the retry waits use await (e.g., Promise+setTimeout) so the test remains async.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/nwc-multi-pay-invoice.test.ts`:
- Around line 31-37: The test uses a deliberately malformed invoice string
("lnbc1invalidinvoice") which can trigger input validation before the capability
check; replace that with a syntactically valid test invoice (use an existing
fixture like VALID_INVOICE or a helper such as createTestInvoice()) when calling
senderClient.multiPayInvoice({ invoices: [{ invoice: VALID_INVOICE }] }) so the
call reaches the capability-path and you can assert rejects.toMatchObject({
code: "NOT_IMPLEMENTED" }); alternatively, if no valid invoice fixture exists,
pass an empty invoices array (invoices: []) to trigger the capability fallback
without format validation; update the call sites referencing senderInfo.methods
and senderClient.multiPayInvoice accordingly.
---
Nitpick comments:
In `@e2e/nwc-list-transactions-after-payment.test.ts`:
- Around line 75-91: The test currently calls senderClient.payInvoice then
immediately calls receiverClient.listTransactions and asserts matchingTx is
incoming/settled, causing flakes; wrap the list+assert logic in a short
retry/poll loop that re-calls receiverClient.listTransactions (with same params)
until a matchingTx with type === "incoming" and state === "settled" and amount
=== AMOUNT_MSATS is found or a timeout (e.g., few seconds / fixed attempts) is
reached; ensure the loop sleeps briefly between attempts and fail the test only
after timeout so matchingTx and its fields are validated reliably.
In `@e2e/nwc-lookup-invoice.test.ts`:
- Around line 61-67: The lookupInvoice call can race with backend propagation;
wrap the receiverClient.lookupInvoice({ payment_hash: invoiceResult.payment_hash
}) call in a short retry loop that polls until the invoice is returned or a
timeout is reached (e.g., max 5 attempts with ~200ms delay or a 2s total
timeout). Replace the single await byHash = await
receiverClient.lookupInvoice(...) with logic that retries on null/missing
invoice or exceptions and then assert byHash.payment_hash and byHash.invoice
once a non-empty result is received; reference invoiceResult,
senderClient.payInvoice, receiverClient.lookupInvoice, and byHash when locating
where to change the test. Ensure the retry waits use await (e.g.,
Promise+setTimeout) so the test remains async.
🪄 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: 6bbba78d-6dfe-4861-87d4-77effa307ced
📒 Files selected for processing (14)
e2e/nwc-cancel-hold-invoice.test.tse2e/nwc-faucet.test.tse2e/nwc-get-balance.test.tse2e/nwc-get-budget.test.tse2e/nwc-get-info.test.tse2e/nwc-list-transactions-after-payment.test.tse2e/nwc-lookup-invoice.test.tse2e/nwc-make-hold-invoice.test.tse2e/nwc-multi-pay-invoice.test.tse2e/nwc-multi-pay-keysend.test.tse2e/nwc-notifications-payment-received.test.tse2e/nwc-settle-hold-invoice.test.tse2e/nwc-sign-message.test.tssrc/nwc/NWCClient.ts
✅ Files skipped from review due to trivial changes (3)
- e2e/nwc-get-balance.test.ts
- e2e/nwc-get-budget.test.ts
- e2e/nwc-notifications-payment-received.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e/nwc-get-info.test.ts
- src/nwc/NWCClient.ts
- e2e/nwc-multi-pay-keysend.test.ts
- e2e/nwc-settle-hold-invoice.test.ts
- e2e/nwc-make-hold-invoice.test.ts
- e2e/nwc-cancel-hold-invoice.test.ts
|
Hi @DSanich , sorry I somehow missed this, thanks for adding extra tests! I would prefer if possible for the changes to the NWCClient to be moved to a new PR as I think we need to review those separately and am not sure that's the best place to do it. |
|
Hello @rolznz! Yes sure |
3ccb8b2 to
f127441
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
e2e/nwc-multi-pay-invoice.test.ts (1)
31-37:⚠️ Potential issue | 🟡 MinorUse a valid invoice for the unsupported-capability probe.
Line 34 uses a malformed invoice string, so rejection can come from input validation instead of capability handling. This makes the
NOT_IMPLEMENTEDassertion flaky.Proposed fix
if (!senderInfo.methods.includes("multi_pay_invoice")) { + const probeInvoice = await receiverClient.makeInvoice({ + amount: AMOUNT_MSATS, + description: "E2E multi_pay_invoice capability probe", + }); await expect( senderClient.multiPayInvoice({ - invoices: [{ invoice: "lnbc1invalidinvoice" }], + invoices: [{ invoice: probeInvoice.invoice }], }), ).rejects.toMatchObject({ code: "NOT_IMPLEMENTED" }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nwc-multi-pay-invoice.test.ts` around lines 31 - 37, The test uses a malformed invoice ("lnbc1invalidinvoice") when calling senderClient.multiPayInvoice, which can trigger input validation instead of the capability check; update the probe to supply a syntactically valid Lightning invoice (e.g., generate or use a known-valid dummy/invoice string) in the invoices array so the call reaches capability handling; ensure the call remains under the branch checking senderInfo.methods and that the assertion still expects rejects.toMatchObject({ code: "NOT_IMPLEMENTED" }) from senderClient.multiPayInvoice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/nwc-cancel-hold-invoice.test.ts`:
- Line 56: The test currently only asserts the invoice format; extend it to also
assert that the returned hold invoice echoes the requested payment hash by
adding an expectation comparing holdInvoiceResult.payment_hash (or the property
name used on the response) to the original requestedPaymentHash (or the variable
used when creating the hold invoice) — locate the assertion after
expect(holdInvoiceResult.invoice).toMatch(/^ln/) and add
expect(holdInvoiceResult.payment_hash).toBe(requestedPaymentHash) (adjust
property/variable names to match the test).
- Around line 44-47: The test is over-constraining the error from
receiverClient.cancelHoldInvoice by asserting only code "NOT_IMPLEMENTED";
update the assertion around cancelHoldInvoice({ payment_hash: paymentHash })
(the call guarded by hasHoldMethods) to accept both "NOT_IMPLEMENTED" and
"RESTRICTED" (or otherwise a regex/collection that matches either code) so
wallets that signal unavailable-by-scope with "RESTRICTED" do not fail the test.
- Around line 65-69: Replace the fixed 1500ms sleep with a bounded
retry/condition wait that polls the invoice state until it is in the hold state
(or a timeout is reached) before calling receiverClient.cancelHoldInvoice;
specifically, remove the setTimeout and implement a small helper loop that
repeatedly calls the invoice lookup method (e.g., receiverClient.getInvoice or
lookupInvoice with payment_hash/paymentHash) at short intervals (e.g.,
100-300ms) for a maximum duration (e.g., 5-10s), proceed to call
receiverClient.cancelHoldInvoice({ payment_hash: paymentHash }) only when the
invoice status indicates it is held, and throw/assert a clear timeout error if
the condition is not met within the bound.
---
Duplicate comments:
In `@e2e/nwc-multi-pay-invoice.test.ts`:
- Around line 31-37: The test uses a malformed invoice ("lnbc1invalidinvoice")
when calling senderClient.multiPayInvoice, which can trigger input validation
instead of the capability check; update the probe to supply a syntactically
valid Lightning invoice (e.g., generate or use a known-valid dummy/invoice
string) in the invoices array so the call reaches capability handling; ensure
the call remains under the branch checking senderInfo.methods and that the
assertion still expects rejects.toMatchObject({ code: "NOT_IMPLEMENTED" }) from
senderClient.multiPayInvoice.
🪄 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: 317b11c4-0bf4-48f7-9c46-62f09a911622
📒 Files selected for processing (12)
e2e/nwc-cancel-hold-invoice.test.tse2e/nwc-faucet.test.tse2e/nwc-get-balance.test.tse2e/nwc-get-budget.test.tse2e/nwc-get-info.test.tse2e/nwc-list-transactions-after-payment.test.tse2e/nwc-lookup-invoice.test.tse2e/nwc-multi-pay-invoice.test.tse2e/nwc-multi-pay-keysend.test.tse2e/nwc-notifications-payment-received.test.tse2e/nwc-settle-hold-invoice.test.tse2e/nwc-sign-message.test.ts
✅ Files skipped from review due to trivial changes (2)
- e2e/nwc-lookup-invoice.test.ts
- e2e/nwc-get-balance.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- e2e/nwc-faucet.test.ts
- e2e/nwc-get-budget.test.ts
- e2e/nwc-get-info.test.ts
- e2e/nwc-list-transactions-after-payment.test.ts
- e2e/nwc-sign-message.test.ts
- e2e/nwc-multi-pay-keysend.test.ts
- e2e/nwc-notifications-payment-received.test.ts
- e2e/nwc-settle-hold-invoice.test.ts
|
@rolznz I've implemented your suggested changes. I'd be happy to receive a second review 🙏 |
Summary
get_info,get_budget, faucetget_balance(approximate balance vs exact faucet amount),lookup_invoice(by invoice string and bypayment_hashonly),list_transactions(outgoing after pay and incoming on the receiver), hold invoices (make / settle / cancel with two-wallet flows and realistic timing),multi_pay_invoice/multi_pay_keysend, andpayment_receivednotifications (subscribe before pay so the relay subscription is active).sign_messagee2e: success when allowed, orNOT_IMPLEMENTED/RESTRICTEDwhen the connection lacks scope—without assuming a single error code for “unsupported”.pay_invoicerejection / promise lifecycle after cancel).Tests
NOT_IMPLEMENTED,RESTRICTED, payer errors after hold cancel, etc.).SDK / client changes
subscribeNotifications(wait until the initial relay subscription is ready) andgetWalletServiceInfo(retries when kind 13194 is missing transiently) live in a separate PR: branchfix/nwc-client-relay-subscription. For green CI on notification- and timing-sensitive flows, merge that client PR first (or temporarily base this branch on it), then rebase ontomasterafter it lands.