-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(rpc/connext): deposit & openchannel calls #1577
Conversation
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):
As discussed today, these two currently unimplemented calls need to be added to https://github.com/ExchangeUnion/rest-api-client to complete above:
|
The boltz submarine channel creation swap won't be happening through xucli though, right? So it wouldn't conflict with the current |
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 A clear mental model would be:
Of course we could leave xucli untouched and just wrap the current |
Testing results:
|
To answer your question: yes, that is correct |
Makes sense to me, I'll rename it. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edad1a8
to
9d360b5
Compare
Thanks a lot for testing these.
Should be fixed now.
Also fixed, I confirmed this.
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. |
I see 2 solutions:
Logs: Note:
|
Sorry, I should've mentioned I went ahead with kilian's suggestion and renamed to
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?
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. |
9d360b5
to
800b4fb
Compare
Agreed! |
|
800b4fb
to
173cb90
Compare
I fixed the first two issues (closechannel was also missing the coins to satoshis conversion for the |
d85cf87
to
6c33478
Compare
Total balance before first withdrawal = 29.99979 Question: who took my coins after first withdrawal?) digits_xud.log Note: i still think that it is because of 8 digits) |
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. 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. Closes #1472. Closes #1473.
6c33478
to
cdc8259
Compare
@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. |
As agreed @erkarl to skim over the code changes, other than that this PR is well tested by @raladev |
Closes #1472. Closes #1473.
This refactors the
Deposit
andOpenChannel
methods inSwapClient
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#29Withdrawing 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
andwithdraw
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 toEdit: this is being implemented by connext/rest-api-client#20 so after it's merged we can integrate it to xud.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.I still need to do more testing with this PR but it's ready for review.