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

Include expiry in invoices #1411

Closed
jkczyz opened this issue Apr 5, 2022 · 9 comments · Fixed by #1422 or #1474
Closed

Include expiry in invoices #1411

jkczyz opened this issue Apr 5, 2022 · 9 comments · Fixed by #1422 or #1474
Assignees
Milestone

Comments

@jkczyz
Copy link
Contributor

jkczyz commented Apr 5, 2022

When creating an Invoice using functions in the lightning-invoice crate's utils module, the invoice expiration is not set. It should be set with the value encoded in the provided payment secret. Otherwise, BOLT 11 defines the default expiry to be 1 hour whereas ChannelManager will use whatever value is encoded in the payment secret.

This may be tricky because either the methods will need to take in a ChannelManager to verify the payment secret and extract the expiry or the expiry needs to be passed in but could potentially not match what's encoded in the payment secret.

@dunxen
Copy link
Contributor

dunxen commented Apr 5, 2022

I can try pick this up. Would be some alright exposure to lightning-invoice crate :)

@jkczyz
Copy link
Contributor Author

jkczyz commented Apr 5, 2022

Great! Also, note that his only is a problem for create_phantom_invoice and create_phantom_invoice_with_description_hash. The other utility functions take a ChannelManager and use DEFAULT_EXPIRY_TIME.

@dunxen
Copy link
Contributor

dunxen commented Apr 6, 2022

Seems only the second option would make sense in this case as requiring a ChannelManager would defeat the purpose of #1384 😅. However, then we are stuck with the possibility of the expiry not matching what's encoded as you mentioned.

@TheBlueMatt
Copy link
Collaborator

I don't think at need to worry about the expiry not being encoded properly - you can't use phantom nodes without it so if the expiry isn't encoded correctly we should be erroring anyway. Does seem like this needs to be based on #1384.

@jkczyz
Copy link
Contributor Author

jkczyz commented Apr 11, 2022

We may want to consider having the phantom invoice utility functions directly call the exposed creation functions from #1384 instead of exposing functions for decoding. Then the interface would more closely match the non-phantom invoice utility functions.

@TheBlueMatt TheBlueMatt added this to the 0.0.107 milestone Apr 13, 2022
@TheBlueMatt
Copy link
Collaborator

Adding to 107 cause it's an oversight we should fix, but if it slips no big deal.

@dunxen
Copy link
Contributor

dunxen commented Apr 13, 2022

I see #1384 is merged so will get back to work on this. Spread myself a bit thin with the Qala.dev stuff 🥲

Edit: Should have a PR out on Friday. Have been working on it today.

@jkczyz
Copy link
Contributor Author

jkczyz commented May 9, 2022

Re-opening since while #1422 added an invoice_expiry_delta_secs to the utility functions, it didn't set expiry on the Invoice itself. Also, we should allow passing invoice_expiry_delta_secs to the non-phantom utilities for those not wanting the implicit default expiry.

@TheBlueMatt TheBlueMatt reopened this May 9, 2022
@dunxen
Copy link
Contributor

dunxen commented May 10, 2022

Will fix today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants