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

rpc: add node color to NodeUpdate and GetInfo #2312

Merged
merged 4 commits into from
May 23, 2019
Merged

rpc: add node color to NodeUpdate and GetInfo #2312

merged 4 commits into from
May 23, 2019

Conversation

xsb
Copy link
Contributor

@xsb xsb commented Dec 10, 2018

This adds the node color to both NodeUpdate (as part of aGraphTopologyUpdate) and to GetInfo response. Tested with a Go gRPC client.

Fixes #2255 and #2268

@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour gRPC needs review PR needs review by regular contributors needs testing PR hasn't yet been actively tested on testnet/mainnet labels Dec 11, 2018
channeldb/graph.go Outdated Show resolved Hide resolved
@@ -312,6 +316,7 @@ func addToTopologyChange(graph *channeldb.ChannelGraph, update *TopologyChange,
Addresses: m.Addresses,
IdentityKey: pubKey,
Alias: m.Alias,
Color: encodeHexColor(m.Color),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is needed here, really. If you want to inspect the color you can use the GetNodeInfo RPC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to implement #2255

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! Ideally, the encode method would be shared, and not duplicated across the two packages though...

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Ideally, independent changes for different packages should be in separate commits. In this case I would suggest lnrpc, server+server_test, rpcserver and finally lncli commits, in that order.

server.go Outdated
@@ -2842,6 +2841,11 @@ func parseHexColor(colorStr string) (color.RGBA, error) {
return color.RGBA{R: colorBytes[0], G: colorBytes[1], B: colorBytes[2]}, nil
}

// encodeHexColor takes a color and returns it in hex code format.
func encodeHexColor(color color.RGBA) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

with this addition, the server_test.go should be expended to test this!

@Roasbeef Roasbeef added this to In progress in High Priority via automation Dec 18, 2018
@Roasbeef Roasbeef moved this from In progress to Needs review in High Priority Dec 18, 2018
@xsb
Copy link
Contributor Author

xsb commented Dec 21, 2018

@halseth I made it with a separate commit per package as you suggested and also added a test. Something I am not convinced though is exposing the utility method EncodeHexColor from the routing package but I wasn't sure where to put it. Seems weird to me, I am open to changes.

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Yeah, not obvious where to put it, but I think routing is alright for now. LGTM 👍

@xsb
Copy link
Contributor Author

xsb commented Mar 15, 2019

Rebased

@halseth halseth added the P3 might get fixed, nice to have label Mar 19, 2019
@cfromknecht cfromknecht added this to the 0.7 milestone Apr 11, 2019
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Needs a rebase and proto regen

@xsb
Copy link
Contributor Author

xsb commented May 10, 2019

@halseth rebased with latest master + proto files regenerated

@cfromknecht
Copy link
Contributor

@xsb looks like we have another conflict 😶

@xsb
Copy link
Contributor Author

xsb commented May 16, 2019

@halseth @cfromknecht Rebased again.

Note: While regenerating the proto files I got a different result for lnrpc/invoicesrpc/invoices.pb.go that I ignored it for the sake of cleanliness. It seems to come from the recently merged #3019. Either the author of that PR or myself are not running the correct protoc, etc. Probably not super important but better to clarify before merging this.

Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

@xsb LGTM! 🎉 Thank you for pointing out the versioning mismatch in the protos, I've created an issue to avoid this in the future, see #3094

@cfromknecht cfromknecht requested a review from halseth May 22, 2019 05:31
@halseth
Copy link
Contributor

halseth commented May 22, 2019

Yet another proto conflict 🤕

Note: While regenerating the proto files I got a different result for lnrpc/invoicesrpc/invoices.pb.go that I ignored it for the sake of cleanliness.

If you still see this after rebasing on master, check your proto+tools version. I just tried rebasing this on master, and regenerating the protos doesn't create any changes to that file for me.

@xsb
Copy link
Contributor Author

xsb commented May 22, 2019

@halseth rebased again. No issue regenerating protos this time (tried on master and on my branch).

@cfromknecht
Copy link
Contributor

sorry @xsb, there was a conflict w/ #3082 :/ you'll need to rebase on master and ensure that your protogen version is on 0.1.3 (step 2 of https://github.com/lightningnetwork/lnd/tree/master/lnrpc#generate-protobuf-definitions)

Feel free to ping me when you're ready, hoping to minimize the number of rebases on your end 😬

@xsb
Copy link
Contributor Author

xsb commented May 23, 2019

@cfromknecht rebased again after upgrading to new protobuf version

High Priority automation moved this from Needs review to Final Testing -- Ready For Merge May 23, 2019
@halseth halseth merged commit 6cd71b7 into lightningnetwork:master May 23, 2019
High Priority automation moved this from Final Testing -- Ready For Merge to Done May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing features / behaviour gRPC needs review PR needs review by regular contributors needs testing PR hasn't yet been actively tested on testnet/mainnet P3 might get fixed, nice to have
Projects
No open projects
High Priority
  
Done
Development

Successfully merging this pull request may close these issues.

Return node color changes in subscribe channel graph graph topology updates
4 participants