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

feat: add support for custom expiry #93

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

louneskmt
Copy link
Contributor

A quick PR adding support for custom invoice expiry delta in seconds. By default, it was set to 3600 (one hour). It is now possible to add an optional parameter to change this value.

The API and internal functions are the only scopes of this PR, a follow-up PR would be needed to add CLI and Web support.

@johncantrell97 Do I need to add an expiry column to the Payment Model?

@@ -292,6 +292,7 @@ message KeysendResponse {}
message CreateInvoiceRequest {
uint64 amt_msat = 1;
string description = 2;
optional uint32 expiry = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if we do this other places with time but I personally prefer to know the units similar to amt_msat. When I look at expiry the first thing I'll ask is if it's in milliseconds or seconds. how do you feel about naming it expiry_secs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great to me 👌

@johncantrell97
Copy link
Contributor

Hm, I guess we could add it explicitly to the payments table. Technically it's encoded in the invoice which is already included in the table, right? I guess some of the other fields are probably also already encoded in the invoice and added seperately as well.

Let's add it. Don't bother with a new migration though, just add it to the create_payments_table one and then you'll need to pass it through when we create an invoice.

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.

2 participants