feat(rpc/connext): deposit & openchannel calls

[sangaman]

Closes #1472. Closes #1473.

This refactors the Deposit and OpenChannel methods in SwapClient to make them generic and applicable to Connext. It implements this functionality for Connext, namely retrieiving an address for sending funds on-chain to the Connext wallet and depositing those funds into Connext channels.

It also refactors the CloseChannel call similarly, however the Connext REST client does not currently implement a /withdraw endpoint for taking funds out of Connext channels. This adds dummy code to construct a withdraw payload and make a request. Edit: withdraw endpoint is being added here connext/rest-api-client#29

Withdrawing coins from the on-chain Connext wallet is not currently supported by the Connext client and so the Withdraw rpc call remains specific to lnd.

I didn't rename the deposit and withdraw cli calls since they won't conflict with the current approach to a separate tool for the deposit/withdrawal swaps described in #1520. I can still rename those if we'd like, I just wasn't sure whether it still made sense.

Also it does look like it's possible to make calls directly to ethProvider from the connext rest-api-client, that would allow us to send on-chain coins directly from the connext wallet (rather than a connext channel), so that's a possibility if we ever need that functionality. Edit: this is being implemented by connext/rest-api-client#20 so after it's merged we can integrate it to xud.

I still need to do more testing with this PR but it's ready for review.

[kilrau]

I didn't rename the deposit and withdraw cli calls since they won't conflict with the current approach to a separate tool for the deposit/withdrawal swaps described in #1520. I can still rename those if we'd like, I just wasn't sure whether it still made sense

Thanks for bringing this up, I would still rename the existing lnd wallet deposit/withdraw -> walletdeposit/walletwithdraw simply because we have two different ways to get from an external on-chain wallet to funds in an active channel and I believe one has to be renamed if we want to keep both (which we want):

1. deposit: btc/ltc: boltz submarine/channel creation swap, connext: direct deposit into indra channel (needs External direct deposits v2 connext/indra#1166). Default option.
2. walletdeposit: throw an address from the local wallet, send on-chain from local wallet, then manually call openchannel once funds are confirmed. Manual re-balancing for inbound liquidity needed on btc/ltc. Manual option.

As discussed today, these two currently unimplemented calls need to be added to https://github.com/ExchangeUnion/rest-api-client to complete above:

• eth/token withdrawal
• indra channel withdrawal

[sangaman]

Thanks for bringing this up, I would still rename the existing lnd wallet deposit/withdraw -> walletdeposit/walletwithdraw simply because we have two different ways to get from an external on-chain wallet to funds in an active channel and I believe one has to be renamed if we want to keep both (which we want):

The boltz submarine channel creation swap won't be happening through xucli though, right? So it wouldn't conflict with the current deposit/withdraw naming scheme, unless I'm misunderstanding something.

[kilrau]

Correct, technically not through xucli, but through the xud-docker cli which is xucli + a bunch of other commands. There, the user should be able to type deposit btc to trigger a boltz submarine swap. I can't think of a good way how this could coexist with the current deposit (which calls lnd's newaddress) apart from renaming the latter.

A clear mental model would be:

1. Type deposit -> deposit into channel, default
2. Type walletdeposit -> deposit into wallet

Of course we could leave xucli untouched and just wrap the current xucli deposit in walletdeposit in the xud-docker cli, just that this leaves deposit in xud-docker doing something different than deposit in xud native. But if you feel strongly about this, that is fine with me too.

[raladev]

Testing results:

• closechannel does not work for connext. 1. because of peer is required 2. because of BigInt error
  Screen
• deposit returns empty address for any currency (not only for connext)
  Screen
• openchannel to amount bigger then exists in wallet = connext server error
  Screen
  Question: is it correct that there is no apportunity to open Connext channel to not our Connext node?

[kilrau]

To answer your question: yes, that is correct

[sangaman]

Correct, technically not through xucli, but though the xud-docker cli which is xucli + a bunch of other commands. There, the user should be able to type deposit btc to trigger a boltz submarine swap. I can't think of a good way how this could coexist with the current deposit (which calls lnd's newaddress) apart from renaming the latter.

A clear mental model would be:

1. Type `deposit` -> deposit into channel, default

2. Type `walletdeposit` -> deposit into wallet

Of course we could leave xucli untouched and just wrap the current xucli deposit in walletdeposit in the xud-docker cli, just that this leaves deposit in xud-docker doing something different than deposit in xud native. But if you feel strongly about this, that is fine with me too.

Makes sense to me, I'll rename it.

[kilrau]

6.6.2 (ExchangeUnion/xud-docker#478) contains connext/rest-api-client#29 - so connext on-chain deposit and withdrawal should be testable with it @raladev

[raladev]

#1577 (comment)

[sangaman]

Thanks a lot for testing these.

Testing results:

* [ ]  closechannel does not work for connext. 1. because of peer is required 2. because of BigInt error

Should be fixed now.

* [ ]  deposit returns empty address for any currency (not only for connext)

Also fixed, I confirmed this.

* [ ]  openchannel to amount bigger then exists in wallet = connext server error

This one I'm still getting, and in fact I'm getting it even when I try to query my balance. I think this is likely an underlying issue with connext that we'd need to address via modifying the connext client, not something I can address in this PR.

[raladev]

• deposit still is not working, but now because of syntax error; @sangaman
  Screen

• Note/question about closechannel command. Daniel solves 'peer is required' problem by moving peer to optional param, but now command 'closechannel BTC' (without peer) does nothing and returns 'success'. How it should work for now? @kilrau @sangaman
  Screen

I see 2 solutions:

1. Close all btc channels for all peers
2. throw error 'peer is required for lnd channles'
   First is more preferable.

• closechannel command stuck for connext; tested with connext 6.6.2; @erkarl

1. closechannel ETH -> connext logs contains

Executing with params: {"multisigAddress":"0xe37A..d684","assetId":"0x0000..0000"}
[1590662664150] INFO  (71 on connext): received request
    reqId: 24549
    url: "/withdraw"
    id: 24549
2020-05-28T10:44:24.152Z [WithdrawalController] withdraw started: {
  "recipient": "",
  "amount": "900000000"
}

2. socket hang up after ~10 sec
3. also it tooks closing commission but there is no closing -_-

Screens:
Screen
Screen

Logs:
close_lnd.log
close_connext.log
close_xud.log

Note:
I can create separate issue if it is required, but it seems as blocker for that issue, so I just wrote about that here.

• feat(rpc/connext): deposit & openchannel calls #1577 (comment) also last bug in this comment as a bonus @erkarl;

[sangaman]

* [ ]  deposit still is not working, but now because of syntax error; @sangaman

Sorry, I should've mentioned I went ahead with kilian's suggestion and renamed to walletdeposit and walletwithdraw. Can you retry with walletdeposit?

* [ ]  Note/question about closechannel command. Daniel solves 'peer is required' problem by moving peer to optional param, but now command 'closechannel BTC' (without peer) does nothing and returns 'success'. How it should work for now? @kilrau @sangaman
  [Screen](https://user-images.githubusercontent.com/29906866/83134981-d76a9080-a0ed-11ea-9858-a855c1835a39.png)

I see 2 solutions:

1. Close all btc channels for all peers

2. throw error 'peer is required for lnd channles'
   First is more preferable.

Thanks for catching this, I would think we'd actually want to stick with the second option, since that is the current behavior, plus it would be unfortunate if you forgot to type in a peer and accidentally closed all your BTC channels! So I can make sure the call returns an error if we expect a peer but don't get one. What do you think @kilrau?

* [ ]  closechannel command stuck for connext; tested with connext 6.6.2; @erkarl

I think this one is probably connext implementation specific, rather than a problem with xud's API or cli, so let's follow up on this separately after this is merged.

[kilrau]

Thanks for catching this, I would think we'd actually want to stick with the second option, since that is the current behavior, plus it would be unfortunate if you forgot to type in a peer and accidentally closed all your BTC channels! So I can make sure the call returns an error if we expect a peer but don't get one. What do you think @kilrau?

Agreed!

[raladev]

• walletdeposit/walletwithraw works
• to do: throw error for closechannel 'lndcurrency'
• one more bug. Incorrect convertation of ETH to satoshiETH between xucli and withdraw call.
  withdraw --amount 2 -> only 0.00000002 withdraw instead of 2 ETH.
  Screenshot
• note:strange balance behavior. max amount increased after first withdrawal operation.(can be because of incorrect precision of eth in getbalance - should be 18 instead of 8)

[sangaman]

* walletdeposit/walletwithraw works

* [ ]  to do: throw error for closechannel 'lndcurrency'

* [ ]  one more bug. Incorrect convertation of ETH to  satoshiETH between xucli and withdraw call.
  withdraw --amount 2 -> only 0.00000002 withdraw instead of 2 ETH.
  [Screenshot](https://user-images.githubusercontent.com/29906866/83265648-72389d00-a1ca-11ea-8d4f-40e953a080b5.png)

* [ ]  note:strange balance  behavior. max amount increased after first withdrawal operation.(can be because of incorrect precision of eth in getbalance - should be 18 instead of 8)

I fixed the first two issues (closechannel was also missing the coins to satoshis conversion for the amount option), but I'm not sure I understand the last one. Are you able to reproduce or give some more detail? Thanks.

[raladev]

• first issue fixed fully;
• second fixed partially, There is still issue with convertation when use closechaanel without params
  Screen

• additional description and logs for third:
  Screen 1
  Screen 2

Total balance before first withdrawal = 29.99979
Total balance after first withdrawal = 29.99978999
Total balance after all next withdrawals = 29.99978999

Question: who took my coins after first withdrawal?)

digits_xud.log
digits_utils.log
digits_connext.log

Note: i still think that it is because of 8 digits)

[sangaman]

@raladev Thanks for the additional details, I fixed the issue with the wrong units being used when no amount is specified when closing a Connext channel.

For the issue with the balances changing, I don't really have an explanation for that yet. I think though that it might be helpful and would make sense to return precise, complete values for the balances. I'll open an issue for that.

[kilrau]

As agreed @erkarl to skim over the code changes, other than that this PR is well tested by @raladev