-
Notifications
You must be signed in to change notification settings - Fork 364
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
Invoice utils: allow to set both description hash and payment hash #3371
Invoice utils: allow to set both description hash and payment hash #3371
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3371 +/- ##
==========================================
- Coverage 89.62% 89.59% -0.04%
==========================================
Files 127 127
Lines 103534 103762 +228
Branches 103534 103762 +228
==========================================
+ Hits 92797 92963 +166
- Misses 8038 8105 +67
+ Partials 2699 2694 -5 ☔ View full report in Codecov by Sentry. |
.map_err(|()| SignOrCreationError::CreationError(CreationError::InvalidAmount))?; | ||
_create_invoice_from_channelmanager_and_duration_since_epoch_with_payment_hash( | ||
channelmanager, node_signer, logger, network, amt_msat, | ||
Bolt11InvoiceDescription::Hash(&description_hash), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't recall if there was a reason why we can't take Bolt11InvoiceDescription
as a parameter instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the lifetimes don't lend themselves to being exposed in bindings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. Though an owned version would work, I suppose. No need to do it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really need to replace this garbage with some kind of config struct/builder pattern so its not a total mess. #3375
I discovered we had no API method to specify both the description hash and the payment hash at the same time.
Here, we add such a helper method, although mid-term we probably should move to a different pattern here (builder?) as the current way is rather inflexible and the method names get ridiculously long.