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/{gov,params,upgrade,distribution} CLI: In-Process test & use grpc query service #6664

Merged
merged 28 commits into from
Jul 20, 2020

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Jul 9, 2020

Description

Refactoring CLI for in-process testing, and use gRPC query service in CLI.

TODO:

ref:

In-process Testing:

CLI to use grpc query service:


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

@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #6664 into master will decrease coverage by 0.02%.
The diff coverage is 19.77%.

@@            Coverage Diff             @@
##           master    #6664      +/-   ##
==========================================
- Coverage   60.25%   60.22%   -0.03%     
==========================================
  Files         512      511       -1     
  Lines       31502    31532      +30     
==========================================
+ Hits        18982    18991       +9     
- Misses      11073    11105      +32     
+ Partials     1447     1436      -11     

@clevinson clevinson added this to the v0.40 [Stargate] milestone Jul 10, 2020
@amaury1093 amaury1093 changed the title x/{gov,params,upgrade,distribution} refactor CLI & use grpc query service x/{gov,params,upgrade,distribution} in process test & use grpc query service Jul 10, 2020
This was referenced Jul 10, 2020
@amaury1093 amaury1093 changed the title x/{gov,params,upgrade,distribution} in process test & use grpc query service x/{gov,params,upgrade,distribution} CLI: In-Process test & use grpc query service Jul 10, 2020
@lgtm-com
Copy link

lgtm-com bot commented Jul 16, 2020

This pull request introduces 1 alert when merging 439990e into ebc3ad6 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@amaury1093 amaury1093 marked this pull request as ready for review July 17, 2020 16:17
@fedekunze fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 20, 2020
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.

LGTM

Left some comments about naming of PageRequest parameters that should be addressed in a separate PR.

ValidatorAddress: validatorAddr,
StartingHeight: startHeight,
EndingHeight: endHeight,
Req: pageReq,
Copy link
Member

Choose a reason for hiding this comment

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

I didn't catch this before but page parameters should not be called req. I would just use page or something like that. This can be fixed separate from this PR but it should be addressed before release. /cc @sahith-narahari @anilcse @atheeshp

ProposalStatus: proposalStatus,
Voter: voterAddr,
Depositor: depositorAddr,
Req: pageReq,
Copy link
Member

Choose a reason for hiding this comment

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

Ditto


res, err := queryClient.Votes(
context.Background(),
&types.QueryVotesRequest{ProposalId: proposalID, Req: pageReq},
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@mergify mergify bot merged commit 8e61ef8 into master Jul 20, 2020
@mergify mergify bot deleted the am-cli-params-distrib branch July 20, 2020 13:10
chengwenxi pushed a commit to irisnet/cosmos-sdk that referenced this pull request Jul 22, 2020
…uery service (cosmos#6664)

* refactor CLI to use grpc query service

* In process CLI for gov

* ReadQueryCommandFlags

* gov tx

* Fix compiler errors

* Formatting

* x/distribution: use gRPC query

* Consistent

* Fix x/distrib test

* Update x/gov

* Add ReadQueryCommandFlags

* Fix lint

* Revert x/params

* x/params use grpc query

* Fix tests

* Use page request

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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. C:CLI C:x/distribution distribution module related C:x/gov C:x/params C:x/upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants