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

gRPC gateway init #7019

Merged
merged 49 commits into from
Aug 25, 2020
Merged

gRPC gateway init #7019

merged 49 commits into from
Aug 25, 2020

Conversation

atheeshp
Copy link
Contributor

ref: #5921

Description

closes: #XXXX

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@atheeshp
WIP: grpc server setup
204f43a
@atheeshp
add register grpc routes
fdaac0b
@atheeshp
updated go mod
d5fadc9
@atheeshp
Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/59…
83b38a6 
…21-grpc-gateway-register
@atheeshp
updated grpc for all modules
9d23a1f
@atheeshp
Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/59…
e8ca8ea 
…21-grpc-gateway-register
@atheeshp
Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/59…
c443a46 
…21-grpc-gateway-register
@atheeshp
added pb file for grpc gateway
f51b946
@atheeshp
udpated gw file
4de3b1c
@atheeshp
added a test for grpc route
138b84b
@atheeshp
Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/59…
1c7cd87 
…21-grpc-gateway-register
@atheeshp
Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/59…
1dec763 
…21-grpc-gateway-register
@atheeshp
fixed conflicts
351a2c1
@codecov
Copy link

codecov bot commented Aug 14, 2020 •
edited
Loading

Codecov Report

Merging #7019 into master will decrease coverage by 0.28%.
The diff coverage is 4.76%.

@@            Coverage Diff             @@
##           master    #7019      +/-   ##
==========================================
- Coverage   54.82%   54.53%   -0.29%     
==========================================
  Files         561      566       +5     
  Lines       38327    38728     +401     
==========================================
+ Hits        21012    21120     +108     
- Misses      15604    15884     +280     
- Partials     1711     1724      +13

@atheeshp atheeshp changed the title Adds grpc gateway register gRPC gateway init Aug 20, 2020
@anilcse
Copy link
Collaborator

anilcse commented Aug 24, 2020

Thanks! Excited about this PR! Are the grpc routes going to be registered in a follow-up PR?

Thanks for a quick review @fedekunze . I've addressed all your comments. Yes, I will create a follow-up PR for integrating all other modules.

@anilcse anilcse requested a review from fedekunze August 24, 2020 21:41
@aaronc aaronc mentioned this pull request Aug 24, 2020
Closed
43 tasks
@clevinson clevinson mentioned this pull request Aug 24, 2020
Closed
25 tasks
fedekunze
fedekunze approved these changes Aug 25, 2020
View reviewed changes
x/capability/module.go Outdated Show resolved Hide resolved
@fedekunze fedekunze added A:automerge Automatically merge PR once all prerequisites pass. C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. labels Aug 25, 2020
amaury1093
amaury1093 suggested changes Aug 25, 2020
View reviewed changes
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Also excited about this PR, thanks @anilcse and @atheeshp!

server/api/server.go Outdated Show resolved Hide resolved
x/bank/client/rest/grpc_query_test.go Outdated Show resolved Hide resolved
x/bank/client/rest/grpc_query_test.go Outdated Show resolved Hide resolved
x/bank/client/rest/grpc_query_test.go Outdated Show resolved Hide resolved
types/rest/rest.go Outdated Show resolved Hide resolved
x/bank/client/rest/grpc_query_test.go Show resolved Hide resolved
x/bank/client/rest/grpc_query_test.go Outdated Show resolved Hide resolved
amaury1093
amaury1093 approved these changes Aug 25, 2020
View reviewed changes
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

The PR lgtm!

Added question about how the staking validators HTTP route should look like

x/staking/client/rest/grpc_query_test.go
true,
},
{
"test query validators gRPC route without status query param",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, if the status query param is required, would it make more sense to have the route as /cosmos/staking/v1beta1/validators/{status}?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I also agree that requiring query parameters seems strange. Is this a pattern we follow elsewhere? Would be nice to not have it at all as a pattern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That will conflict with other route: /cosmos/staking/v1beta1/validators/{validator_id}. I also spent some time to update this in annotations PR but ended up here. Old REST querier is also using it in the same way. May be we can add a default status for this. By default we can fetch all the bonded validators, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Having a default status sounds better than erroring out no a REST call without query parameters IMO.

@mergify mergify bot merged commit 78194e1 into master Aug 25, 2020
@mergify mergify bot deleted the atheesh/5921-grpc-gateway-register branch August 25, 2020 15:44
@amaury1093
Copy link
Contributor

ah, forgot about automerge. My question #7019 (comment) is still relevant

@anilcse anilcse mentioned this pull request Aug 25, 2020
Merged
9 tasks
This was referenced Aug 26, 2020
Merged
Merged
@mdaleem mdaleem mentioned this pull request Sep 1, 2020
Merged
9 tasks
This was referenced Sep 1, 2020
Merged
Merged
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@atheeshp @anilcse @fedekunze
gRPC gateway init (cosmos#7019)
b7b16e8 
* WIP: grpc server setup

* add register grpc routes

* updated go mod

* updated grpc for all modules

* added pb file for grpc gateway

* udpated gw file

* added a test for grpc route

* fixed conflicts

* added grpc server

* grpc tests added

* cleanup

* Fix gateway forward issue

* Add godoc

* updated tests

* fix lint

* fix tests

* fix tests

* fix tests

* fixed test

* Add grpc headers

* Fix error handling

* Fix tests - hacky

* Fix lint

* remove debug logs

* Fix review comments

* move grpc tests into a separate file

* Fix protobuf version

* Update x/capability/module.go

* Fix godoc

* Fix review suggestions

* Fix codec

* Add query params test for gateway request

* Fix gofmt

Co-authored-by: anilCSE <anil@vitwit.com>
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anilcse anilcse anilcse left review comments

@clevinson clevinson clevinson left review comments

@fedekunze fedekunze fedekunze approved these changes

@amaury1093 amaury1093 amaury1093 approved these changes

@aaronc aaronc Awaiting requested review from aaronc

@alessio alessio Awaiting requested review from alessio

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

@blushi blushi Awaiting requested review from blushi

Assignees
No one assigned
Labels
A:automerge Automatically merge PR once all prerequisites pass. C: gRPC Issues and PRs related to the gRPC service and HTTP gateway.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@atheeshp @anilcse @amaury1093 @clevinson @alexanderbez @fedekunze