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

build+lncli: default to hex encoding for byte slices #3647

Merged
merged 2 commits into from Dec 21, 2019

Conversation

@cfromknecht
Copy link
Collaborator

cfromknecht commented Oct 29, 2019

This commit swaps out golang/protobuf/jsonpb for a custom variant https://github.com/lightninglabs/protobuf-hex-display that
by default prints byte slices as hex, which is more useful for our
setting. Some existing wrapper structs are removed as they can now be
printed directly with the new jsonpb.

!!! NOTE !!!

This commit introduces a breaking change to lncli listinvoices since
payment hashes and preimages will now be printed in hex instead of
base64.

@cfromknecht cfromknecht force-pushed the cfromknecht:hex-jsonpb branch from 98638bd to 3998efc Oct 29, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Oct 29, 2019

I am not worried about the breaking change to lncli. We try to keep our grpc api reasonably stable, but I think the cli is intended for use by humans and it shouldn't be a problem to change it without preceding deprecation.

Aren't there more display structs that can now be removed?

@cfromknecht cfromknecht force-pushed the cfromknecht:hex-jsonpb branch from 3998efc to dd320a2 Oct 29, 2019
@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

cfromknecht commented Oct 29, 2019

I am not worried about the breaking change to lncli. We try to keep our grpc api reasonably stable, but I think the cli is intended for use by humans and it shouldn't be a problem to change it without preceding deprecation.

I have seen people use lncli in production-ish settings, so we should still try to announce as widely as possible to avoid breaking. I agree tho we can probably proceed faster given that it is mostly used as a dev tool.

Aren't there more display structs that can now be removed?

Indeed, I found many more :)

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Nov 7, 2019

Users can stay on the old lncli if they need the old format.

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Dec 13, 2019

Needs update and rebase.

@cfromknecht cfromknecht force-pushed the cfromknecht:hex-jsonpb branch from ce03d1e to 09b5f00 Dec 14, 2019
@cfromknecht cfromknecht requested review from joostjager and halseth Dec 14, 2019
@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

cfromknecht commented Dec 14, 2019

rebased

Copy link
Collaborator

joostjager left a comment

Seems that there is one hex encode left in commands.go lline 3822
ChanBackup: hex.EncodeToString(..)

Open question for me it still how to deal with other custom display logic, for example for channel point. Are we going to add that to our jsonpb fork somehow too?

@@ -88,7 +88,7 @@ func buildRoute(ctx *cli.Context) error {
return err
}

printJSON(route)

This comment has been minimized.

Copy link
@halseth

halseth Dec 17, 2019

Collaborator

can remove the printJSON method now?

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Dec 20, 2019

Author Collaborator

no there are still a few others that use it, mostly for things that encode outpoints

@halseth

This comment has been minimized.

Copy link
Collaborator

halseth commented Dec 17, 2019

Since it's a breaking change, would perhaps be nice to get in before 0.9?

@cfromknecht cfromknecht added the v0.9.0 label Dec 17, 2019
@cfromknecht cfromknecht added this to the 0.9.0 milestone Dec 17, 2019
@cfromknecht cfromknecht added this to WIP in v0.9.0-beta via automation Dec 17, 2019
@cfromknecht cfromknecht moved this from WIP to Needs Review in v0.9.0-beta Dec 17, 2019
@cfromknecht cfromknecht force-pushed the cfromknecht:hex-jsonpb branch 2 times, most recently from f97a611 to f8bb680 Dec 19, 2019
@cfromknecht cfromknecht requested a review from halseth Dec 20, 2019
@cfromknecht cfromknecht requested a review from joostjager Dec 20, 2019
@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

cfromknecht commented Dec 20, 2019

@joostjager @halseth added a commit to reorder getinfo via the proto directly, so we no longer need the display struct. lmk what you think

Copy link
Collaborator

joostjager left a comment

As discussed offline, convert last hex conversion on ChanBackup to []byte.

cfromknecht added 2 commits Dec 20, 2019
This commit swaps out golang/protobuf/jsonpb for a custom variant that
by default prints byte slices as hex, which is more useful for our
setting. Some existing wrapper structs are removed as they can now be
printed directly with the new jsonpb.

!!! NOTE !!!

This commit introduces a breaking change to lncli listinvoices since
payment hashes and preimages will now be printed in hex instead of
base64.
@cfromknecht cfromknecht force-pushed the cfromknecht:hex-jsonpb branch from f8bb680 to 153cd58 Dec 20, 2019
@cfromknecht cfromknecht requested a review from joostjager Dec 20, 2019
Copy link
Collaborator

joostjager left a comment

Still have the feeling that this isn't the ultimate solution as we can't specify custom encoding for other types like channel points. But hopefully we will find a good way for that in the future. Maybe by customizing json pb further with some serialization function map.

v0.9.0-beta automation moved this from Needs Review to Approved Dec 20, 2019
@cfromknecht cfromknecht merged commit 72a49d4 into lightningnetwork:master Dec 21, 2019
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.02%) to 62.145%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
v0.9.0-beta automation moved this from Approved to Done Dec 21, 2019
@cfromknecht cfromknecht deleted the cfromknecht:hex-jsonpb branch Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.