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.

Sign up for GitHub

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

Jump to bottom

proto+lnrpc: update gRPC-REST gateway to v1.8.6 #3650

Merged
merged 2 commits into from
Nov 1, 2019
Merged

proto+lnrpc: update gRPC-REST gateway to v1.8.6 #3650

merged 2 commits into from
Nov 1, 2019

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Oct 30, 2019

Fixes #3168

When using the REST gateway, fields that have the type bytes in rpc.proto cannot be used in query or path parameters (see #552 which lead to workarounds like #592).

The root cause of this is that the version of grpc-gateway we use doesn't support mapping to []byte yet.
Trying to use bytes parameters results in {"error":"unsupported field type uint8","code":3}.
This has been fixed in grpc-ecosystem/grpc-gateway#489.

Unfortunately, we can't use the newest version of grpc-gateway (which would be 1.11.3 at the moment) because that has a bug that messes up our rpc.swagger.json (grpc-ecosystem/grpc-gateway#746).

After some trial and error, I found that version 1.8.6 is the newest version that works for our use case and produces the smallest diff in our generated code.

@guggero guggero mentioned this pull request Oct 30, 2019
Closed
wpaulino
wpaulino reviewed Oct 30, 2019
View reviewed changes
lnrpc/README.md Outdated
```bash
$ git clone https://github.com/grpc-ecosystem/grpc-gateway $GOPATH/src/github.com/grpc-ecosystem/grpc-gateway
$ cd $GOPATH/src/github.com/grpc-ecosystem/grpc-gateway
$ git reset --hard f2862b476edcef83412c7af8687c9cd8e4097c0f
$ git reset --hard v1.8.6
$ go mod init
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required? This version seems to support go modules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not required. That was left over from a trial with an older version. Fixed.

@joostjager
Copy link
Contributor

it would be nice if we can also mark all the string fields as deprecated now and remove them in 0.10

@guggero guggero added this to the 0.9.0 milestone Oct 31, 2019
@guggero guggero self-assigned this Oct 31, 2019
@guggero
Copy link
Collaborator Author

guggero commented Oct 31, 2019

@wpaulino no, he meant the fields introduced in #592 that worked around the bug that this PR fixes.

@guggero guggero removed this from the 0.9.0 milestone Oct 31, 2019
cfromknecht
cfromknecht approved these changes Oct 31, 2019
View reviewed changes
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.

nice, this actually reduces the size of our gateway code! LGTM 🔥

halseth
halseth approved these changes Nov 1, 2019
View reviewed 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.

Tested and LGTM 👍

joostjager
joostjager reviewed Nov 1, 2019
View reviewed changes
Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

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

it would be nice if we can also mark all the string fields as deprecated now and remove them in 0.10

Do we want to do this? I think it would be nice to be able to remove the string fields at some point

@halseth
Copy link
Contributor

halseth commented Nov 1, 2019

it would be nice if we can also mark all the string fields as deprecated now and remove them in 0.10

Do we want to do this? I think it would be nice to be able to remove the string fields at some point

Yep, can be done in separate PR though.

@halseth halseth merged commit e0744a3 into lightningnetwork:master Nov 1, 2019
@guggero guggero deleted the update-grpc-gateway branch November 1, 2019 08:06
@guggero guggero mentioned this pull request Nov 1, 2019
Merged
@joostjager
Copy link
Contributor

I am getting a new error:

joost@joost-desktop $ make rpc
 Compiling protos.
cd ./lnrpc; ./gen_protos.sh
Generating root gRPC server protos
F1102 13:10:55.891383    8655 main.go:73] Cannot set flag paths=source_relative
--grpc-gateway_out: protoc-gen-grpc-gateway: Plugin failed with status code 255.

@joostjager
Copy link
Contributor

Nvm, needed to update dep

@guggero guggero mentioned this pull request May 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joostjager joostjager joostjager left review comments

@cfromknecht cfromknecht cfromknecht approved these changes

@halseth halseth halseth approved these changes

@wpaulino wpaulino wpaulino approved these changes

@Roasbeef Roasbeef Awaiting requested review from Roasbeef

Assignees

@guggero guggero

Labels
bug fix REST
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wallet recovery with passphrase is not working
5 participants
@guggero @joostjager @halseth @cfromknecht @wpaulino