Skip to content

Conversation

@bhandras
Copy link
Member

This PR adds ListChannels to LightningClient and extends RouterClient.SendPayment with keysend.

@bhandras bhandras force-pushed the lndclient_extension branch from 5b9b6be to e8ff2d0 Compare May 20, 2020 12:32
@bhandras bhandras changed the title extend LightningClient with ListChannels and RouterClient with keysend lndclient: extend LightningClient with ListChannels and RouterClient with keysend May 20, 2020
@bhandras bhandras marked this pull request as ready for review May 20, 2020 12:34
@bhandras bhandras requested review from carlaKC and joostjager May 20, 2020 12:34

// ListChannels retrieves all channels of the backing lnd node.
func (s *lightningClient) ListChannels(ctx context.Context) (
[]*lnrpc.Channel, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea of the wrapper is to not expose things like string based pubkeys. Ideally we'd create a strongly-typed struct to return a slice of. Can for now just contains the fields that we need.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

// to complete the full amount.
MaxParts uint32

// KeySend is set to true if the payment request will encode the preimage.
Copy link
Contributor

Choose a reason for hiding this comment

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

payment request generally refers to the hex-encoded invoice. You probably mean the tlv payload?

Copy link
Member Author

Choose a reason for hiding this comment

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

ptal

// Override the payment hash.
rpcReq.DestCustomRecords = make(map[uint64][]byte)
rpcReq.DestCustomRecords[record.KeySendType] = preimage[:]
request.PaymentHash = preimage.Hash()
Copy link
Contributor

Choose a reason for hiding this comment

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

Assert that PaymentHash wasn't set, to catch unintended use?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@bhandras bhandras force-pushed the lndclient_extension branch from e8ff2d0 to f9a8c72 Compare May 20, 2020 14:18
@bhandras bhandras requested a review from joostjager May 20, 2020 14:19

result := make([]ChannelInfo, len(response.Channels))
for i, channel := range response.Channels {
remoteVertex, err := route.NewVertexFromStr(channel.RemotePubkey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Future devs will thank you for the luxury of not having to parse everywhere :)

PubKeyBytes route.Vertex

// Capacity is the total amount of funds held in this channel.
Capacity int64
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be type btcutil.Amount, just as the balances below

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, that is much better

rpcReq.LastHopPubkey = request.LastHopPubkey[:]
}
if request.KeySend {
if !bytes.Equal(request.PaymentHash[:], emptyHash[:]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If payment hash is optional, it would be better to make it type *lntypes.Hash to avoid the magic value.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, much nicer to have the *lntypes.Hash, done

@bhandras bhandras force-pushed the lndclient_extension branch 2 times, most recently from de0c509 to 78c358c Compare May 20, 2020 14:49
@bhandras bhandras requested a review from joostjager May 20, 2020 14:52
@bhandras bhandras force-pushed the lndclient_extension branch from 78c358c to 1515c92 Compare May 20, 2020 14:53

// Override the payment hash.
rpcReq.DestCustomRecords = make(map[uint64][]byte)
rpcReq.DestCustomRecords[record.KeySendType] = preimage[:]
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative. Fewer characters but more lines :) Non-blocking ofc

rpcReq.DestCustomRecords = map[uint64][]byte {
   record.KeySendType: preimage[:],
}

Copy link
Member Author

Choose a reason for hiding this comment

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

done, probably compiles to the same code, but more readable :)

@bhandras bhandras force-pushed the lndclient_extension branch from 1515c92 to fee150c Compare May 20, 2020 17:43
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

LGTM!

ListTransactions(ctx context.Context) ([]*wire.MsgTx, error)

// ListChannels retrieves all channels of the backing lnd node.
ListChannels(ctx context.Context) ([]ChannelInfo, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

[]*ChannelInfo?

@bhandras bhandras force-pushed the lndclient_extension branch from fee150c to 620365b Compare May 21, 2020 07:37
@bhandras bhandras force-pushed the lndclient_extension branch from 620365b to cf9c374 Compare May 21, 2020 18:04
@bhandras bhandras merged commit bd2d39b into lightninglabs:master May 21, 2020
@bhandras bhandras deleted the lndclient_extension branch September 12, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants