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.

Sign up for GitHub

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

Jump to bottom

lightning-invoice/utils: Actually add expiry to invoices #1474

Merged
merged 2 commits into from
May 10, 2022
Merged

lightning-invoice/utils: Actually add expiry to invoices #1474

merged 2 commits into from
May 10, 2022
+48 −20

Conversation

dunxen
Copy link
Contributor

@dunxen dunxen commented May 10, 2022

My bad.

I've also added the expiry to non-phantom invoice utilities.

This should now fix #1411

tnull
tnull previously approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Tiny nit, otherwise LGTM.

@dunxen dunxen force-pushed the 2022-05-actually-add-expiry branch from 983a462 to b61a7a2 Compare May 10, 2022 13:21
jkczyz
jkczyz reviewed May 10, 2022
View reviewed changes
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround! We should have caught this in review, so no worries.

@dunxen dunxen force-pushed the 2022-05-actually-add-expiry branch from b61a7a2 to ec774ff Compare May 10, 2022 15:41
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2022 •
edited
Loading

Codecov Report

Merging #1474 (717047c) into main (29727a3) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 717047c differs from pull request most recent head 3369b29. Consider uploading reports for the commit 3369b29 to get more accurate results

@@            Coverage Diff             @@
##             main    #1474      +/-   ##
==========================================
- Coverage   90.94%   90.93%   -0.01%     
==========================================
  Files          75       75              
  Lines       41891    41900       +9     
  Branches    41891    41900       +9     
==========================================
+ Hits        38097    38102       +5     
- Misses       3794     3798       +4
Impacted Files Coverage Δ
lightning-invoice/src/payment.rs 92.75% <100.00%> (ø)
lightning-invoice/src/utils.rs 96.89% <100.00%> (+0.04%) ⬆️
lightning/src/ln/functional_tests.rs 97.11% <0.00%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29727a3...3369b29. Read the comment docs.

@dunxen dunxen force-pushed the 2022-05-actually-add-expiry branch from ec774ff to c962c18 Compare May 10, 2022 17:01
@TheBlueMatt
Copy link
Collaborator

Needs rebase, it appears?

@dunxen
Copy link
Contributor Author

dunxen commented May 10, 2022

Rebasing

@dunxen
Copy link
Contributor Author

dunxen commented May 10, 2022

Needs rebase, it appears?

Beat me to it. Was quite behind on my local.

@dunxen dunxen force-pushed the 2022-05-actually-add-expiry branch from c962c18 to 717047c Compare May 10, 2022 17:13
jkczyz
jkczyz reviewed May 10, 2022
View reviewed changes
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Looks good modulo some test changes.

@dunxen dunxen force-pushed the 2022-05-actually-add-expiry branch from 717047c to 3369b29 Compare May 10, 2022 18:23
jkczyz
jkczyz approved these changes May 10, 2022
View reviewed changes
lightning-invoice/src/utils.rs
Comment on lines -231 to +234
channelmanager, keys_manager, network, amt_msat, description, duration
channelmanager, keys_manager, network, amt_msat,
description, duration, invoice_expiry_delta_secs
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would just wrap at 100 chars instead of making them evenly distributed to ease future automation

TheBlueMatt
TheBlueMatt approved these changes May 10, 2022
View reviewed changes
lightning-invoice/src/utils.rs
let invoice = ::utils::create_phantom_invoice::<EnforcingSigner, &test_utils::TestKeysInterface>(Some(payment_amt), payment_hash, "test".to_string(), 3600, route_hints, &nodes[1].keys_manager, Currency::BitcoinTestnet).unwrap();
let invoice = ::utils::create_phantom_invoice::<
EnforcingSigner, &test_utils::TestKeysInterface
>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: bleh, I hate these kinds of blank lines. Better to \n\t after the = or run over 100 chars than this :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll get to these in a follow-up today :) (also Jeff's)

@TheBlueMatt TheBlueMatt merged commit edd3030 into lightningdevkit:main May 10, 2022
@dunxen dunxen deleted the 2022-05-actually-add-expiry branch May 11, 2022 08:39
@dunxen dunxen mentioned this pull request May 11, 2022
Open
dunxen added a commit to dunxen/rust-lightning that referenced this pull request May 11, 2022
@dunxen
Address post-ACK formatting nits from lightningdevkit#1474
ea016cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkczyz jkczyz jkczyz approved these changes

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@tnull tnull tnull left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Include expiry in invoices
5 participants
@dunxen @codecov-commenter @TheBlueMatt @tnull @jkczyz