lightning: add lnurl pay send flow#4214
Conversation
|
@CodeRabbit review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughChangesThis PR extends the Lightning payment flow to support LNURL-Pay inputs alongside existing BOLT11 invoices. On the backend, Sequence Diagram(s)See diagrams embedded within the review stack artifact above. Related PRs: None identified. Suggested labels: backend, frontend, lightning, enhancement Suggested reviewers: Not enough information available to determine reviewers. 🐇 A payment hops through LNURL's gate, ✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/lightning/payments.go (1)
471-509: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the BOLT11 send path with the balance precheck
sendBolt11Paymentstill skips the sameBalance()/checkPaymentBalance()guard used byprepareBolt11PaymentandsendLNURLPay. If funds change after quoting, BOLT11 will only fail insideSendPayment; add the balance check here for parity.🤖 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 `@backend/lightning/payments.go` around lines 471 - 509, The BOLT11 send flow in sendBolt11Payment is missing the same Balance()/checkPaymentBalance() precheck used by prepareBolt11Payment and sendLNURLPay, so add that guard after CheckActive and before PrepareSendPayment to keep payment validation consistent. Use the Lightning methods Balance, checkPaymentBalance, and sendBolt11Payment to locate the change, and make sure the balance is verified against request.AmountSat/ApprovedFeeSat before proceeding to the SDK send path.frontends/web/src/routes/lightning/send/components/payment-input-details.tsx (1)
122-157: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the confirm heading to the LNURL-Pay review step.
send.tsxalready renders the page title (lightning.send.title), butLNURLPayReviewStepstill skips the confirmation heading thatBolt11PaymentDetailsshows, so LNURL-Pay reviews read like an unfinished state.🤖 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 `@frontends/web/src/routes/lightning/send/components/payment-input-details.tsx` around lines 122 - 157, The LNURL-Pay review UI is missing the confirmation heading shown in the Bolt11 flow, so add the same confirm title to LNURLPayRecipientDetails or LNURLPayReviewStep using the existing translation key lightning.send.confirm.title. Make the change in the payment-input-details component so LNURL-Pay review matches Bolt11PaymentDetails and the send.tsx page title remains separate from the confirmation heading.
🤖 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 `@frontends/web/src/routes/lightning/send/components/bolt11-review-step.tsx`:
- Around line 38-247: The payment review logic in bolt11-review-step.tsx is
duplicated with LNURLPayReviewStep, including preparePayment, sendPayment, the
reset/debounce effects, and TSdkError handling. Extract this shared behavior
into a reusable hook or helper (for example, a payment review hook parameterized
by payment type and prepare/send functions) and have Bolt11PaymentReviewStep
consume it. Keep the invoice-specific UI and inputs in Bolt11PaymentReviewStep,
but move the common state/effect/error-branching flow into the shared
implementation to prevent further drift.
- Around line 112-130: The catch block in bolt11-review-step.tsx only guards
setIsSending(false) with mounted.current, but backToPaymentInput, setSendError,
and the preparePayment retry can still run after unmount. Add the same early
unmounted return pattern used in LNURLPayReviewStep before any side effects in
this catch path, and keep the TSdkError branches and errorMessage handling
inside the mounted-safe flow.
In
`@frontends/web/src/routes/lightning/send/components/lnurl-pay-review-step.tsx`:
- Around line 36-41: The LNURL Pay amount state in lnurl-pay-review-step.tsx
always initializes customAmount as undefined, which misses the common
fixed-amount case. Update the component around the customAmount/useDebounce
state setup to prefill customAmount when minAmountSat equals maxAmountSat (the
fixed LNURL pay amount case), so the user doesn’t need to enter the only valid
value manually. Keep the behavior tied to the existing prepared-payment flow and
ensure any amount-setting logic in this component continues to use customAmount
consistently.
In `@frontends/web/src/routes/lightning/send/components/payment-review.ts`:
- Around line 6-11: TPreparedPayment currently mixes optional result fields with
an explicit isPreparing boolean, which conflicts with the loading-state
convention. Update the type in payment-review.ts to use a discriminated union
with mutually exclusive preparing/ready/error states instead of a separate
boolean, and adjust the shape around TPreparedPayment so callers can rely on
status-based narrowing. Keep the change centered on TPreparedPayment and the
related payment review state model so the two consumers can be updated
consistently later.
---
Outside diff comments:
In `@backend/lightning/payments.go`:
- Around line 471-509: The BOLT11 send flow in sendBolt11Payment is missing the
same Balance()/checkPaymentBalance() precheck used by prepareBolt11Payment and
sendLNURLPay, so add that guard after CheckActive and before PrepareSendPayment
to keep payment validation consistent. Use the Lightning methods Balance,
checkPaymentBalance, and sendBolt11Payment to locate the change, and make sure
the balance is verified against request.AmountSat/ApprovedFeeSat before
proceeding to the SDK send path.
In
`@frontends/web/src/routes/lightning/send/components/payment-input-details.tsx`:
- Around line 122-157: The LNURL-Pay review UI is missing the confirmation
heading shown in the Bolt11 flow, so add the same confirm title to
LNURLPayRecipientDetails or LNURLPayReviewStep using the existing translation
key lightning.send.confirm.title. Make the change in the payment-input-details
component so LNURL-Pay review matches Bolt11PaymentDetails and the send.tsx page
title remains separate from the confirmation heading.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 78385121-c289-432e-b555-aee107e31b0c
📒 Files selected for processing (12)
backend/lightning/handlers.gobackend/lightning/payments.gobackend/lightning/payments_test.gofrontends/web/src/api/lightning.tsfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/lightning/send/components/bolt11-review-step.tsxfrontends/web/src/routes/lightning/send/components/lnurl-pay-review-step.tsxfrontends/web/src/routes/lightning/send/components/payment-input-details.tsxfrontends/web/src/routes/lightning/send/components/payment-review.tsfrontends/web/src/routes/lightning/send/components/review-step.tsxfrontends/web/src/routes/lightning/send/components/select-payment-input-step.tsxfrontends/web/src/routes/lightning/send/send.tsx
bznein
left a comment
bznein
left a comment
There was a problem hiding this comment.
I did a first quick pass on the backend, but I'll take some time to review more thoroughly
| func msatToCeilSat(msat uint64) uint64 { | ||
| if msat%1000 == 0 { | ||
| return msat / 1000 | ||
| } | ||
| return msat/1000 + 1 | ||
| } |
There was a problem hiding this comment.
[nit] not even sure if it's nicer to read or not, but
func msatToCeilSat(msat uint64) uint64 {
return (msat + 999) / 1000
}would be functionally the same (provided msat + 999 doesn't overflow)
| func msatToCeilSat(msat uint64) uint64 { | ||
| if msat%1000 == 0 { | ||
| return msat / 1000 | ||
| } | ||
| return msat/1000 + 1 | ||
| } | ||
|
|
||
| func msatToFloorSat(msat uint64) uint64 { | ||
| return msat / 1000 | ||
| } |
There was a problem hiding this comment.
overall, I think these two methods could be combined into a single one with an additional parameter, wdyt?
| for _, entry := range metadata { | ||
| if len(entry) >= 2 && entry[0] == "text/plain" { | ||
| description := entry[1] | ||
| return &description | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
can we add some comments here please? It's not super clear what this does :)
|
@bznein all done 👍 |
|
@thisconnect can you have a look at the frontend? 🙏 |
thisconnect
left a comment
thisconnect
left a comment
There was a problem hiding this comment.
LGTM with some questions and small nit
| const amount = event.currentTarget.valueAsNumber; | ||
| setCustomAmount(Number.isNaN(amount) ? undefined : amount); | ||
| }} | ||
| value={customAmount === undefined ? '' : `${customAmount}`} |
There was a problem hiding this comment.
nit
| value={customAmount === undefined ? '' : `${customAmount}`} | |
| value={customAmount} |
| type TPaymentReviewDetails = TLightningBolt11Invoice | TLightningLNURLPay; | ||
|
|
||
| const isBolt11Invoice = (paymentDetails: TPaymentReviewDetails): paymentDetails is TLightningBolt11Invoice => ( | ||
| 'invoice' in paymentDetails |
There was a problem hiding this comment.
usePaymentReview is used in <Bolt11ReviewStep> and <LNURLPayReviewStep> so we should already know what type it is.
small nit: instead checking for 'invoice' in paymentDetails key, wouldn't it make sense to add type (type: TPaymentInputType.BOLT11 | TPaymentInputType.LNURL_PAY) to TLightningBolt11Invoice and TLightningLNURLPay or similar?
no blocker it's also ok as is.
There was a problem hiding this comment.
Good point, added a type field inside TPaymentReviewDetails to improve it.
| const amountSat = isBolt11Invoice(paymentDetails) && paymentDetails.amountSat !== undefined | ||
| ? undefined | ||
| : currentAmountSat; | ||
| await preparePayment(amountSat); |
There was a problem hiding this comment.
I am reading something like "in case of error: set amount to undefined in case of Bolt11Invoice, but to currentAmountSat in case of lnurl."
but I am not 100% why that is.
Could you add a comment why this is needed?
There was a problem hiding this comment.
Bolt11Invoice can already define an amount: we don't need to pass it to the prepare call in that case. So we use currentAmountSat, which is the user-defined amount, for amountless Bolt11 invoices and LNURLPay (which is amountless by definition). Will add a comment 🙏
There was a problem hiding this comment.
That all makes sense, I'm more wondering why this has to be done in the catch in case of postSendPayment error.
There was a problem hiding this comment.
This is an edge case: it is possible that the payment fee retrieved during the prepare phase is no longer valid when trying to send. In this case, the send call returns a PAYMENT_APPROVAL_REQUIRED error and we need to re-prepare the payment to display the new fee and confirm sending again.
There was a problem hiding this comment.
Makes sense. Maybe small comment about this edge case that the fee may be expired could make this line more clear.
Beerosagos
force-pushed
the
lnurl-pay
branch
2 times, most recently
from
4c77953 to
bdf5891
Compare
|
rebased |
|
@thisconnect PTAL 🙏 |
thisconnect
left a comment
thisconnect
left a comment
There was a problem hiding this comment.
very nice untested LGTM
| const amountSat = isBolt11Invoice(paymentDetails) && paymentDetails.amountSat !== undefined | ||
| ? undefined | ||
| : currentAmountSat; | ||
| await preparePayment(amountSat); |
There was a problem hiding this comment.
Makes sense. Maybe small comment about this edge case that the fee may be expired could make this line more clear.
Add LNURL Pay support to the Lightning send flow while keeping rail selection inside the backend lightning package. Parse payment inputs into explicit BOLT11 and LNURL Pay variants, prepare and send each rail through the shared payment endpoints, and return localized error codes for invalid input and amounts. Refactor the frontend review step into rail-specific BOLT11 and LNURL Pay components. Pass each component only the payload it needs, keep BOLT11 behavior aligned with the existing review flow, and keep LNURL Pay as an amount-entry flow within the server-provided min and max bounds.
3 participants
Add LNURL Pay support to the Lightning send flow while keeping rail selection inside the backend lightning package. Parse payment inputs into explicit BOLT11 and LNURL Pay variants, prepare and send each rail through the shared payment endpoints, and return localized error codes for invalid input and amounts.
Refactor the frontend review step into rail-specific BOLT11 and LNURL Pay components. Pass each component only the payload it needs, keep BOLT11 behavior aligned with the existing review flow, and keep LNURL Pay as an amount-entry flow within the server-provided min and max bounds.
Before asking for reviews, here is a check list of the most common things you might need to consider: