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

x/staking: gRPC query Service #6490

Merged
merged 67 commits into from
Jul 14, 2020
Merged

x/staking: gRPC query Service #6490

merged 67 commits into from
Jul 14, 2020

Conversation

sahith-narahari
Copy link
Contributor

Description

Ref: #5921


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

@aaronc aaronc mentioned this pull request Jun 23, 2020
43 tasks
@codecov
Copy link

codecov bot commented Jun 23, 2020

Codecov Report

Merging #6490 into master will increase coverage by 0.18%.
The diff coverage is 70.86%.

@@            Coverage Diff             @@
##           master    #6490      +/-   ##
==========================================
+ Coverage   58.46%   58.65%   +0.18%     
==========================================
  Files         511      512       +1     
  Lines       31047    31280     +233     
==========================================
+ Hits        18151    18346     +195     
- Misses      11543    11546       +3     
- Partials     1353     1388      +35     

@lgtm-com
Copy link

lgtm-com bot commented Jun 24, 2020

This pull request introduces 3 alerts when merging bc3775f into 06d7dbe - view on LGTM.com

new alerts:

  • 3 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Jun 24, 2020

This pull request introduces 9 alerts when merging e4738c7 into cb6c552 - view on LGTM.com

new alerts:

  • 9 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Jun 25, 2020

This pull request introduces 3 alerts when merging a485a85 into 51c35f4 - view on LGTM.com

new alerts:

  • 3 for Useless assignment to local variable

@sahith-narahari sahith-narahari marked this pull request as draft July 9, 2020 22:43
@clevinson clevinson added this to the v0.40 [Stargate] milestone Jul 9, 2020
@sahith-narahari sahith-narahari marked this pull request as ready for review July 12, 2020 20:24
x/staking/keeper/grpc_query_test.go Outdated Show resolved Hide resolved
x/staking/keeper/grpc_query_test.go Outdated Show resolved Hide resolved
x/staking/keeper/grpc_query_test.go Outdated Show resolved Hide resolved
@fedekunze fedekunze requested a review from aaronc July 13, 2020 19:56
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

I notice gogoproto.nullable = false is used a lot. Is this really needed? I'd rather minimize the usage of those annotations to things needed for backwards golang API compatibility. Is it to reduce nil checks?

@sahith-narahari
Copy link
Contributor Author

I notice gogoproto.nullable = false is used a lot. Is this really needed? I'd rather minimize the usage of those annotations to things needed for backwards golang API compatibility. Is it to reduce nil checks?

The main reason was to avoid dealing with nil data, but agree gogoproto extension usage should be minimised. Will ensure to keep it in mind for future work

@sahith-narahari sahith-narahari added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 14, 2020
@aaronc
Copy link
Member

aaronc commented Jul 14, 2020

@sahith-narahari thanks. I think it's okay to leave it for now. I understand it would be a bigger refactor.

@mergify mergify bot merged commit 5656e86 into master Jul 14, 2020
@mergify mergify bot deleted the sahith/5921-staking-grpc branch July 14, 2020 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants