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

Use gRPC in GetCmdQueryValidators #7626

Merged
merged 4 commits into from
Oct 22, 2020

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Oct 22, 2020

Description

to be merged into #7597

  • Migrate CLI GetCmdQueryValidators to use gRPC and outputs protoJSON instead of aminoJSON (this fixes the test)
  • revert MakeEncodingConfigTests name change
  • remove RegisterCodecsTests

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 Oct 22, 2020

Codecov Report

Merging #7626 into robert/validator-pubkey will decrease coverage by 2.59%.
The diff coverage is 53.84%.

@@                     Coverage Diff                     @@
##           robert/validator-pubkey    #7626      +/-   ##
===========================================================
- Coverage                    59.82%   57.23%   -2.60%     
===========================================================
  Files                          317      453     +136     
  Lines                        17992    27723    +9731     
===========================================================
+ Hits                         10763    15866    +5103     
- Misses                        6081    10331    +4250     
- Partials                      1148     1526     +378     

return res, height, err
}

ctx.LegacyAmino.MustUnmarshalBinaryBare(resRaw, &res)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fmt.Printlned the res here, it's a []KVPair of []bytes->[]bytes, and I'm assuming those bytes are proto-encoded.

This means that the client.Context would need to hold a reference to the BinaryMarshaler too, which seems like a code smell.

Instead, I deleted this function (only used in 1 place), and used gRPC queries in GetCmdQueryValidators.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, I assume we are.ok to delete this public function?

@amaury1093 amaury1093 mentioned this pull request Oct 22, 2020
9 tasks
@@ -22,7 +22,7 @@ import (

func TestMsgService(t *testing.T) {
priv, _, _ := testdata.KeyTestPubAddr()
encCfg := simapp.MakeEncodingConfigTests()
encCfg := simapp.MakeEncodingConfig()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you change this?

Copy link
Contributor Author

@amaury1093 amaury1093 Oct 22, 2020

Choose a reason for hiding this comment

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

return res, height, err
}

ctx.LegacyAmino.MustUnmarshalBinaryBare(resRaw, &res)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, I assume we are.ok to delete this public function?

@amaury1093 amaury1093 marked this pull request as ready for review October 22, 2020 13:01
@robert-zaremba robert-zaremba merged commit 334253f into robert/validator-pubkey Oct 22, 2020
@robert-zaremba robert-zaremba deleted the am-validator-pubkey branch October 22, 2020 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants