Split Payment struct depending on method

[hydra-yse]

Fixes #563.

[roeierez]

I am not sure we need this struct as what we really want to return from send_payment & send_spontaneous_payment is something that describes the lightning payment right?
So why not just return the LnPaymentDetails from these methods?
cc @JssDWt as the issue opener.

[JssDWt]

The main goal is to make a cleaner api, where the fields returned from an api call describe exactly what you did, and contain all the relevant information. The main issue with the send_payment and send_spontaneous_payment is the details field right now. That contains either ClosedChannelPaymentDetails or LnPaymentDetails. It would improve, by returning only LnPaymentDetails, but still it would contain many fields that you don't need, like the lnurl and swap fields. The reason for creating the issue, is to get rid of the ClosedChannelPaymentDetails in the response to a payment.

I do like the idea of having internal structs that represent exactly the information available at any point in the code. That reduces the amount of things you have to keep in mind when developing on this stuff. When you see a struct, you should know what information it contains and not have to scavenge through the codebase looking for potential other uses of the same struct. So I do like the idea of introducing a new struct for this case. LnPaymentDetails can be used as an aggregate. Either for persistence purposes or to include in the payment list. But ideally every subcall has its own response type.

Looking at the 'PendingPayment' struct, it still contains the PaymentDetails, with the LnPaymentDetails/ClosedChannelPaymentDetails. I would like this struct to represent a 'lightning payment' alone, without any wrappers for lnurl/swaps/closed channels whatsoever. So when there has been a lightning payment somewhere in the code, it's clear that at that point we're looking at a lightning payment, and nothing else.

Another comment about the PendingPayment struct is that the name implies it is pending. But it might not be pending. So we'd need to make clear whether the payment is pending or not.

As for the struct exposed to the client, maybe we can fill fields of the SendPaymentResponse itself, or indeed return the PendingPayment struct.

Generally it's a good idea in terms of code isolation to at least use different structs for

• persistence
• the api you expose
• the api you call
  That helps with being able to feely modify the code for specific purposes in different parts of the codebase.

(sorry about the long reply)

[hydra-yse]

Not at all @JssDWt, now I have a clearer idea of what you had in mind. I'll try to split everything as best as I can, so let the reviews continue!

[JssDWt]

Added some comments in the UDL as an addition to the comment I left above.

What I do really like about renaming Payment to PaymentListItem, it is immediately clear where the Payment struct was 'abused' before, in the sense that this specific struct is not the right one for every usecase.

[JssDWt]

An InvoicePaid event maybe shouldn't include A PaymentListItem, but only the data relevant to the paid invoice.

[JssDWt]

Maybe this can be LNPaymentDetails

[JssDWt]

This should probably contain the PendingPayment

[roeierez]

I think both here and in PaymentSucceed event we should contain the same struct/data.
@JssDWt @hydra-yse how about using the LNPaymentDetails here and there? This way we don't need to introduce another struct (PendingPayment).

[JssDWt]

Question: How would you feel about extending the PaymentDetails enum with variants for lnurl_pay, ln_address, lnurl_withdraw, swap and reverse swap? And make LnPaymentDetails basically the thing we mean with PendingPayment here?