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

Use hex encoding for raw bytes in REST API #286

Merged
merged 3 commits into from
May 23, 2023
Merged

Use hex encoding for raw bytes in REST API #286

merged 3 commits into from
May 23, 2023

Conversation

guggero
Copy link
Member

@guggero guggero commented May 19, 2023

Fixes #282.

By using our fork of google.golang.org/protobuf that encodes raw bytes
as hex for the REST proxy, this automatically turns all raw byte gRPC
fields into hex encoded strings in the REST API.
@guggero guggero requested a review from Roasbeef May 19, 2023 12:26
// should be used when interacting with the REST proxy only.
RESTJsonUnmarshalOpts = &protojson.UnmarshalOptions{
AllowPartial: false,
UseHexForBytes: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

q: I guess it's to late to change this in LND too because other tools are using already base64 encoding?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about adding an option to give the user the choice. But the problem is that yes, most of the apps using the REST interface now rely on the raw bytes being base64 encoded. So it shouldn't be the user's choice but rather the developer's. This gives me an idea though: Maybe there is a way to specify the desired encoding as a HTTP header field? For example: Accept: application/json+hex instead of just Accept: application/json...


// We want to format raw bytes as hex instead of base64. The forked version
// allows us to specify that as an option.
replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-display v1.30.0-hex-display
Copy link
Member

Choose a reason for hiding this comment

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

Why not just import it directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we need to keep it as the original package name, if I import it directly I get this error:

        module declares its path as: google.golang.org/protobuf
                but was required as: github.com/lightninglabs/protobuf-go-hex-display

And renaming the package in the fork would require a way bigger diff than just protocolbuffers/protobuf-go@9c1f7c5 as we'd need to refactor a bunch of usages of the internal package as well. So rebasing the hex render feature on top of a new version in the future would become a bigger task, potentially causing us to lag behind on the versions of this package.

@guggero guggero requested a review from Roasbeef May 23, 2023 11:12
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🔮

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🔮

@Roasbeef Roasbeef added this pull request to the merge queue May 23, 2023
Merged via the queue into main with commit 4f22232 May 23, 2023
11 checks passed
guggero added a commit that referenced this pull request May 23, 2023
This is a follow-up to #286 which allowed developers to specify any raw
bytes as a hex string in the REST API (instead of the library default of
base64).
The only area where that change didn't apply is the URI part of REST
calls. For those we need to use a string field to support hex encoding,
the same way we already did for the universe REST API.
guggero added a commit that referenced this pull request May 23, 2023
This is a follow-up to #286 which allowed developers to specify any raw
bytes as a hex string in the REST API (instead of the library default of
base64).
The only area where that change didn't apply is the URI part of REST
calls. For those we need to use a string field to support hex encoding,
the same way we already did for the universe REST API.
@guggero guggero deleted the rest-hex branch May 26, 2023 10:43
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.

rpc: use hex encoding for all REST RPC outputs
3 participants