Allow to manually register and claim payments #308

tnull · 2024-06-14T13:19:33Z

Fixes #280.
Fixes #57.

Previously, we wouldn't allow invoice generation for third-party payment hashes. However, as some users require this feature, we here allow them to generate invoices for which they supply the payment hashes, allowing them to retrieve the preimage out-of-bound.

To this end, we add receive_for_hash and receive_variable_amount_for_hash variants to the Bolt11Payment handler. Then, when we receive the payment, we emit a new PaymentClaimable event that users need to call claim_manually for with the correct preimage. Note that this currently only implements the BOLT11 flow as for BOLT12 additions to LDK will be necessary that are pending the next release.

We also add a commit resolving #57, i.e., adding a latest_update_timestamp field to PaymentDetails which allows to filter and sort payment based on how recent they are.

tnull · 2024-06-14T13:27:20Z

src/event.rs

+					// If this is known by the store but ChannelManager doesn't know the preimage,
+					// the payment has been registered via `_for_hash` variants and needs to be manually claimed via
+					// user interaction.
+					match info.kind {


As I'm currently not fully sure how we'll be able to cover the BOLT12 flow, I refrained from adding a new PaymentKind for these kind of payments here. Instead, I'm just leaning on us knowing the payment but ChannelManager not knowing the preimage. We might however want to 'upgrade' to a more expressive PaymentKind once we include the BOLT12 flow. Not sure if you have an opinion on this, @jkczyz?

Do we need to differentiate them? Checking PaymentPurpose::preimage().is_none() irrespective of the variant should work, IIUC.

Do we need to differentiate them? Checking PaymentPurpose::preimage().is_none() irrespective of the variant should work, IIUC.

Well, for BOLT12 the flow will be a bit different, as we currently don't register inbound payments with the payment store. We'll then need to mark them as pending in the new event I guess. At that point we could consider whether we want to add a different PaymentKind variant, or, as we'll generally aim to move the invoice/offer registration to an to-be-added invoice/offer storage could consider whether to add a is_hold or similar flag there. At least these are things we could discuss in the future, in any case for that reason I'd kept it as simple as possible for now.

jkczyz · 2024-06-14T16:23:34Z

src/payment/bolt11.rs

+	/// payment hash.
+	///
+	/// [`claim_manually`]: Self::claim_manually
+	pub fn receive_for_hash(


More of a high-level question, but I wonder if we should use either:

a builder for the parameters so that there are fewer methods or

change the calling structure such that you would pick send/receive prior to bolt11/bolt12/onchain, while also using some builder to add the amount/hash/etc.

For the latter, the call might looks something like:

node.inbound_payment() .bolt12() .amount(10_000) .payment_hash(PaymentHash([42; 32]) .receive();

I can't recall our earlier discussions around this. I seem to recall maybe some concern around bindings, though.

Hmm, so generally I'd prefer to try to keep the API as simple method calls at this point as it's easier to pick-up/experiment with rather than to having to understand a builder pattern (especially in bindings languages where it might not be as common/entirely uncommon).

Also, exposing builder in Uniffi is bit awkward as we can't take &mut self or mut self there, which would require us to always wrap an inner builder (see NodeBuilder). However, we could of course work around that via some smart macros that would allow to do that automatically for us, if we find we should do this for many more places.

Regarding the call structure: I'm not entirely convinced adding one more layer to the API much improves simplicity at this point. If we find that we need to keep adding methods, we should probably revisit this idea, however, I really hope that we should be close to done covering additional use-cases by now. Btw, we're about to get two additional payment handlers (payjoin and unified_qr) that will partially re-use/combine the bolt11/bolt12/onchain handlers.

TLDR: Not sure we should go further at this point in time, but agree that we might want to reevaluate the API structure in the future, especially when we're positive that we provide a simplified version. For that I'd still like to learn a bit more about changes-to-come (e.g. regarding human-readable names) and user requirements though. Seems like a something we should reevaluate for ~v0.5 or so?

src/payment/bolt11.rs

jkczyz · 2024-06-14T16:40:43Z

src/event.rs

+					// If this is known by the store but ChannelManager doesn't know the preimage,
+					// the payment has been registered via `_for_hash` variants and needs to be manually claimed via
+					// user interaction.
+					match info.kind {


Do we need to differentiate them? Checking PaymentPurpose::preimage().is_none() irrespective of the variant should work, IIUC.

src/payment/store.rs

src/event.rs

src/payment/bolt11.rs

tests/common/mod.rs

src/payment/store.rs

src/payment/bolt11.rs

tests/common/mod.rs

src/payment/bolt11.rs

src/event.rs

jkczyz

Go ahead and squash. I'll take a final look in the morning.

tnull · 2024-06-18T07:39:09Z

Go ahead and squash. I'll take a final look in the morning.

Squashed without additional changes.

src/payment/bolt11.rs

jkczyz · 2024-06-18T15:52:21Z

src/payment/bolt11.rs

+			if let Some(expected_amount_msat) = details.amount_msat {
+				if claimable_amount_msat < expected_amount_msat {
+					log_error!(
+						self.logger,
+						"Failed to manually claim payment {} as the claimable amount is less than expected",
+						payment_id
+					);
+					return Err(Error::InvalidAmount);
+				}
+			}


Any reason why we don't do this for the user prior to generating an event? If not, they wouldn't need to pass the amount here.

I considered that but went this way as it felt like breaking the API contract if we just auto-fail in this case. At least with the current version users decide a) exactly when they'd like to fail and b) could bend the value if they somehow decided it's fine for them to claim less than expected. Let me know if you think immediately failing if the amounts don't match would be much preferable though..

tests/common/mod.rs

... requiring users to manucally claim them.

.. we previously got reports from users trying to pay their own JIT invoice, which we currently don't support (and possibly never will). In order to avoid entering any weird states, we just reject our own circular payment.

.. which allows to filter and sort payment based on how recent they are.

tnull · 2024-06-19T09:34:24Z

Force-pushed with the following changes:

> git diff-tree -U2 32fb2a8 a678c5e
diff --git a/src/event.rs b/src/event.rs
index a0363ef..838df42 100644
--- a/src/event.rs
+++ b/src/event.rs
@@ -87,7 +87,7 @@ pub enum Event {
        /// A payment for a previously-registered payment hash has been received.
        ///
-       /// This needs to be manually claimed by supplying the correct pre-image to [`claim_for_hash`].
+       /// This needs to be manually claimed by supplying the correct preimage to [`claim_for_hash`].
        ///
-       /// If the the provided parameters don't match the expectations or the pre-image can't be
+       /// If the the provided parameters don't match the expectations or the preimage can't be
        /// retrieved in time, should be failed-back via [`fail_for_hash`].
        ///
diff --git a/src/payment/bolt11.rs b/src/payment/bolt11.rs
index a543101..e8d030b 100644
--- a/src/payment/bolt11.rs
+++ b/src/payment/bolt11.rs
@@ -266,5 +266,5 @@ impl Bolt11Payment {
        /// been registered via [`receive_for_hash`] or [`receive_variable_amount_for_hash`].
        ///
-       /// This should be called in reponse to a [`PaymentClaimable`] event as soon as the pre-image is
+       /// This should be called in reponse to a [`PaymentClaimable`] event as soon as the preimage is
        /// available.
        ///
@@ -323,5 +323,5 @@ impl Bolt11Payment {
        ///
        /// This should be called in reponse to a [`PaymentClaimable`] event if the payment needs to be
-       /// failed back, e.g., if the correct pre-image can't be retrieved in time before the claim
+       /// failed back, e.g., if the correct preimage can't be retrieved in time before the claim
        /// deadline has been reached.
        ///
@@ -473,5 +473,5 @@ impl Bolt11Payment {
                                .get_payment_preimage(payment_hash, payment_secret.clone())
                                .ok();
-                       debug_assert!(res.is_some(), "We just let ChannelManager create an inbound payment, it can't have forgotten the pre-image by now.");
+                       debug_assert!(res.is_some(), "We just let ChannelManager create an inbound payment, it can't have forgotten the preimage by now.");
                        res
                } else {
diff --git a/tests/common/mod.rs b/tests/common/mod.rs
index f587b0c..5959bd5 100644
--- a/tests/common/mod.rs
+++ b/tests/common/mod.rs
@@ -609,5 +609,5 @@ pub(crate) fn do_channel_full_cycle<E: ElectrumApi>(
        assert!(matches!(node_b.payment(&payment_id).unwrap().kind, PaymentKind::Bolt11 { .. }));

-       // Test manually registered/claimed payments.
+       // Test claiming manually registered payments.
        let invoice_amount_3_msat = 5_532_000;
        let manual_preimage = PaymentPreimage([42u8; 32]);
@@ -646,4 +646,50 @@ pub(crate) fn do_channel_full_cycle<E: ElectrumApi>(
        assert!(matches!(node_b.payment(&manual_payment_id).unwrap().kind, PaymentKind::Bolt11 { .. }));

+       // Test failing manually registered payments.
+       let invoice_amount_4_msat = 5_532_000;
+       let manual_fail_preimage = PaymentPreimage([43u8; 32]);
+       let manual_fail_payment_hash =
+               PaymentHash(Sha256::hash(&manual_fail_preimage.0).to_byte_array());
+       let manual_fail_invoice = node_b
+               .bolt11_payment()
+               .receive_for_hash(invoice_amount_3_msat, &"asdf", 9217, manual_fail_payment_hash)
+               .unwrap();
+       let manual_fail_payment_id = node_a.bolt11_payment().send(&manual_fail_invoice).unwrap();
+
+       expect_payment_claimable_event!(
+               node_b,
+               manual_fail_payment_id,
+               manual_fail_payment_hash,
+               invoice_amount_4_msat
+       );
+       node_b.bolt11_payment().fail_for_hash(manual_fail_payment_hash).unwrap();
+       expect_event!(node_a, PaymentFailed);
+       assert_eq!(node_a.payment(&manual_fail_payment_id).unwrap().status, PaymentStatus::Failed);
+       assert_eq!(
+               node_a.payment(&manual_fail_payment_id).unwrap().direction,
+               PaymentDirection::Outbound
+       );
+       assert_eq!(
+               node_a.payment(&manual_fail_payment_id).unwrap().amount_msat,
+               Some(invoice_amount_4_msat)
+       );
+       assert!(matches!(
+               node_a.payment(&manual_fail_payment_id).unwrap().kind,
+               PaymentKind::Bolt11 { .. }
+       ));
+       assert_eq!(node_b.payment(&manual_fail_payment_id).unwrap().status, PaymentStatus::Failed);
+       assert_eq!(
+               node_b.payment(&manual_fail_payment_id).unwrap().direction,
+               PaymentDirection::Inbound
+       );
+       assert_eq!(
+               node_b.payment(&manual_fail_payment_id).unwrap().amount_msat,
+               Some(invoice_amount_4_msat)
+       );
+       assert!(matches!(
+               node_b.payment(&manual_fail_payment_id).unwrap().kind,
+               PaymentKind::Bolt11 { .. }
+       ));
+
        // Test spontaneous/keysend payments
        println!("\nA send_spontaneous_payment");
@@ -677,6 +723,6 @@ pub(crate) fn do_channel_full_cycle<E: ElectrumApi>(
                PaymentKind::Spontaneous { .. }
        ));
-       assert_eq!(node_a.list_payments().len(), 5);
-       assert_eq!(node_b.list_payments().len(), 6);
+       assert_eq!(node_a.list_payments().len(), 6);
+       assert_eq!(node_b.list_payments().len(), 7);

        println!("\nB close_channel (force: {})", force_close);

tvolk131 · 2024-06-19T13:51:19Z

src/payment/bolt11.rs

+	/// [`PaymentClaimable`]: crate::Event::PaymentClaimable
+	/// [`PaymentReceived`]: crate::Event::PaymentReceived
+	pub fn claim_for_hash(
+		&self, payment_hash: PaymentHash, claimable_amount_msat: u64, preimage: PaymentPreimage,


Could we remove the payment_hash field and calculate it from the preimage?

We could, but I intentionally let the users supply it as the API is focused around custom payment hashes. They will however also know it from PaymentClaimable.

tnull requested a review from jkczyz June 14, 2024 13:19

tnull commented Jun 14, 2024

View reviewed changes

tnull force-pushed the 2024-06-manual-payment-hash-registrations branch from 6f363ca to 32bf91c Compare June 14, 2024 13:34

tnull added this to the 0.3 milestone Jun 14, 2024

jkczyz reviewed Jun 14, 2024

View reviewed changes

tvolk131 reviewed Jun 15, 2024

View reviewed changes

src/event.rs Show resolved Hide resolved

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

tnull force-pushed the 2024-06-manual-payment-hash-registrations branch 3 times, most recently from 1c5dbd5 to ddd34d9 Compare June 17, 2024 09:02

tnull mentioned this pull request Jun 17, 2024

Consider whether to support looping payments #292

Closed

tvolk131 reviewed Jun 17, 2024

View reviewed changes

tnull force-pushed the 2024-06-manual-payment-hash-registrations branch 2 times, most recently from 7fb9eef to 36838f7 Compare June 17, 2024 13:47

tvolk131 approved these changes Jun 17, 2024

View reviewed changes

jkczyz reviewed Jun 17, 2024

View reviewed changes

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

src/payment/bolt11.rs Show resolved Hide resolved

src/event.rs Show resolved Hide resolved

tnull force-pushed the 2024-06-manual-payment-hash-registrations branch from 36838f7 to c841e7f Compare June 17, 2024 18:35

jkczyz reviewed Jun 18, 2024

View reviewed changes

tnull force-pushed the 2024-06-manual-payment-hash-registrations branch from c841e7f to 32fb2a8 Compare June 18, 2024 07:38

jkczyz reviewed Jun 18, 2024

View reviewed changes

tnull added 5 commits June 19, 2024 11:25

Allow to generate invoices for custom payment hashes

018a5b6

... requiring users to manucally claim them.

Implement and yield PaymentClaimable events

be6e8c2

Test manual-claiming flow

d5709b7

Refuse circular payments

74943cd

.. we previously got reports from users trying to pay their own JIT invoice, which we currently don't support (and possibly never will). In order to avoid entering any weird states, we just reject our own circular payment.

Add a latest_update_timestamp field to PaymentDetails

a678c5e

.. which allows to filter and sort payment based on how recent they are.

tnull force-pushed the 2024-06-manual-payment-hash-registrations branch from 32fb2a8 to a678c5e Compare June 19, 2024 09:32

tvolk131 reviewed Jun 19, 2024

View reviewed changes

jkczyz approved these changes Jun 20, 2024

View reviewed changes

tnull merged commit b180a65 into lightningdevkit:main Jun 20, 2024

rolznz mentioned this pull request Jun 24, 2024

Experiment with LDK-node registering and claiming payment hashes getAlby/hub#45

Open

Allow to manually register and claim payments #308

Allow to manually register and claim payments #308

Uh oh!

Conversation

tnull commented Jun 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

tnull commented Jun 18, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull commented Jun 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tnull Jun 17, 2024 •

edited

Loading

tnull Jun 19, 2024 •

edited

Loading