invoice: Use PaymentHash in raw invoice types #4363
Conversation
|
👋 I see @valentinewallace was un-assigned. |
jgmcalpine
force-pushed
the
invoice-raw-use-payment-hash
branch
5 times, most recently
from
da905d6 to
f967d8a
Compare
lightning/src/ln/channelmanager.rs
Outdated
| let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); | ||
| let payment_hash = invoice.payment_hash(); | ||
| let payment_hash = | ||
| PaymentHash(Sha256::from_byte_array(invoice.payment_hash().0).to_byte_array()); |
There was a problem hiding this comment.
Ah, good catch. That was a redundant round-trip left over from when I was refactoring the types. Simplified it to just use invoice.payment_hash() directly
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4363 +/- ##
=======================================
Coverage 86.09% 86.09%
=======================================
Files 156 156
Lines 102804 102853 +49
Branches 102804 102853 +49
=======================================
+ Hits 88508 88553 +45
+ Misses 11788 11784 -4
- Partials 2508 2516 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jgmcalpine
force-pushed
the
invoice-raw-use-payment-hash
branch
2 times, most recently
from
6b9f7ec to
59b1623
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Note: This is a breaking change for InvoiceBuilder. Downstream consumers (like ldk-node) will need to update their builder calls to pass PaymentHash directly.
Please note that we employ a "if you break it, you keep it" rule here by now. ;)
I.e., please make sure to open a PR on the LDK Node end to clean up the breakage as soon as this lands.
lightning/src/ln/channelmanager.rs
Outdated
| Err(()) => return None, | ||
| }; | ||
|
|
||
| let payment_hash = invoice.payment_hash(); |
| let mut invoice_builder = InvoiceBuilder::new(currency) | ||
| .description(description.to_string()) | ||
| .payment_hash(payment_hash) | ||
| .payment_hash(PaymentHash(payment_hash.to_byte_array())) |
There was a problem hiding this comment.
Here too we're building a sha256::Hash just to undo it.
lightning/src/ln/invoice_utils.rs
Outdated
| Bolt11InvoiceDescriptionRef::Direct(&Description::new("test".to_string()).unwrap()) | ||
| ); | ||
| assert_eq!(invoice.payment_hash(), payment_hash); | ||
| assert_eq!(invoice.payment_hash().0, payment_hash.0); |
Understood! I'm happy to handle the LDK Node fix. I'll keep an eye on this and open the fix PR in ldk-node after this lands. |
jgmcalpine
force-pushed
the
invoice-raw-use-payment-hash
branch
from
59b1623 to
d652d86
Compare
4 participants
Closes #4292
Note: This is a breaking change for InvoiceBuilder. Downstream consumers (like ldk-node) will need to update their builder calls to pass PaymentHash directly.
Refactors
RawBolt11Invoice,RawDataPart, andTaggedFieldto uselightning_types::payment::PaymentHashinstead ofbitcoin::hashes::sha256::Hash.This improves type safety and aligns the raw invoice types with the high-level
Bolt11Invoice.This is a follow-up to #4293 to complete the migration of invoice types to PaymentHash.
Specific changes:
TaggedField::PaymentHashnow holds aPaymentHash.InvoiceBuilder::payment_hashnow accepts aPaymentHashdirectly.serialization.
the TaggedField variant.
lightningcrate (channelmanager andinvoice_utils) to remove redundant hash conversions now that the builder
accepts the strong type.