Skip to content

Move BlindedPayInfo into BlindedPaymentPath #3245

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

Conversation

valentinewallace
Copy link
Contributor

Rather than a BOLT 12 invoice's paths containing tuples of (BlindedPayInfo, BlindedPaymentPath), move the payinfo into the path itself.

@valentinewallace valentinewallace force-pushed the 2024-07-blinded-pay-path-refactor branch 2 times, most recently from 143e794 to 402682e Compare August 16, 2024 19:22
@valentinewallace valentinewallace force-pushed the 2024-07-blinded-pay-path-refactor branch from 402682e to f9bc326 Compare August 19, 2024 19:08
@valentinewallace valentinewallace marked this pull request as ready for review August 19, 2024 19:08
@@ -772,7 +775,7 @@ impl Writeable for PaymentParameters {
(5, self.max_channel_saturation_power_of_half, required),
(6, self.expiry_time, option),
(7, self.previously_failed_channels, required_vec),
(8, *blinded_hints, optional_vec),
(8, blinded_hints, option),
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 think previously we were writing a length prefix for this, actually... Giving this a second look.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, used to be just a count and then elements, now its a yes/no flag followed by a length and the elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, the current code is passing a round trip ser test actually. I don't think there's a yes/no flag written for options, and optional_vec writes as WithoutLength, IIUC, so it might be fine? Will expand the test to include multiple hints..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, sorry, I was confusing myself on the option, but had missed its an IterableOnwed which doesnt have a length unlike a vec.

@valentinewallace valentinewallace force-pushed the 2024-07-blinded-pay-path-refactor branch from f9bc326 to 305f03b Compare August 19, 2024 20:14
@valentinewallace
Copy link
Contributor Author

Rebased.

@valentinewallace valentinewallace force-pushed the 2024-07-blinded-pay-path-refactor branch from 305f03b to a052589 Compare August 19, 2024 20:22
@valentinewallace
Copy link
Contributor Author

Sorry, one more rustfmt + fuzz fix to push.

Also removes the implementation of Writeable for BlindedPaymentPath, to ensure
callsites are explicit about what they're writing.
@valentinewallace valentinewallace force-pushed the 2024-07-blinded-pay-path-refactor branch from a052589 to 6522d60 Compare August 19, 2024 20:28
@valentinewallace valentinewallace force-pushed the 2024-07-blinded-pay-path-refactor branch from 6522d60 to 4ef83a0 Compare August 19, 2024 20:42
@valentinewallace
Copy link
Contributor Author

And one more doc test :(

diff --git a/lightning/src/offers/invoice.rs b/lightning/src/offers/invoice.rs
index d361c7ce4..40ee40090 100644
--- a/lightning/src/offers/invoice.rs
+++ b/lightning/src/offers/invoice.rs
@@ -29,8 +29,8 @@
 //! use lightning::util::ser::Writeable;
 //!
 //! # use lightning::ln::types::PaymentHash;
-//! # use lightning::offers::invoice::{BlindedPayInfo, ExplicitSigningPubkey, InvoiceBuilder};
-//! # use lightning::blinded_path::payment::BlindedPaymentPath;
+//! # use lightning::offers::invoice::{ExplicitSigningPubkey, InvoiceBuilder};
+//! # use lightning::blinded_path::payment::{BlindedPayInfo, BlindedPaymentPath};
 //! #
 //! # fn create_payment_paths() -> Vec<BlindedPaymentPath> { unimplemented!() }
 //! # fn create_payment_hash() -> PaymentHash { unimplemented!() }

Copy link

codecov bot commented Aug 19, 2024

Codecov Report

Attention: Patch coverage is 97.29730% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.69%. Comparing base (fb4403f) to head (4ef83a0).
Report is 4 commits behind head on main.

Files Patch % Lines
lightning/src/blinded_path/payment.rs 88.57% 1 Missing and 3 partials ⚠️
lightning/src/offers/invoice.rs 95.65% 0 Missing and 1 partial ⚠️
lightning/src/util/ser.rs 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3245      +/-   ##
==========================================
- Coverage   89.69%   89.69%   -0.01%     
==========================================
  Files         125      125              
  Lines      102794   102801       +7     
  Branches   102794   102801       +7     
==========================================
+ Hits        92206    92210       +4     
- Misses       7869     7874       +5     
+ Partials     2719     2717       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt TheBlueMatt merged commit 61153f1 into lightningdevkit:main Aug 19, 2024
18 of 21 checks passed
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.

3 participants