Skip to content
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

Merged
merged 3 commits into from
May 31, 2021

Conversation

cfromknecht
Copy link
Contributor

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.

Copy link
Member

@Roasbeef Roasbeef left a 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.

lnrpc/invoicesrpc/addinvoice.go Outdated Show resolved Hide resolved
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@halseth halseth left a 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 ✅

lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved
}

func testSendPaymentAMPInvoiceCase(net *lntest.NetworkHarness, t *harnessTest,
useExternalPayAddr bool) {
Copy link
Contributor

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.
@Roasbeef Roasbeef added this to the 0.13.0 milestone May 31, 2021
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🏓

@Roasbeef Roasbeef merged commit 7e91a02 into lightningnetwork:master May 31, 2021
@cfromknecht cfromknecht deleted the reuse-amp-invoice branch June 1, 2021 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants