Skip to content

Add expiry to phantom invoice utility functions - #1422

Merged
valentinewallace merged 1 commit into
lightningdevkit:mainfrom
dunxen:2022-04-invoice-expiry
Apr 17, 2022
Merged

Add expiry to phantom invoice utility functions#1422
valentinewallace merged 1 commit into
lightningdevkit:mainfrom
dunxen:2022-04-invoice-expiry

Conversation

@dunxen

@dunxendunxen commented Apr 15, 2022

Copy link
Copy Markdown
Contributor

Closes#1411

@dunxen

dunxen commented Apr 15, 2022

Copy link
Copy Markdown
ContributorAuthor

Hmm. So I assumed we don't need to pass in a payment secret anymore. If this is wrong then I can fix up.

@codecov-commenter

codecov-commenter commented Apr 15, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1422 (8f047ca) into main (03f6550) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1422 +/- ##
==========================================
+ Coverage 90.84% 90.85% +0.01% 
==========================================
Files 74 74 Lines 41288 41361 +73 Branches 41288 41361 +73 ==========================================
+ Hits 37507 37580 +73 
Misses 3781 3781
Impacted FilesCoverage Δ
lightning-invoice/src/utils.rs96.57% <100.00%> (-0.11%)⬇️
lightning/src/ln/functional_tests.rs97.04% <0.00%> (-0.10%)⬇️
lightning-persister/src/util.rs98.90% <0.00%> (+3.12%)⬆️
lightning-block-sync/src/init.rs99.05% <0.00%> (+3.24%)⬆️

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 03f6550...8f047ca. Read the comment docs.

TheBlueMatt
TheBlueMatt reviewed Apr 15, 2022
View reviewed changes
Comment threadlightning-invoice/src/utils.rs Outdated
amt_msat: Option<u64>, description_hash: Sha256, payment_hash: PaymentHash,
payment_secret: PaymentSecret, phantom_route_hints: Vec<PhantomRouteHints>,
keys_manager: K, network: Currency,
amt_msat: Option<u64>, payment_hash: Option<PaymentHash>, invoice_expiry_delta_secs: u32, description_hash: Sha256, phantom_route_hints: Vec<PhantomRouteHints>, keys_manager: K, network: Currency,

@TheBlueMattTheBlueMattApr 15, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Lets break the line at least a bit so its not so long :). Try to break around 100 chars.

TheBlueMatt
TheBlueMatt reviewed Apr 15, 2022
View reviewed changes
Comment threadlightning-invoice/src/utils.rs Outdated
TheBlueMatt
TheBlueMatt reviewed Apr 15, 2022
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Basically lg, is there a reason this is still draft?

@dunxen

dunxen commented Apr 15, 2022

Copy link
Copy Markdown
ContributorAuthor

Basically lg, is there a reason this is still draft?

Ah, just wanted to check on something. Thought I had made some mistake in my conceptual understand. Will promote it out of draft now :)

@dunxen
dunxen marked this pull request as ready for review April 15, 2022 17:18
@dunxen
dunxenforce-pushed the 2022-04-invoice-expiry branch from bf41004 to 2d954c4CompareApril 15, 2022 18:30
valentinewallace
valentinewallace reviewed Apr 15, 2022
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great! I'm ACK if CI passes

Comment threadlightning-invoice/src/utils.rs Outdated
@dunxen
dunxenforce-pushed the 2022-04-invoice-expiry branch from 2d954c4 to b741ca5CompareApril 15, 2022 19:59
TheBlueMatt
TheBlueMatt reviewed Apr 15, 2022
View reviewed changes
Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning-invoice/src/utils.rs
@dunxen
dunxenforce-pushed the 2022-04-invoice-expiry branch from b741ca5 to 3170ac9CompareApril 16, 2022 07:22
TheBlueMatt
TheBlueMatt reviewed Apr 16, 2022
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Otherwise LGTM.

Comment threadlightning-invoice/src/utils.rs Outdated
@dunxen
dunxenforce-pushed the 2022-04-invoice-expiry branch from 3170ac9 to 8f047caCompareApril 16, 2022 20:50
@valentinewallace
valentinewallace merged commit b4c645a into lightningdevkit:mainApr 17, 2022
@dunxen
dunxen deleted the 2022-04-invoice-expiry branch April 17, 2022 22:05
@jkczyzjkczyz mentioned this pull request May 9, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

@valentinewallacevalentinewallace

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Include expiry in invoices

4 participants

@dunxen@codecov-commenter@TheBlueMatt@valentinewallace