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.

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

lncli: allow openchannel to accept a node's URI #1402

Conversation

jamesob
Copy link

@jamesob jamesob commented Jun 16, 2018

Many websites (recksplorer, lightninggem) advertise their node in terms of URI.
Allow lncli openchannel to accept a URI (pubkey@host:port) without throwing a
decode error, and use the hostname information to attempt a connection without
having to specify --connect.

This removes --connect as it is no longer needed.

Example usage

 $ lncli openchannel --sat_per_byte 13 02ad6fb8d693dc1e4569bcedefadf5f72a931ae027dc0f0c544b34c1c6f3b9a02b@167.99.50.31:9735 6000000 0

{
        "funding_txid": "d780dd9ff1adc76dfa2f32d05e22d2101c448296b8741262a398c2e3xxxxx"
}

@jamesob jamesob force-pushed the 2018-06-openchannel-accept-uri branch from 3db01ac to 3159ff5 Compare June 16, 2018 18:24
@jamesob jamesob changed the title Allow lncli openchannel to accept a node's URI lncli: allow openchannel to accept a node's URI Jun 16, 2018
@jamesob jamesob force-pushed the 2018-06-openchannel-accept-uri branch from 3159ff5 to bfcaf32 Compare June 18, 2018 20:06
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think a better approach would be to just provide either a node pubkey or a URI without the need for a connect flag. If we detect a URI, then we can attempt to connect to it before attempting to open the channel. Thoughts?

@jamesob
Copy link
Author

jamesob commented Jun 26, 2018

@wpaulino Thanks for the look. Removing --connect sounds fine with me and I'm happy to do so, but I'm wondering how sensitive lncli is to maintaining backwards compatibility. Do we want to first deprecate this flag and then remove it in a subsequent release, or are we okay with an unceremonious removal?

@wpaulino
Copy link
Contributor

I'm on board with removing it, it's a CLI flag so it would only impact those workflows. We usually recommend people to stay pinned to the latest tagged release if they're looking for stability anyway. Will leave the decision to the rest of the team.

@jamesob jamesob force-pushed the 2018-06-openchannel-accept-uri branch from bfcaf32 to 1250593 Compare June 27, 2018 14:13
@jamesob
Copy link
Author

jamesob commented Jun 27, 2018

I've pushed changes removing --connect (in lieu of URI use) as suggested by @wpaulino.

if err != nil {
return fmt.Errorf("unable to decode node public key: %v", err)
}
nodePubHex, connectVal, err = decodeNodePubkeyOrURI(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I believe req.NodePubkey can be used here directly.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think so given that the first arg (like node_key above) can either be a pubkey or a URI, and in the latter case we need to unpack it, but maybe I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

to clarify, what I meant was the following 😄 Not sure if it works?

	switch {
	case ctx.IsSet("node_key"):
		req.NodePubkey, connectVal, err = decodeNodePubkeyOrURI(
			ctx.String("node_key"))

	case args.Present():
		req.NodePubkey, connectVal, err = decodeNodePubkeyOrURI(
			args.First())
		args = args.Tail()
	default:
		return fmt.Errorf("node id argument missing")
	}

split := strings.Split(val, "@")

if len(split) > 1 {
nodeKey, connectVal = split[0], split[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like nodeKey is never set in the case where no URI is given?

Copy link
Author

Choose a reason for hiding this comment

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

D'oh! Good find. Willfix.

@halseth
Copy link
Contributor

halseth commented Jul 10, 2018

I'm in favour of removing --connect 👍

@Roasbeef Roasbeef added cli Related to the command line interface P3 might get fixed, nice to have needs testing PR hasn't yet been actively tested on testnet/mainnet first pass review done PR has had first pass of review, needs more tho labels Jul 11, 2018
Removes the `--connect` option.

Many websites (recksplorer, lightninggem) advertise their node in terms of URI.
Allow `openchannel` to accept a URI (pubkey@host:port) without throwing a
decode error, and use the hostname information to attempt a connection.
@jamesob jamesob force-pushed the 2018-06-openchannel-accept-uri branch from 1250593 to a8ff6ff Compare July 11, 2018 20:54
@jamesob
Copy link
Author

jamesob commented Jul 11, 2018

Pushed changes incorporating @halseth's feedback.

@jamesob
Copy link
Author

jamesob commented Jul 12, 2018

Tested with both URI and pubkey on mainnet:

$ lncli openchannel --sat_per_byte 40 024a2e265cd66066b78a788ae615acdc84b5b0dec9efac36d7ac87513015eaf6ed@lnd.bitrefill.com 9000000 0

{
        "funding_txid": "3fb66e74b8ece13eebf883d9d210adacc9390bcdd446c580bde59ffc80ab342d"
}

$ lncli openchannel --sat_per_byte 40 03e50492eab4107a773141bb419e107bda3de3d55652e6e1a41225f06a0bbf2d56 2000000 0

{
        "funding_txid": "eb7a1c1d7719131e1756b552564886355cbb83ae749afb7a98fc866cbc6608ed"
}

@halseth halseth added this to the 0.5.1 milestone Aug 28, 2018
@halseth halseth modified the milestones: 0.5.1, 0.5.2 Sep 20, 2018
@Roasbeef
Copy link
Member

Needs a rebase!

@Roasbeef Roasbeef removed this from the 0.5.2 milestone Jan 16, 2019
@Roasbeef Roasbeef closed this May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command line interface first pass review done PR has had first pass of review, needs more tho needs testing PR hasn't yet been actively tested on testnet/mainnet P3 might get fixed, nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants