Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(service): add open channel support #1044

Merged
1 commit merged into from
Jul 10, 2019
Merged

feat(service): add open channel support #1044

1 commit merged into from
Jul 10, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jun 19, 2019 •
edited by ghost
Loading

Still WIP and not ready for in depth review. Putting this out here for early feedback.

TODO:

  • finish skipped tests for LndClient
  • add OpenChannel gRPC call
  • consume gRPC call via CLI xucli openchannel [nodePubKey] [amount] [currency]
  • manual lnd channel testing
  • share lnd pubKeys with peers
  • manual raiden channel testing

@ghost ghost self-assigned this Jun 19, 2019
@kilrau
Copy link
Contributor

kilrau commented Jun 22, 2019

If this is supposed to close #909, it also should include an RPC call to set lnd's autopilot to true/false. Can also be done in a separate PR.

@ghost ghost force-pushed the feat/channel-mgmt branch 4 times, most recently from 861f3a2 to a3a921a Compare June 23, 2019 14:11
@ghost ghost force-pushed the feat/channel-mgmt branch from a3a921a to a817c2f Compare July 1, 2019 09:24
@ghost ghost changed the title [wip] feat(service): add open channel support feat(service): add open channel support Jul 1, 2019
@ghost
Copy link
Author

ghost commented Jul 1, 2019

This is ready for review, but I suggest we deal with #1055 first because this is built on top of this. Afterwards I'll rebase this against master.

@ghost ghost force-pushed the feat/channel-mgmt branch 2 times, most recently from 2d7c29a to 888225a Compare July 2, 2019 12:40
sangaman
sangaman requested changes Jul 3, 2019
View reviewed changes
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed everything except the tests, made comments below. This will be a very handy feature, thanks!

lib/cli/commands/openchannel.ts Outdated Show resolved Hide resolved
lib/swaps/SwapClient.ts Outdated Show resolved Hide resolved
lib/lndclient/LndClient.ts
if (this.identityPubKey !== getInfoResponse.getIdentityPubkey()) {
newPubKey = getInfoResponse.getIdentityPubkey();
this.logger.debug(`pubkey is ${newPubKey}`);
this.identityPubKey = newPubKey;
newUris = getInfoResponse.getUrisList();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we'll only update newUris if the identity pub key changes. Maybe we should have a separate if statement that checks if the new uris list is different from the one we currently have and set newUris if so.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that newUris don't change at runtime so there's no need to add logic to compare array contents of newUris.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But they would change if lnd stops and starts while xud is running, right? In that case I'm thinking we'd want to update our lnd uris within xud.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be OK if I create an issue for it and deal with in a separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK with me.

lib/swaps/SwapClient.ts Outdated Show resolved Hide resolved
lib/lndclient/LndClient.ts Outdated Show resolved Hide resolved
lib/service/Service.ts Show resolved Hide resolved
proto/xudrpc.proto Outdated Show resolved Hide resolved
proto/xudrpc.proto Outdated Show resolved Hide resolved
proto/xudrpc.proto Outdated Show resolved Hide resolved
proto/xudrpc.proto Outdated Show resolved Hide resolved
@ghost ghost force-pushed the feat/channel-mgmt branch from 888225a to 131ec60 Compare July 5, 2019 15:25
@ghost
Copy link
Author

ghost commented Jul 5, 2019 •
edited by ghost
Loading

@sangaman
Thanks for the detailed review. I've applied most of the changes requested to the fixup commit.

Things left to do:

  • simplify tryConnectPeerAddresses loop/function
  • use map<string, LndUris> instead of map<string, string> on p2p level
  • inject logger to Service and log error when openchannel call fails
  • convert Service.openChannel amounts to correct units for the currency

@ghost ghost force-pushed the feat/channel-mgmt branch from dd8de2e to a014aab Compare July 9, 2019 10:03
@ghost ghost requested a review from sangaman July 9, 2019 13:44
sangaman
sangaman approved these changes Jul 10, 2019
View reviewed changes
Copy link
Collaborator

@sangaman sangaman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I don't think we should hold this up much longer. UnitConverter is a good first step towards handling raiden currency amounts without hardcoding too.

lib/lndclient/LndClient.ts
const { peerPubKey, address } = uri;
let timeout;
try {
timeout = setTimeout(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking this might be a good spot to use Promise.race - but after looking at this a bit more the grpc calls actually take an options parameter with a deadline field to specify the max time for the call to complete. Doesn't have to be in this PR, but it'd be nice to use that (and maybe reuse it elsewhere down the road). I can create a quick followup PR perhaps.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... perhaps. The benefit of having the timeout here would be a more clear error message.

I initially tried it with Promise.race, but I ran into some issues (can't remember the specifics).

lib/p2p/packets/utils.ts Outdated Show resolved Hide resolved
@ghost ghost force-pushed the feat/channel-mgmt branch from 521b984 to dab84af Compare July 10, 2019 08:06
@ghost ghost mentioned this pull request Jul 10, 2019
Open
@ghost ghost merged commit df694c1 into master Jul 10, 2019
@ghost ghost deleted the feat/channel-mgmt branch July 10, 2019 10:06
@kilrau kilrau mentioned this pull request Apr 10, 2020
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kilrau @sangaman