ExchangeUnion / xud

Exchange Union Daemon 🔁 ⚡️
https://exchangeunion.com
GNU Affero General Public License v3.0
115 stars 49 forks source link

LND: switch to https://api.lightning.community/#sendpaymentv2 #1590

Closed kilrau closed 3 years ago

kilrau commented 4 years ago

EDIT: lndbtc & lndltc are upgraded to 0.11 and from that perspective we are ready to enable MPP. Question now is if to move to the streaming call sendpaymentv2 since MPP is currently not available in the sync version sendpaymentsync. TBD if it ever will be (see https://github.com/ExchangeUnion/xud/issues/1590#issuecomment-680199496).

sangaman commented 4 years ago

As I understand it for this issue we are waiting for 0.10.1 to be used for lnd in the xud-docker repo.

kilrau commented 4 years ago

And before that lndltc to be upgraded to that version too, yes.

cc @losh11

losh11 commented 4 years ago

lndltc 0.10.1 release is planned. currently have an alpha running on my computer, just trying to fix a few issues, then will release ASAP

kilrau commented 4 years ago

If you need help with testing -> push the branch and ping us :muscle:

rsercano commented 4 years ago

This API uses streaming instead of unary calls we already use, this's the unary call we use; https://api.lightning.community/#sendpaymentsync and there's no unary call for sendpaymentv2

Just to ensure, will we switch to stream calls? Note that since this's async, I'm not sure how to handle this.

kilrau commented 4 years ago

True, didn't see that. We ideally would want to stick with https://api.lightning.community/#sendpaymentsync. Can you check if it supports max_parts? https://api.lightning.community/#sendpaymentsync indicates it doesn't, but maybe the documentation is wrong.

rsercano commented 4 years ago

Unfortunately it doesn't as of today https://github.com/lightningnetwork/lnd/blob/fc12656a1a62e5d69430bba6e4feb8cfbaf21542/lnrpc/rpc.proto#L590

losh11 commented 4 years ago

sendpaymentv2 has no sync call, which is the only version which supports MPP.

kilrau commented 4 years ago

sendpaymentv2 has no sync call, which is the only version which supports MPP.

Is it realistic that MPP will be available in sendpaymentsync at some point or should we switch to sendpaymentv2? @roasbeef

Roasbeef commented 4 years ago

sendpaymentsync was only a workaround as we didn't yet support streaming responses over the REST interface. SendPaymentv2 will be the only endpoint that'll be maintained and extended from now on.

On Tue, Aug 25, 2020 at 11:35 AM Kilian Rausch | ⚡ OpenDEX ⚡ < notifications@github.com> wrote:

sendpaymentv2 has no sync call, which is the only version which supports MPP.

Is it realistic that MPP will be available in sendpaymentsync at some point or should we switch to sendpaymentv2? @Roasbeef https://github.com/Roasbeef

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ExchangeUnion/xud/issues/1590#issuecomment-680199496, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHTWLWBQ7M66ER4JWXJM3DSCP75TANCNFSM4NPU4UOA .

erkarl commented 4 years ago

sendpaymentsync was only a workaround as we didn't yet support streaming responses over the REST interface. SendPaymentv2 will be the only endpoint that'll be maintained and extended from now on. On Tue, Aug 25, 2020 at 11:35 AM Kilian Rausch | zap OpenDEX zap < @.***> wrote: sendpaymentv2 has no sync call, which is the only version which supports MPP. Is it realistic that MPP will be available in sendpaymentsync at some point or should we switch to sendpaymentv2? @Roasbeef https://github.com/Roasbeef — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1590 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHTWLWBQ7M66ER4JWXJM3DSCP75TANCNFSM4NPU4UOA .

@sangaman maybe it's time we decouple SwapClient.sendPayment from returning the preimage? I believe this will also simplify the logic for swaps and align it with how we're sending payments for Connext.

sangaman commented 4 years ago

maybe it's time we decouple SwapClient.sendPayment from returning the preimage? I believe this will also simplify the logic for swaps and align it with how we're sending payments for Connext.

We could certainly do that but it seems like it may still be easiest to keep it as is and handle the async nature of SendPaymentv2 within LndClient. From our perspective there aren't any intermediary steps we'd need to take between the payment being started and the payment succeeding/failing, unless perhaps we want to log information such as when the HTLC is ready, but even then we could do that within LndClient.