Require proto.Message in client.Context.PrintOutput #6999

aaronc · 2020-08-10T17:19:10Z

Was originally part of #6859 but split out into a smaller PR

This is a stepping stone to enabling protobuf JSON generally in #6859.

client.Context.PrintOutput now requires proto.Message - proto JSON is more or less impossible to enforce without this. Queries that returned arrays from query responses, now return the full response. This probably the correct behavior because these queries should also eventually include pagination data if they don't already
client.Context.PrintOutputLegacy was added to use amino JSON where we don't have proto types yet. Rather than dealing with all of the migrations in this already large PR, stuff that wasn't migrated uses PrintOutputLegacy and the issues are tracked in Migrate remaining CLI cmd's to proto #6987

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 · 2020-08-10T17:23:32Z

Codecov Report

Merging #6999 into master will decrease coverage by 2.54%.
The diff coverage is 44.11%.

@@            Coverage Diff             @@
##           master    #6999      +/-   ##
==========================================
- Coverage   61.94%   59.39%   -2.55%     
==========================================
  Files         519      368     -151     
  Lines       32088    21741   -10347     
==========================================
- Hits        19877    12914    -6963     
+ Misses      10597     7754    -2843     
+ Partials     1614     1073     -541

…ronc/cli-print-output-proto

aaronc · 2020-08-10T18:52:42Z

x/distribution/client/cli/tx.go

@@ -287,7 +287,7 @@ Where proposal.json contains:
 				return err
 			}

-			proposal, err := ParseCommunityPoolSpendProposalJSON(clientCtx.JSONMarshaler, args[0])
+			proposal, err := ParseCommunityPoolSpendProposalJSON(clientCtx.Codec, args[0])


To be migrated in #6987

aaronc · 2020-08-10T18:53:07Z

x/distribution/client/cli/utils.go

@@ -19,7 +19,8 @@ type (
 )

 // ParseCommunityPoolSpendProposalJSON reads and parses a CommunityPoolSpendProposalJSON from a file.
-func ParseCommunityPoolSpendProposalJSON(cdc codec.JSONMarshaler, proposalFile string) (CommunityPoolSpendProposalJSON, error) {
+// TODO: migrate this to protobuf


To be migrated in #6987

aaronc · 2020-08-10T18:53:29Z

client/context.go

+}
+
+// PrintOutputLegacy is a variant of PrintOutput that doesn't require a proto type
+// and uses amino JSON encoding. It will be removed in the near future!


To be removed in #6987 when all the remaining pieces are migrated to proto

…ronc/cli-print-output-proto

…nto aaronc/cli-print-output-proto

x/gov/client/cli/query.go

x/ibc/02-client/client/cli/query.go

fedekunze · 2020-08-10T19:50:59Z

x/mint/client/cli/query.go

@@ -84,7 +85,7 @@ func GetCmdQueryInflation() *cobra.Command {
 				return err
 			}

-			return clientCtx.PrintOutput(res.Inflation)
+			return clientCtx.PrintString(fmt.Sprintf("%s\n", res.Inflation))


why PrintString?

Inflation is a Dec and we can't print simply a Dec with jsonpb, but a Dec can just be printed as a string.

The previous output was something like "1234.00000" in quotes. The new version just prints a number not in quotes 1234.00000. If we actually want to print a JSON string we will need to print something like {"inflation":"1234.00000"} wrapped in an object. Would that be preferable?

Yes, {"inflation":"1234.00000"} would be better. May be return clientCtx.PrintOutput(res)?

fedekunze · 2020-08-10T19:51:04Z

x/mint/client/cli/query.go

@@ -116,7 +117,7 @@ func GetCmdQueryAnnualProvisions() *cobra.Command {
 				return err
 			}

-			return clientCtx.PrintOutput(res.AnnualProvisions)
+			return clientCtx.PrintString(fmt.Sprintf("%s\n", res.AnnualProvisions))


see comment above regarding Inflation Dec

x/staking/client/cli/query.go

fedekunze · 2020-08-10T19:51:35Z

x/upgrade/client/cli/query.go

@@ -105,7 +105,7 @@ func GetAppliedPlanCmd() *cobra.Command {
 			if err != nil {
 				return err
 			}
-			return clientCtx.PrintOutput(string(bz))
+			return clientCtx.PrintString(fmt.Sprintf("%s\n", string(bz)))


This was just printing a string using amino. It doesn't work in proto. So the easiest thing seems to be to just print a string. If a json string is really needed we could use encoding/json

x/auth/client/cli/query.go

client/context.go

Co-authored-by: Anil Kumar Kammari <anil@vitwit.com>

Enable proto JSON json for cli tx & query

dc76abb

anilcse mentioned this pull request Aug 10, 2020

Migrate remaining CLI cmd's to proto #6987

Closed

8 tasks

WIP on tests

71c24a5

aaronc changed the title ~~Enable proto JSON json for cli tx & query~~ Enable proto JSON for cli tx & query Aug 10, 2020

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into aa…

4d84ecf

…ronc/cli-print-output-proto

aaronc changed the title ~~Enable proto JSON for cli tx & query~~ Require proto.Message in client.Context.PrintOutput Aug 10, 2020

Test fixes, cleanup

1236c91

aaronc commented Aug 10, 2020

View reviewed changes

Cleanup

90cf4e7

aaronc marked this pull request as ready for review August 10, 2020 19:05

aaronc requested review from alessio, alexanderbez, clevinson, fedekunze and jgimeno as code owners August 10, 2020 19:05

aaronc requested review from atheeshp, amaury1093, anilcse and blushi August 10, 2020 19:06

aaronc added C:CLI C:Encoding labels Aug 10, 2020

fedekunze and others added 3 commits August 10, 2020 21:45

Merge branch 'master' into aaronc/cli-print-output-proto

876ad75

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into aa…

29035e6

…ronc/cli-print-output-proto

Merge remote-tracking branch 'origin/aaronc/cli-print-output-proto' i…

0f4f83f

…nto aaronc/cli-print-output-proto

fedekunze reviewed Aug 10, 2020

View reviewed changes

Address review comments

62536b4

anilcse reviewed Aug 10, 2020

View reviewed changes

x/auth/client/cli/query.go Show resolved Hide resolved

client/context.go Outdated Show resolved Hide resolved

aaronc and others added 2 commits August 10, 2020 17:41

Update client/context.go

f2a90da

Co-authored-by: Anil Kumar Kammari <anil@vitwit.com>

Fixes

0772483

anilcse approved these changes Aug 10, 2020

View reviewed changes

fedekunze approved these changes Aug 11, 2020

View reviewed changes

fedekunze merged commit 7de8ef7 into master Aug 11, 2020

fedekunze deleted the aaronc/cli-print-output-proto branch August 11, 2020 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require proto.Message in client.Context.PrintOutput #6999

Require proto.Message in client.Context.PrintOutput #6999

aaronc commented Aug 10, 2020 •

edited

Loading

codecov bot commented Aug 10, 2020 •

edited

Loading

aaronc Aug 10, 2020

aaronc Aug 10, 2020

aaronc Aug 10, 2020

fedekunze Aug 10, 2020

aaronc Aug 10, 2020 •

edited

Loading

anilcse Aug 10, 2020 •

edited

Loading

fedekunze Aug 10, 2020

aaronc Aug 10, 2020

fedekunze Aug 10, 2020

aaronc Aug 10, 2020

Require proto.Message in client.Context.PrintOutput #6999

Require proto.Message in client.Context.PrintOutput #6999

Conversation

aaronc commented Aug 10, 2020 • edited Loading

codecov bot commented Aug 10, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

anilcse Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Aug 10, 2020 •

edited

Loading

codecov bot commented Aug 10, 2020 •

edited

Loading

aaronc Aug 10, 2020 •

edited

Loading

anilcse Aug 10, 2020 •

edited

Loading