Expose `payer_note` in `PaymentKind::Bolt12` #327

slanesuke · 2024-07-15T22:10:36Z

Resolves #320

This PR adds support for including the payer_note field in PaymentKind::Bolt12. It also updates the relevant parts of the code to handle where payer_note is required.

tnull

Thanks for looking into this!

While we're at it, we should probably also add the payer_note field to the Bolt12Refund variant and allow to set it via initiate_refund in a second commit.

src/event.rs

src/payment/store.rs

src/payment/bolt12.rs

src/payment/store.rs

tnull · 2024-07-19T12:05:52Z

Seems some tests are failing currently

tnull

Generally already looks pretty good, but please clean up the git history so that the changes match the commit messages. Preferably splitting to one commit per field or similar.

src/event.rs

src/payment/store.rs

src/payment/bolt12.rs

tnull · 2024-07-22T11:17:45Z

src/payment/bolt12.rs

@@ -95,6 +96,9 @@ impl Bolt12Payment {
 					preimage: None,
 					secret: None,
 					offer_id: offer.id(),
+					payer_note: payer_note.map(UntrustedString),
+					payer_id: None,


Mh, this is a bit weird as internally to LDK we very well know the payer_id at this point. Maybe we should consider returning the InvoiceRequest from pay_for_offer.

For consistency it would also be good to actually set the payer note based on the invoice request field rather than what the user gave us.

Mh, this is a bit weird as internally to LDK we very well know the payer_id at this point. Maybe we should consider returning the InvoiceRequest from pay_for_offer.

For consistency it would also be good to actually set the payer note based on the invoice request field rather than what the user gave us.

Should I open an issue on rust-lightning before making the change, or should I just make the change and open a PR directly?

We discussed already a bit offline and may actually arrive at removing payer_id from the LDK APIs if we can make it work, but it's not sure yet. See lightningdevkit/rust-lightning#3198

slanesuke · 2024-07-22T21:00:38Z

For now, I removed the payer_id fields. I also ended up adding a quantity parameter that's set by the user in the BOLT 12 send method.

src/payment/bolt12.rs

slanesuke · 2024-07-31T13:34:58Z

rebased

tnull

Mostly looks good to me, just one comment.

Tests are currently failing due to inconsistent argument ordering:

error[E0308]: arguments to this method are incorrect
   --> tests/integration_tests_rust.rs:470:4
    |
470 |         .send_using_amount(&offer, None, None, less_than_offer_amount)
    |          ^^^^^^^^^^^^^^^^^               ----  ---------------------- expected `std::option::Option<u64>`, found `u64`
    |                                          |
    |                                          expected `u64`, found `std::option::Option<_>`
    |
note: method defined here
   --> /Users/erohrer/workspace/ldk-node/src/payment/bolt12.rs:152:9
    |
152 |     pub fn send_using_amount(
    |            ^^^^^^^^^^^^^^^^^
help: swap these arguments
    |
470 |         .send_using_amount(&offer, None, less_than_offer_amount, None)
    |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error[E0308]: arguments to this method are incorrect
   --> tests/integration_tests_rust.rs:474:4
    |
474 |         .send_using_amount(&offer, None, None, expected_amount_msat)
    |          ^^^^^^^^^^^^^^^^^               ----  -------------------- expected `std::option::Option<u64>`, found `u64`
    |                                          |
    |                                          expected `u64`, found `std::option::Option<_>`
    |
note: method defined here
   --> /Users/erohrer/workspace/ldk-node/src/payment/bolt12.rs:152:9
    |
152 |     pub fn send_using_amount(
    |            ^^^^^^^^^^^^^^^^^
help: swap these arguments
    |
474 |         .send_using_amount(&offer, None, expected_amount_msat, None)
    |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error[E0308]: arguments to this method are incorrect
   --> tests/integration_tests_rust.rs:514:39
    |
514 |     let refund = node_b.bolt12_payment().initiate_refund(overpaid_amount, None, 3600).unwrap();
    |                                          ^^^^^^^^^^^^^^^                  ----  ---- expected `std::option::Option<u64>`, found `{integer}`
    |                                                                           |
    |                                                                           expected `u32`, found `std::option::Option<_>`
    |
note: method defined here
   --> /Users/erohrer/workspace/ldk-node/src/payment/bolt12.rs:316:9
    |
316 |     pub fn initiate_refund(
    |            ^^^^^^^^^^^^^^^
help: swap these arguments
    |
514 |     let refund = node_b.bolt12_payment().initiate_refund(overpaid_amount, 3600, None).unwrap();
    |                                                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

For more information about this error, try `rustc --explain E0308`.
error: could not compile `ldk-node` (test "integration_tests_rust") due to 4 previous errors
exit 101

Feel free to squash fixups and rebase on current main to have CI pass.

tnull · 2024-08-06T11:18:43Z

src/payment/bolt12.rs

@@ -143,14 +150,13 @@ impl Bolt12Payment {
 	/// If `payer_note` is `Some` it will be seen by the recipient and reflected back in the invoice
 	/// response.
 	pub fn send_using_amount(
-		&self, offer: &Offer, payer_note: Option<String>, amount_msat: u64,
+		&self, offer: &Offer, payer_note: Option<String>, amount_msat: u64, quantity: Option<u64>,


Let's move amount_msat directly after offer to have all optional fields at the end.

tnull

One comment, otherwise LGTM.

Also feel free to rebase to fix CI

src/payment/bolt12.rs

tnull

Cool, just two more nits.

Also, could we add some test coverage asserting that the payer_note and quantity we're setting while sending/initiating refunds actually appear as expected on the receiver's end?

bindings/ldk_node.udl

src/payment/bolt12.rs

slanesuke · 2024-08-08T21:31:42Z

@tnull I pushed an updated simple_bolt12_send_receive unit test where I added a quantity and payer_note to send, send_using_amount, and initiate_refund. Unfortunately initiate_refund is the only Bolt12Payment method that was successful when the quantity was Some. Otherwise, the send methods failed due to unexpected/undetermined behavior. Do you think you can spot the problem? It seems to be failing when we call pay_for_offer but I'm unsure..

tnull · 2024-08-09T08:44:11Z

@tnull I pushed an updated simple_bolt12_send_receive unit test where I added a quantity and payer_note to send, send_using_amount, and initiate_refund. Unfortunately initiate_refund is the only Bolt12Payment method that was successful when the quantity was Some. Otherwise, the send methods failed due to unexpected/undetermined behavior. Do you think you can spot the problem? It seems to be failing when we call pay_for_offer but I'm unsure..

Ah, this seems to be a bug (?), i.e. the Offer's default Quantity::One variant being incompatible with setting quantity: Some(1), which is rather confusing: lightningdevkit/rust-lightning#3233.

I think for now we just want to set the supported_quantity in the offer to fix our tests. Btw, this also shows we still need to add a supported_quantity field to receive to allow setting a quantity when generating the offer. I'm a bit on the fence right now if we need to create our own Quantity enum for this (to make it bindings-compatible) or simply have it be an Option<u64> (which would require not covering the Unbounded variant and erroring if the given quantity is 0). I think we still should do the latter (i.e., taking it as an Option<u64>) and possibly reconsider if a user complains that they require the Unbounded case.

tnull · 2024-08-09T08:18:51Z

tests/integration_tests_rust.rs

@@ -425,16 +425,27 @@ fn simple_bolt12_send_receive() {

 	let expected_amount_msat = 100_000_000;
 	let offer = node_b.bolt12_payment().receive(expected_amount_msat, "asdf").unwrap();
-	let payment_id = node_a.bolt12_payment().send(&offer, None).unwrap();
+	let quantity = Some(1);


Could you prefix the expected quantity and payer note with expected_? This also avoids the necessity to rename the fields in the match statements below.

slanesuke · 2024-08-09T13:53:39Z

@tnull I pushed an updated simple_bolt12_send_receive unit test where I added a quantity and payer_note to send, send_using_amount, and initiate_refund. Unfortunately initiate_refund is the only Bolt12Payment method that was successful when the quantity was Some. Otherwise, the send methods failed due to unexpected/undetermined behavior. Do you think you can spot the problem? It seems to be failing when we call pay_for_offer but I'm unsure..

Ah, this seems to be a bug (?), i.e. the Offer's default Quantity::One variant being incompatible with setting quantity: Some(1), which is rather confusing: lightningdevkit/rust-lightning#3233.

I think for now we just want to set the supported_quantity in the offer to fix our tests. Btw, this also shows we still need to add a supported_quantity field to receive to allow setting a quantity when generating the offer. I'm a bit on the fence right now if we need to create our own Quantity enum for this (to make it bindings-compatible) or simply have it be an Option<u64> (which would require not covering the Unbounded variant and erroring if the given quantity is 0). I think we still should do the latter (i.e., taking it as an Option<u64>) and possibly reconsider if a user complains that they require the Unbounded case.

Thanks for pointing that out! But, I noticed that it's not just Some(1) that fails, the send methods fail regardless of the value set for quantity. It seems like the issue could be something else maybe?

tnull · 2024-08-12T10:15:20Z

Thanks for pointing that out! But, I noticed that it's not just Some(1) that fails, the send methods fail regardless of the value set for quantity. It seems like the issue could be something else maybe?

Well this is expected: if you don't set a supported_quantity during offer generation, anything >1 would also fail. IMO it's just confusing that it also fails for 1. But, yeah, hence we should allow setting supported_quantity in this PR as mentioned above.

slanesuke · 2024-08-12T21:49:42Z

Thanks for pointing that out! But, I noticed that it's not just Some(1) that fails, the send methods fail regardless of the value set for quantity. It seems like the issue could be something else maybe?

Well this is expected: if you don't set a supported_quantity during offer generation, anything >1 would also fail. IMO it's just confusing that it also fails for 1. But, yeah, hence we should allow setting supported_quantity in this PR as mentioned above.

Ah okay! Makes sense

slanesuke · 2024-08-13T12:44:25Z

rebased

tnull · 2024-08-13T13:16:13Z

Mh, I think you'll need another rebase, seems the CI caching issue wasn't fully fixed afterall. Sorry!

slanesuke · 2024-08-13T17:38:28Z

So while setting the quantity to None when sending a payment via the bolt12 send results in a an InvoiceRequestCreationFailed. The only way I was able to get around it was defaulting the quantity to 1 when the user sets it to None. I know this isn't ideal though

tnull

So while setting the quantity to None when sending a payment via the bolt12 send results in a an InvoiceRequestCreationFailed. The only way I was able to get around it was defaulting the quantity to 1 when the user sets it to None. I know this isn't ideal though

Right, this is expected as you're currently always setting supported_quantity on the offer. Please let's just not do this and drop the default quantity of 1.

src/payment/bolt12.rs

tnull

LGTM

One tiny nit, feel free to address it while squashing in the (last) fixup commit, so we can land this PR ASAP.

src/payment/bolt12.rs

Add support for including `payer_note` in `Bolt12Offer` and `PaymentKind::Bolt12` and updated the relevant code to handle where the new `payer_note` field was required.

slanesuke · 2024-08-19T15:20:20Z

LGTM

One tiny nit, feel free to address it while squashing in the (last) fixup commit, so we can land this PR ASAP.

Okay, squashed!

tnull reviewed Jul 17, 2024

View reviewed changes

tnull reviewed Jul 18, 2024

View reviewed changes

src/payment/store.rs Outdated Show resolved Hide resolved

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch 2 times, most recently from d4a92b0 to a2c1144 Compare July 19, 2024 23:16

tnull reviewed Jul 22, 2024

View reviewed changes

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch from a2c1144 to d89ad6c Compare July 22, 2024 20:52

tnull reviewed Jul 23, 2024

View reviewed changes

src/payment/bolt12.rs Show resolved Hide resolved

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch 2 times, most recently from 7d1bbd9 to b1ddd4d Compare July 23, 2024 16:32

tnull reviewed Jul 24, 2024

View reviewed changes

src/payment/bolt12.rs Outdated Show resolved Hide resolved

src/payment/bolt12.rs Outdated Show resolved Hide resolved

src/payment/bolt12.rs Outdated Show resolved Hide resolved

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch 3 times, most recently from 813856f to 1b0f4da Compare July 31, 2024 13:34

tnull reviewed Aug 6, 2024

View reviewed changes

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch from 1b0f4da to 31f517d Compare August 6, 2024 21:12

tnull reviewed Aug 7, 2024

View reviewed changes

src/payment/bolt12.rs Show resolved Hide resolved

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch from 31f517d to f35c8bb Compare August 7, 2024 15:44

tnull reviewed Aug 8, 2024

View reviewed changes

bindings/ldk_node.udl Outdated Show resolved Hide resolved

src/payment/bolt12.rs Outdated Show resolved Hide resolved

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch from f35c8bb to 4788f82 Compare August 8, 2024 21:30

tnull mentioned this pull request Aug 9, 2024

offer::Quantity::One is incompatible with quantity: Some(1) lightningdevkit/rust-lightning#3233

Open

tnull reviewed Aug 9, 2024

View reviewed changes

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch from 4788f82 to afe0c65 Compare August 12, 2024 22:03

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch from afe0c65 to c34abe3 Compare August 13, 2024 12:43

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch 2 times, most recently from 3bbba2f to b45b799 Compare August 13, 2024 17:33

tnull reviewed Aug 14, 2024

View reviewed changes

src/payment/bolt12.rs Outdated Show resolved Hide resolved

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch from b45b799 to ecf91cb Compare August 14, 2024 20:08

tnull reviewed Aug 19, 2024

View reviewed changes

src/payment/bolt12.rs Show resolved Hide resolved

Add payer_note in PaymentKind::Bolt12

683bfb3

Add support for including `payer_note` in `Bolt12Offer` and `PaymentKind::Bolt12` and updated the relevant code to handle where the new `payer_note` field was required.

slanesuke force-pushed the 2024-07-expose-payer_note-in-PKbolt12 branch from ecf91cb to 683bfb3 Compare August 19, 2024 15:19

tnull approved these changes Aug 19, 2024

View reviewed changes

tnull merged commit 76fb23f into lightningdevkit:main Aug 19, 2024
6 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `payer_note` in `PaymentKind::Bolt12` #327

Expose `payer_note` in `PaymentKind::Bolt12` #327

slanesuke commented Jul 15, 2024

tnull left a comment

tnull commented Jul 19, 2024

tnull left a comment

tnull Jul 22, 2024

slanesuke Jul 22, 2024

tnull Jul 22, 2024

slanesuke commented Jul 22, 2024

slanesuke commented Jul 31, 2024

tnull left a comment

tnull Aug 6, 2024

tnull left a comment

tnull left a comment

slanesuke commented Aug 8, 2024

tnull commented Aug 9, 2024

tnull Aug 9, 2024

slanesuke commented Aug 9, 2024 •

edited

Loading

tnull commented Aug 12, 2024

slanesuke commented Aug 12, 2024

slanesuke commented Aug 13, 2024

tnull commented Aug 13, 2024

slanesuke commented Aug 13, 2024

tnull left a comment

tnull left a comment •

edited

Loading

slanesuke commented Aug 19, 2024

Expose payer_note in PaymentKind::Bolt12 #327

Expose payer_note in PaymentKind::Bolt12 #327

Conversation

slanesuke commented Jul 15, 2024

tnull left a comment

Choose a reason for hiding this comment

tnull commented Jul 19, 2024

tnull left a comment

Choose a reason for hiding this comment

tnull Jul 22, 2024

Choose a reason for hiding this comment

slanesuke Jul 22, 2024

Choose a reason for hiding this comment

tnull Jul 22, 2024

Choose a reason for hiding this comment

slanesuke commented Jul 22, 2024

slanesuke commented Jul 31, 2024

tnull left a comment

Choose a reason for hiding this comment

tnull Aug 6, 2024

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

slanesuke commented Aug 8, 2024

tnull commented Aug 9, 2024

tnull Aug 9, 2024

Choose a reason for hiding this comment

slanesuke commented Aug 9, 2024 • edited Loading

tnull commented Aug 12, 2024

slanesuke commented Aug 12, 2024

slanesuke commented Aug 13, 2024

tnull commented Aug 13, 2024

slanesuke commented Aug 13, 2024

tnull left a comment

Choose a reason for hiding this comment

tnull left a comment • edited Loading

Choose a reason for hiding this comment

slanesuke commented Aug 19, 2024

Expose `payer_note` in `PaymentKind::Bolt12` #327

Expose `payer_note` in `PaymentKind::Bolt12` #327

slanesuke commented Aug 9, 2024 •

edited

Loading

tnull left a comment •

edited

Loading