Refactor and modularize payment APIs

[tnull]

Based on #243.
Based on #244.
Based on #266.

Another prefactor to #193, splitting this out to reduce rebase pain, e.g., with #141.

As we're about to introduce a bunch more payment API methods for BOLT12 support, we first clean up and declutter our payment APIs so far.

What previously were individual methods on Node we now move to individual PaymentHandlers:

• Bolt11PaymentHandler
• SpontaneousPaymentHandler
• OnchainPaymentHandler

While we're at it, we slightly remove the API methods to shorten them, allowing to call them like so:

	let invoice = Bolt11Invoice::from_str("INVOICE_STR").unwrap();
	node.bolt11_payment().send(&invoice).unwrap();

Additionally, we move PaymentStore to a sub-module of crate::payment to clean up our top-level docs a bit.

[tnull]

@jkczyz Any thoughts on the naming here? I'm a bit on the fence if we should just drop the Handler suffix, but then again a type like Bolt11Payment would indicate that it represents a single payment, not a handler with which to send and receive such. Previously also had it as a plural, i.e., Bolt11PaymentsHandler, but that sounds strange in my head even though it seems to make more sense?

[jkczyz]

@jkczyz Any thoughts on the naming here? I'm a bit on the fence if we should just drop the Handler suffix, but then again a type like Bolt11Payment would indicate that it represents a single payment, not a handler with which to send and receive such.

How about Bolt11InvoicePayer? I guess that wouldn't work for receiving though.

Might be better to not be specific to protocol, FWIW. Rather just have a Payments struct used for sending any type of payment and for listing/retrieving/removing payments as well.

Previously also had it as a plural, i.e., Bolt11PaymentsHandler, but that sounds strange in my head even though it seems to make more sense?

My rule of thumb is to not use plurals in prefixes. Thus, EventHandler instead of EventsHandler. In LDK, I would have renamed EventsProvider as EventProvider even though the interface is named in terms of events.

[tnull]

How about Bolt11InvoicePayer? I guess that wouldn't work for receiving though.

Hmm, indeed that doesn't work for the receiving part.

Might be better to not be specific to protocol, FWIW. Rather just have a Payments struct used for sending any type of payment and for listing/retrieving/removing payments as well.

I'm not sure: the API surface of covering all possible payment variations is rather large already, we're about to add substantially to it with BOLT12, and we eventually will also need to add, e.g., BOLT12 JIT variations. I think we eventually also want to provide helpers to generate and pay more unified interface (e.g., unified QR code, DNS payment addresses), but I'm not seeing how we can avoid the API getting a bit unwieldy if all variations keep living on a single object?

My rule of thumb is to not use plurals in prefixes. Thus, EventHandler instead of EventsHandler. In LDK, I would have renamed EventsProvider as EventProvider even though the interface is named in terms of events.

Yeah, seems we're on the same page here.

[jkczyz]

I'm not sure: the API surface of covering all possible payment variations is rather large already, we're about to add substantially to it with BOLT12, and we eventually will also need to add, e.g., BOLT12 JIT variations. I think we eventually also want to provide helpers to generate and pay more unified interface (e.g., unified QR code, DNS payment addresses), but I'm not seeing how we can avoid the API getting a bit unwieldy if all variations keep living on a single object?

Maybe Bolt11Protocol? Though I'm a bit more inclined to use Bolt11Payment to align with the others. If you have the methods take self by value, then it really would be only one payment. Not sure how nice that works with bindings, however.

Another idea would be to divide by inbound and outbound instead of by protocol / mechanism.

[tnull]

Maybe Bolt11Protocol? Though I'm a bit more inclined to use Bolt11Payment to align with the others. If you have the methods take self by value, then it really would be only one payment. Not sure how nice that works with bindings, however.

You mean the Node::X_payment methods? No, for one we of course can't consume Node, but in bindings we can only return Arcs and never use self/&mut self. I'll however see to store/cache the corresponding Arcs in Node itself to reduce the number of clones on each call. I might also explore whether we can return a reference rather than an Arc in Rust, although I'm not entirely sure if that makes much of a difference.

[jkczyz]

You mean the Node::X_payment methods? No, for one we of course can't consume Node, but in bindings we can only return Arcs and never use self/&mut self. I'll however see to store/cache the corresponding Arcs in Node itself to reduce the number of clones on each call. I might also explore whether we can return a reference rather than an Arc in Rust, although I'm not entirely sure if that makes much of a difference.

Not the methods on Node but rather the methods on Bolt11Payment. But, right, I didn't notice that bolt11_payment returns an Arc. Why can't it return Bolt11Payment by value? Shouldn't need a reference if they are to be used per-payment.

[tnull]

Not the methods on Node but rather the methods on Bolt11Payment. But, right, I didn't notice that bolt11_payment returns an Arc. Why can't it return Bolt11Payment by value? Shouldn't need a reference if they are to be used per-payment.

So self or &mut self methods are generally impossible to expose in bindings as they require interior mutability. That said, we can return Bolt11PaymentHandler by value, I now added fixup commits that switch the interface to theArced versions based on the uniffi feature.

I also included fixups to rename the PaymentHandlers to Payments, even though I'm still a bit on the fence regarding naming. At least these variants are more succinct.

[tnull]

Rebased to resolve minor conflicts.

[tnull]

Now added two commits introducing a PaymentKind to PaymentStore and creating a separate kind for Bolt11Jit, which can hold the lsp_fee_limits as they are only required in this kind.

[tnull]

Rebased on main.

[jkczyz]

No major concerns left.

[jkczyz]

Should we name this PaymentPurpose for a closer mapping to LDK?

[tnull]

Hm, tbh, I find the 'Purpose' part a bit of a misnomer? And, given this is not actually the same object, shadowing the name might be misleading?

[jkczyz]

No strong opinion. Though I wonder if "kind" is just another way of saying "type". I'm a little more inclined with using the latter, but happy to defer to you here.

[tnull]

Ah, I think I also started out with PaymentType but then discovered that it would result in a shorthand type (e.g., as the field name in PaymentDetails where we omit the payment_ prefix), which of course doesn't work as type is a keyword. Rather than entering r#type territory, I just omitted the issue alltogether by switching to PaymentKind. So, generally I agree PaymentType seems a bit more natural, but that would mean we'd need to rename the PaymentDetails fields, and it's probably not worth?

[jkczyz]

LGTM. Please squash

[tnull]

LGTM. Please squash

Squashed without further changes.