search

lightningdevkit / ldk-node

A ready-to-go node implementation built using LDK.
Other
140 stars 72 forks source link

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

Closed slanesuke closed 3 weeks ago

slanesuke commented 1 month ago

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 commented 1 month ago

Seems some tests are failing currently

slanesuke commented 1 month ago

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.

slanesuke commented 1 month ago

rebased

slanesuke commented 1 month ago

@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 commented 1 month ago

@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: https://github.com/lightningdevkit/rust-lightning/issues/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.

slanesuke commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

rebased

tnull commented 1 month ago

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

slanesuke commented 1 month ago

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

slanesuke commented 3 weeks ago

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!