-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lnrpc: allow AMP pay-addr override + bump invoice timeouts #5336
lnrpc: allow AMP pay-addr override + bump invoice timeouts #5336
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Straightforward change that should provide a lot of utility in the short term! Only blocking thing is that it looks like a case statement is flipped during invoice generation.
// Generate an external payment address when attempting to pseudo-reuse | ||
// an AMP invoice. When using an external payment address, we'll also | ||
// expect an extra invoice to appear in the ListInvoices response, since | ||
// a new invoice will be JIT inserted under a different payment address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM barring @Roasbeef's remark ✅
} | ||
|
||
func testSendPaymentAMPInvoiceCase(net *lntest.NetworkHarness, t *harnessTest, | ||
useExternalPayAddr bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Not doing so prevents consecutive subtests from starting properly.
This permits an AMP invoice to be "pseudo-reusable", where the invoice paramters can be used multiple times so long as a new payment address is supplied. This prevents additional round trips between payer and payee to obtain a new invoice, even though the payments/invoices won't be logically associated via the RPC interface like they would when the full reusable invoices are deployed.
Increases the default MPP expiry from 1 hour to 1 day. For the new AMP invoices, we increase the interval to 1 month. The longer time frames for AMP invoices is used so that the invoice can be pseudo reused as implemented in the prior commit. The BOLT 11 default of 1 hour is still preserved whenever the field is missing in the payment request itself, but as of this commit the field will always be set by lnd.
452f694
to
4bb6cc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🏓
This PR permits an AMP invoice to be "pseudo-reusable", where the invoice
paramters can be used multiple times so long as a new payment address is
supplied. This prevents additional round trips between payer and payee
to obtain a new invoice, even though the payments/invoices won't be
logically associated via the RPC interface like they would when the full
reusable invoices are deployed.
It also increases the default MPP expiry from 1 hour to 1 day. For the new AMP
invoices, we increase the interval to 1 month. The longer time frames
for AMP invoices is used so that the invoice can be pseudo reused as
implemented in the prior commit.
The BOLT 11 default of 1 hour is still preserved whenever the field is
missing in the payment request itself, but as of this commit the field
will always be set by lnd.