feat: add NWC-321 pay and receive methods with BOLT-11 support - #2543
Open
rolznz wants to merge 1 commit into
Open
feat: add NWC-321 pay and receive methods with BOLT-11 support#2543rolznz wants to merge 1 commit into
rolznz wants to merge 1 commit into
Conversation
Implements the NWC-321 (BIP-321 Lightning Payments) pay and receive methods, limited to BOLT-11 instructions: - pay parses the BIP-321 URI, selects the lightning (BOLT-11) instruction and rejects URIs without one (UNSUPPORTED_PAYMENT_INSTRUCTION), validates the invoice network against the node network (UNSUPPORTED_NETWORK), rejects conflicting or invalid amounts, unknown req- parameters and payer_note (undeliverable over BOLT-11) - receive returns a BIP-321 URI containing a single BOLT-11 invoice; a variable amount is rejected as zero-amount invoices are not supported - both methods reuse the existing pay_invoice / make_invoice scopes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdded NIP-47 ChangesNWC-321 commands
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NIP47Client
participant EventHandler
participant Controller
participant LNService
NIP47Client->>EventHandler: pay or receive request
EventHandler->>Controller: dispatch NIP-47 method
Controller->>LNService: submit payment or create invoice
LNService-->>Controller: transaction result
Controller-->>NIP47Client: BIP-321 response or mapped error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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}
Contributor
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 `@nip47/controllers/pay_controller.go`:
- Around line 298-307: Update the amount-parsing logic around the whole and
fractional ParseUint calls to wrap each returned error with fmt.Errorf using
descriptive context identifying the failing BTC amount component and the
original error via %w; preserve the existing zero-value returns and validation
flow.
- Line 163: Guard transaction.Preimage in the existing-settlement response
construction before dereferencing it. Handle a nil preimage safely while
preserving the current Preimage assignment for non-nil values, preventing the
handler from panicking on NULL database rows.
In `@nip47/controllers/receive_controller.go`:
- Around line 34-44: Update the receive controller’s amount validation before
MakeInvoice to reject both nil and zero amounts with ERROR_BAD_REQUEST; in
nip47/controllers/receive_controller.go lines 34-44, extend the existing check
while preserving its response and return behavior. Add a regression case in
nip47/controllers/receive_controller_test.go lines 83-102 that submits amount 0
and asserts ERROR_BAD_REQUEST.
In `@nip47/event_handler.go`:
- Around line 441-446: Redact pay and receive request parameters in
nip47/event_handler.go:441-446 before the existing full-params log, then remove
sensitive payment data from each affected log: omit payment at
nip47/controllers/pay_controller.go:57-63, bolt11 at :90-94 and :127-139, and
request description and amount at nip47/controllers/receive_controller.go:46-60
while retaining request/app identifiers; use WithError(err).Error(...) for
receive invoice failures.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da6bcfc1-cec8-4623-8317-adde61efa3f9
📒 Files selected for processing (15)
constants/constants.golnclient/bark/bark.golnclient/cashu/cashu.golnclient/cln/cln.golnclient/ldk/ldk.golnclient/lnd/lnd.golnclient/phoenixd/phoenixd.gonip47/controllers/pay_controller.gonip47/controllers/pay_controller_test.gonip47/controllers/receive_controller.gonip47/controllers/receive_controller_test.gonip47/event_handler.gonip47/models/models.gonip47/permissions/permissions.gotests/mock_ln_client.go
| Amount: transaction.AmountMsat, | ||
| FeesPaid: transaction.FeeMsat, | ||
| PaymentHash: transaction.PaymentHash, | ||
| Preimage: *transaction.Preimage, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map and inspect the transaction type and every SendPaymentSync implementation.
while IFS= read -r file; do
if rg -q 'SendPaymentSync|type\s+\w*Transaction\b|Preimage' "$file"; then
ast-grep outline "$file" --items all
rg -n -C 5 'SendPaymentSync|type\s+\w*Transaction\b|Preimage' "$file"
fi
done < <(fd -t f -e go)Repository: getAlby/hub
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -t f -e go | sort | sed -n '1,240p'
printf '%s\n' '--- relevant symbols ---'
rg -n -C 6 'SendPaymentSync|type[[:space:]]+[A-Za-z0-9_]*Transaction|Preimage' -g '*.go' .Repository: getAlby/hub
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- SendPaymentSync definitions and calls ---'
rg -n -C 12 'func[[:space:]]+\([^)]*\)[[:space:]]*SendPaymentSync|func[[:space:]]+SendPaymentSync|SendPaymentSync[[:space:]]*\(' -g '*.go' . \
| grep -v '_test.go' | sed -n '1,420p'
printf '%s\n' '--- transaction model declarations ---'
rg -n -C 20 'type[[:space:]]+Transaction[[:space:]]+struct|Preimage[[:space:]]+\*string|Preimage[[:space:]]+\*' -g '*.go' . \
| grep -v '_test.go' | sed -n '1,260p'
printf '%s\n' '--- pay handler around the dereference ---'
sed -n '125,180p' nip47/controllers/pay_controller.goRepository: getAlby/hub
Length of output: 38221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transaction service files ---'
fd -t f transactions | sort
printf '%s\n' '--- exact service implementation ---'
rg -n -C 18 'SendPaymentSync' transactions -g '*.go' | sed -n '1,520p'
printf '%s\n' '--- PayInvoiceResponse declaration and uses ---'
rg -n -C 16 'type[[:space:]]+PayInvoiceResponse|PayInvoiceResponse[[:space:]]*\{' -g '*.go' . \
| grep -v 'tests/mocks' | sed -n '1,520p'
printf '%s\n' '--- successful response return statements ---'
rg -n -C 12 'return[[:space:]]+&lnclient\.PayInvoiceResponse|return[[:space:]]+&PayInvoiceResponse|PayInvoiceResponse\{' -g '*.go' lnclient tests | grep -v 'tests/mocks' | sed -n '1,520p'Repository: getAlby/hub
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- SendPaymentSync complete control flow ---'
sed -n '301,475p' transactions/transactions_service.go
printf '%s\n' '--- settlement implementation ---'
rg -n -C 24 'func[[:space:]]+\([^)]*\)[[:space:]]*markTransactionSettled|func[[:space:]]+markTransactionSettled|markTransactionSettled[[:space:]]*\(' transactions -g '*.go' | sed -n '1,360p'
printf '%s\n' '--- self-payment implementation ---'
rg -n -C 24 'interceptSelfPayment' transactions -g '*.go' | sed -n '1,300p'
printf '%s\n' '--- all assignments to db.Transaction.Preimage ---'
rg -n -C 8 '\.Preimage[[:space:]]*=|Preimage:[[:space:]]*&|Preimage:[[:space:]]*[A-Za-z_][A-Za-z0-9_]*' transactions -g '*.go' | grep -v '_test.go' | sed -n '1,360p'Repository: getAlby/hub
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- markTransactionSettled definition ---'
rg -n -C 35 'markTransactionSettled' transactions/transactions_service.go | sed -n '1,320p'
printf '%s\n' '--- transaction conversion after settlement ---'
rg -n -C 24 'func[[:space:]]+.*Transaction|Preimage[[:space:]]*:' transactions/transactions_service.go | sed -n '1,360p'
printf '%s\n' '--- direct behavioral shape check ---'
python3 - <<'PY'
from pathlib import Path
p = Path("transactions/transactions_service.go")
s = p.read_text()
needle = "markTransactionSettled"
for i, line in enumerate(s.splitlines(), 1):
if needle in line:
print(f"{i}: {line}")
PYRepository: getAlby/hub
Length of output: 26512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1445,1535p' transactions/transactions_service.goRepository: getAlby/hub
Length of output: 3323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 35 'func[[:space:]]+\([^)]*\)[[:space:]]*findSettledTransaction|findSettledTransaction[[:space:]]*\(' transactions/transactions_service.goRepository: getAlby/hub
Length of output: 8028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path("transactions/transactions_service.go").read_text()
start = source.index("func (svc *transactionsService) markTransactionSettled")
end = source.index("\n}\n\n// createSettledTransactionFromNotification", start) + 2
body = source[start:end]
checks = {
"empty preimage rejected": re.search(r'if preimage == "" \{\s*return nil, errors\.New\("no preimage in payment"\)', body, re.S),
"new settlement writes pointer": re.search(r'"Preimage":\s*&preimage', body),
"existing settlement returned unchanged": re.search(
r'if existingSettledTransaction != nil \{.*?settledTransaction = existingSettledTransaction',
body, re.S
),
}
for name, matched in checks.items():
print(f"{name}: {'yes' if matched else 'no'}")
if not all(checks.values()):
raise SystemExit(1)
PYRepository: getAlby/hub
Length of output: 257
Guard the nullable transaction.Preimage before dereferencing it. The existing-settlement path can return a database row with a NULL preimage, which can panic this handler.
🤖 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 `@nip47/controllers/pay_controller.go` at line 163, Guard transaction.Preimage
in the existing-settlement response construction before dereferencing it. Handle
a nil preimage safely while preserving the current Preimage assignment for
non-nil values, preventing the handler from panicking on NULL database rows.
Comment on lines
+298
to
+307
| whole, err := strconv.ParseUint(intPart, 10, 64) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| if whole > 21_000_000 { | ||
| return 0, fmt.Errorf("amount too large") | ||
| } | ||
| frac, err := strconv.ParseUint(fracPart, 10, 64) | ||
| if err != nil { | ||
| return 0, err |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap ParseUint errors with parse context.
Lines 300 and 307 return parser errors without identifying the failing BTC amount component. Wrap both errors with %w.
Proposed fix
if err != nil {
- return 0, err
+ return 0, fmt.Errorf("parse whole BTC amount: %w", err)
}
...
if err != nil {
- return 0, err
+ return 0, fmt.Errorf("parse fractional BTC amount: %w", err)
}As per coding guidelines, **/*.go requires errors to use fmt.Errorf("context: %w", err) for debugging.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| whole, err := strconv.ParseUint(intPart, 10, 64) | |
| if err != nil { | |
| return 0, err | |
| } | |
| if whole > 21_000_000 { | |
| return 0, fmt.Errorf("amount too large") | |
| } | |
| frac, err := strconv.ParseUint(fracPart, 10, 64) | |
| if err != nil { | |
| return 0, err | |
| whole, err := strconv.ParseUint(intPart, 10, 64) | |
| if err != nil { | |
| return 0, fmt.Errorf("parse whole BTC amount: %w", err) | |
| } | |
| if whole > 21_000_000 { | |
| return 0, fmt.Errorf("amount too large") | |
| } | |
| frac, err := strconv.ParseUint(fracPart, 10, 64) | |
| if err != nil { | |
| return 0, fmt.Errorf("parse fractional BTC amount: %w", err) |
🤖 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 `@nip47/controllers/pay_controller.go` around lines 298 - 307, Update the
amount-parsing logic around the whole and fractional ParseUint calls to wrap
each returned error with fmt.Errorf using descriptive context identifying the
failing BTC amount component and the original error via %w; preserve the
existing zero-value returns and validation flow.
Source: Coding guidelines
Comment on lines
+34
to
+44
| if receiveParams.Amount == nil { | ||
| // variable-amount (zero-amount) invoices are not supported | ||
| publishResponse(&models.Response{ | ||
| ResultType: nip47Request.Method, | ||
| Error: &models.Error{ | ||
| Code: constants.ERROR_BAD_REQUEST, | ||
| Message: "amount is required", | ||
| }, | ||
| }, nostr.Tags{}) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject zero receive amounts.
A request with "amount": 0 passes the current check and reaches MakeInvoice. This creates a variable-amount invoice, but this PR supports fixed-amount BOLT-11 receive requests only.
nip47/controllers/receive_controller.go#L34-L44: Reject both a nil amount and an amount of zero before callingMakeInvoice.nip47/controllers/receive_controller_test.go#L83-L102: Add a regression test for"amount": 0and assertERROR_BAD_REQUEST.
📍 Affects 2 files
nip47/controllers/receive_controller.go#L34-L44(this comment)nip47/controllers/receive_controller_test.go#L83-L102
🤖 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 `@nip47/controllers/receive_controller.go` around lines 34 - 44, Update the
receive controller’s amount validation before MakeInvoice to reject both nil and
zero amounts with ERROR_BAD_REQUEST; in nip47/controllers/receive_controller.go
lines 34-44, extend the existing check while preserving its response and return
behavior. Add a regression case in nip47/controllers/receive_controller_test.go
lines 83-102 that submits amount 0 and asserts ERROR_BAD_REQUEST.
Comment on lines
+441
to
+446
| case models.PAY_METHOD: | ||
| controller. | ||
| HandlePayEvent(ctx, nip47Request, requestEvent.ID, &app, publishResponse) | ||
| case models.RECEIVE_METHOD: | ||
| controller. | ||
| HandleReceiveEvent(ctx, nip47Request, requestEvent.ID, app.ID, publishResponse) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log payment request data.
The new flows log full request parameters, BIP-321 URIs, BOLT-11 invoices, descriptions, and amounts. These values can expose payment and user information in application logs.
nip47/event_handler.go#L441-L446: Redact pay and receive request parameters before the existing full-params log executes.nip47/controllers/pay_controller.go#L57-L63: Removepaymentfrom the error log fields.nip47/controllers/pay_controller.go#L90-L94: Removebolt11from the decode-failure log fields.nip47/controllers/pay_controller.go#L127-L139: Removebolt11from payment lifecycle log fields.nip47/controllers/receive_controller.go#L46-L60: Remove request description and amount from log fields. Retain request and app identifiers. UseWithError(err).Error(...)for invoice failures.
As per coding guidelines, **/*.{go,ts,tsx} says, “Never log sensitive data such as seeds, macaroons, or tokens.”
📍 Affects 3 files
nip47/event_handler.go#L441-L446(this comment)nip47/controllers/pay_controller.go#L57-L63nip47/controllers/pay_controller.go#L90-L94nip47/controllers/pay_controller.go#L127-L139nip47/controllers/receive_controller.go#L46-L60
🤖 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 `@nip47/event_handler.go` around lines 441 - 446, Redact pay and receive
request parameters in nip47/event_handler.go:441-446 before the existing
full-params log, then remove sensitive payment data from each affected log: omit
payment at nip47/controllers/pay_controller.go:57-63, bolt11 at :90-94 and
:127-139, and request description and amount at
nip47/controllers/receive_controller.go:46-60 while retaining request/app
identifiers; use WithError(err).Error(...) for receive invoice failures.
Source: Coding guidelines
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.
Closes #2504
Implements the NWC-321 (BIP-321 Lightning Payments)
payandreceivemethods with the minimum scope: BOLT-11 instructions only.JS SDK implementation: getAlby/js-sdk#576
pay
paymentparam as a BIP-321 URI (case-insensitive scheme and parameter keys, so fully-uppercased QR URIs work) and selects thelightning(BOLT-11) instruction.UNSUPPORTED_PAYMENT_INSTRUCTIONif the URI has nolightningparam (e.g. onlylno=).UNSUPPORTED_NETWORKon mismatch.amountor BIP-321amountparameter that contradicts the invoice amount, or a zero amount. For zero-amount invoices, the requestamountor the URIamountfunds the payment.payer_notewithBAD_REQUEST— BOLT-11 cannot deliver payer-provided messages, and NWC-321 requires the note to be delivered or the request rejected before payment.req-prefixed parameters (includingreq-pop) per BIP-321; the optionalpopparameter is ignored, as BIP-321 permits for optional parameters.transaction_id,state,instruction_type: "bolt11",amount,fees_paid,payment_hash,preimage,created_at,settled_at.receive
bip321: "bitcoin:?lightning=<invoice>"plustransaction_id.amount(spec's variable-amount case) returnsBAD_REQUEST, since zero-amount invoices are not supported by the transactions service.Wiring
payreuses thepay_invoicescope andreceivereusesmake_invoice— no new scopes and no frontend changes; the existing 6-hour stale-request guard applies automatically.modelsconstants.payer_noterejection, unknownreq-parameter rejection, and missing receive amount.tests.MockNodeInfo.Networkchanged fromtestnettosignetto match the signet (lntbs) mock invoices now that the network is validated.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
payandreceivemethods across supported Lightning clients.Permissions
Testing