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

Make gRPC requests go through tendermint Query #8549

Merged
merged 19 commits into from
Feb 15, 2021
Merged

Make gRPC requests go through tendermint Query #8549

Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
e0e2242
Make gRPC requests go through tendermint Query
amaury1093 Feb 9, 2021
1f5cdd1
Remove commented code
amaury1093 Feb 9, 2021
db1ccb9
Dry run in InitChain?
amaury1093 Feb 9, 2021
af70bf7
Save type of first run
amaury1093 Feb 10, 2021
66b8fb1
Add metadata in repsonse
amaury1093 Feb 10, 2021
437ada0
Factorize some code
amaury1093 Feb 10, 2021
7c44ee5
Fix lint
amaury1093 Feb 10, 2021
38775aa
Update comments
amaury1093 Feb 10, 2021
e274572
Merge branch 'master' into am-grpc-tm
amaury1093 Feb 10, 2021
40afd23
Fix md test
amaury1093 Feb 10, 2021
97178dc
Fix test expected
amaury1093 Feb 10, 2021
2c5a017
Don't put RunGRPCQuery as clientCtx method
amaury1093 Feb 11, 2021
2cc56a0
Merge branch 'master' into am-grpc-tm
amaury1093 Feb 11, 2021
6f24859
Merge branch 'master' into am-grpc-tm
amaury1093 Feb 12, 2021
31ac78a
Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-g…
amaury1093 Feb 15, 2021
60760d1
Update baseapp/grpcserver.go
amaury1093 Feb 15, 2021
ba06cd4
Merge branch 'am-grpc-tm' of ssh://github.com/cosmos/cosmos-sdk into …
amaury1093 Feb 15, 2021
de3c822
Address review comments
amaury1093 Feb 15, 2021
2405701
Merge branch 'master' into am-grpc-tm
amaury1093 Feb 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update baseapp/grpcserver.go 
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
  • Loading branch information
@amaury1093 @robert-zaremba
amaury1093 and robert-zaremba committed Feb 15, 2021
commit 60760d118c05531a35fb206fe78a7ed5863f6c80
2 changes: 1 addition & 1 deletion baseapp/grpcserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func (app *BaseApp) RegisterGRPCServer(clientCtx client.Context, server gogogrpc
// for decoding.
returnType, found := app.GRPCQueryRouter().returnTypes[info.FullMethod]
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

@robert-zaremba robert-zaremba Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of using reflections, I would propose something else.

All return types are proto.Message

Instead of having a map:

methodName -> reflect.Type

we can use factory functions:

methodName -> func() proto.Message

I'm not sure where is the best place to create that functions - probably each module will need to do it itself when registering interfaces? So a bit more work, and more verbose. Not for this task, but idea for the future. If you think this make sense then we can create an issue for this.

Copy link
Contributor Author

@amaury1093 amaury1093 Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

we could do that already, but still using reflection:

- qrt.returnTypes[fqName] = reflect.TypeOf(res)
+ qrt.returnTypes[fqName] = func() proto.Message {
+   typ := reflect.TypeOf(res)
+   return reflect.New(typ.Elem()).Interface().(proto.Message)
+ }

Both are equivalent in my eyes, I don't have a preference. happy to switch to factory functions (with reflection inside) if you find them clearer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking more about registering this methods in the modules when registering the query server, rather than baseapp. So that would be before even running the query server. It's more declarative - so a bit more code, but we won't use reflection.

Copy link
Collaborator

Choose a reason for hiding this comment

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

shall we create a task to discuss it further?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I created one: #8588

if !found {
return nil, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "cannot find %s return type", info.FullMethod)
return nil, sdkerrors.Wrapf(sdkerrors.ErrLogic, "cannot find %s return type", info.FullMethod)
}

// returnType is a pointer to a struct. Here, we're creating res which
Expand Down