Make JSONMarshaler methods require proto.Message #7054

blushi · 2020-08-14T13:45:30Z

Description

closes: #6982

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-14T13:49:53Z

Codecov Report

Merging #7054 into master will decrease coverage by 0.02%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master    #7054      +/-   ##
==========================================
- Coverage   54.53%   54.50%   -0.03%     
==========================================
  Files         566      566              
  Lines       38727    38752      +25     
==========================================
+ Hits        21119    21122       +3     
- Misses      15884    15900      +16     
- Partials     1724     1730       +6

client/context.go

…rie/6982-json-marshaler-proto

lgtm-com · 2020-08-17T16:09:21Z

This pull request introduces 2 alerts when merging fa96cfe into c25b3b9 - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

client/tx/tx.go

x/gov/client/cli/query.go

amaury1093

this is a big PR, I didn't review everything yet, will continue after lunch.

client/context.go

x/genutil/client/cli/validate_genesis.go

x/genutil/client/cli/migrate.go

aaronc

Thanks for getting to all of this @blushi ! This is a massive and complex PR. And much needed IMHO.

I also didn't make it all the through in this review, but I think I get the general gist of what's needed next.

REST and legacy querier changes look correct.

We don't want to use legacy amino at all with simulation I believe.

Also, we don't want to use amino in CLI tests except when PrintOutputLegacy is used. In all other cases we want to use JSONMarshaler

client/tx/tx.go

simapp/app.go

simapp/state.go

x/bank/client/cli/cli_test.go

x/bank/simulation/genesis.go

x/bank/simulation/genesis_test.go

x/bank/simulation/params.go

x/bank/client/cli/cli_test.go

amaury1093

Nice! Here are the last couple of pieces that didn't make sense to me.

The biggest one is why are we using encoding/json for GenesisStates? Is it because they simple enough that JSONMarshaler.Marshal() == json.Marshal() in those cases?

server/util.go

simapp/state.go

x/gov/simulation/genesis.go

x/params/client/cli/tx.go

amaury1093

This lgtm! I'm happy to merge this (one last small nit)

server/util.go

fedekunze

utACK

codec/amino_codec.go

codec/proto_codec.go

types/module/module_test.go

types/module/simulation.go

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

server/util.go

anilcse

lgtm

addressed

amaury1093

@blushi Feel free to put automerge on when you're happy with this PR

server/util_test.go

blushi · 2020-08-26T08:28:10Z

@blushi Feel free to put automerge on when you're happy with this PR

I still need to fix some issues after merging master(cf. CI failing).

* Make JSONMarshaler require proto.Message * Use &msg with MarshalJSON * Use *LegacyAmino in queriers instead of JSONMarshaler * Revert ABCIMessageLogs String() and coins tests * Use LegacyAmino in client/debug and fix subspace tests * Use LegacyAmino in all legacy queriers and adapt simulation * Make AminoCodec implement Marshaler and some godoc fixes * Test fixes * Remove unrelevant comment * Use TxConfig.TxJSONEncoder * Use encoding/json in genutil cli migrate/validate genesis cmds * Address simulation related comments * Use JSONMarshaler in cli tests * Use proto.Message as respType in cli tests * Use tmjson for tm GenesisDoc * Update types/module/simulation.go Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> * Update types/module/module_test.go Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> * Add godoc comments * Remove unused InsertKeyJSON * Fix tests Co-authored-by: Aaron Craelius <aaronc@users.noreply.github.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

blushi added 8 commits August 13, 2020 12:49

Make JSONMarshaler require proto.Message

97c59de

Use &msg with MarshalJSON

8f43ba6

Use *LegacyAmino in queriers instead of JSONMarshaler

b6dd69b

Revert ABCIMessageLogs String() and coins tests

3af5055

Use LegacyAmino in client/debug and fix subspace tests

282cc28

Merge branch 'master' into marie/6982-json-marshaler-proto

f572cd3

Use LegacyAmino in all legacy queriers and adapt simulation

3f68d9a

Merge branch 'master' into marie/6982-json-marshaler-proto

8ed3b64

Make AminoCodec implement Marshaler and some godoc fixes

2d452d4

aaronc reviewed Aug 14, 2020

View reviewed changes

client/context.go Show resolved Hide resolved

blushi requested a review from anilcse August 17, 2020 15:27

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

fa96cfe

…rie/6982-json-marshaler-proto

Test fixes

28db5ea

aaronc requested changes Aug 17, 2020

View reviewed changes

client/tx/tx.go Outdated Show resolved Hide resolved

aaronc mentioned this pull request Aug 18, 2020

Simd: cannot protobuf JSON encode unsupported type: *tx.builder #7079

Closed

4 tasks

blushi commented Aug 19, 2020

View reviewed changes

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

blushi commented Aug 19, 2020

View reviewed changes

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

blushi added 3 commits August 19, 2020 09:46

Remove unrelevant comment

2b65f5a

Merge branch 'master' into marie/6982-json-marshaler-proto

48103b9

Use TxConfig.TxJSONEncoder

b4da972

blushi marked this pull request as ready for review August 19, 2020 08:55

blushi requested review from alessio, alexanderbez, clevinson, fedekunze and jgimeno as code owners August 19, 2020 08:55

amaury1093 reviewed Aug 19, 2020

View reviewed changes

client/context.go Show resolved Hide resolved

x/genutil/client/cli/validate_genesis.go Outdated Show resolved Hide resolved

x/genutil/client/cli/migrate.go Outdated Show resolved Hide resolved

x/genutil/client/cli/migrate.go Outdated Show resolved Hide resolved

Use encoding/json in genutil cli migrate/validate genesis cmds

ecc133a

aaronc previously requested changes Aug 19, 2020

View reviewed changes

blushi added 2 commits August 19, 2020 17:26

Address simulation related comments

7e6d09d

Use JSONMarshaler in cli tests

abfdd22

aaronc reviewed Aug 19, 2020

View reviewed changes

x/bank/client/cli/cli_test.go Outdated Show resolved Hide resolved

blushi added 2 commits August 20, 2020 09:39

Use proto.Message as respType in cli tests

a3e394e

Merge branch 'master' into marie/6982-json-marshaler-proto

e5f88e1

amaury1093 suggested changes Aug 21, 2020

View reviewed changes

server/util.go Outdated Show resolved Hide resolved

simapp/state.go Outdated Show resolved Hide resolved

x/gov/simulation/genesis.go Show resolved Hide resolved

x/params/client/cli/tx.go Show resolved Hide resolved

blushi added 2 commits August 24, 2020 13:05

Use tmjson for tm GenesisDoc

7665df4

Merge branch 'master' into marie/6982-json-marshaler-proto

8ab0066

amaury1093 approved these changes Aug 24, 2020

View reviewed changes

server/util.go Outdated Show resolved Hide resolved

clevinson mentioned this pull request Aug 24, 2020

Tracking Issue: Stargate Release Candidate #7152

Closed

25 tasks

fedekunze reviewed Aug 25, 2020

View reviewed changes

codec/amino_codec.go Show resolved Hide resolved

codec/proto_codec.go Show resolved Hide resolved

types/module/module_test.go Outdated Show resolved Hide resolved

types/module/simulation.go Outdated Show resolved Hide resolved

blushi and others added 2 commits August 25, 2020 16:58

Update types/module/simulation.go

12754cd

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

Update types/module/module_test.go

17ea317

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

alexanderbez approved these changes Aug 25, 2020

View reviewed changes

server/util.go Outdated Show resolved Hide resolved

anilcse approved these changes Aug 25, 2020

View reviewed changes

blushi added 3 commits August 26, 2020 09:50

Add godoc comments

4121810

Remove unused InsertKeyJSON

4aa1d08

Merge branch 'master' into marie/6982-json-marshaler-proto

5060ae3

amaury1093 approved these changes Aug 26, 2020

View reviewed changes

server/util_test.go Outdated Show resolved Hide resolved

Fix tests

0cff5ca

blushi added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 26, 2020

mergify bot merged commit 7f59723 into master Aug 26, 2020

mergify bot deleted the marie/6982-json-marshaler-proto branch August 26, 2020 09:39

aaronc mentioned this pull request Aug 28, 2020

General Amino audit / cleanup / IAVL changes #6194

Closed

3 tasks

blushi mentioned this pull request Oct 15, 2020

Remove uses of LegacyAmino to unmarshal into arrays of proto messages #7559

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make JSONMarshaler methods require proto.Message #7054

Make JSONMarshaler methods require proto.Message #7054

blushi commented Aug 14, 2020

codecov bot commented Aug 14, 2020 •

edited

Loading

lgtm-com bot commented Aug 17, 2020

amaury1093 left a comment •

edited

Loading

aaronc left a comment

amaury1093 left a comment •

edited

Loading

amaury1093 left a comment

fedekunze left a comment

anilcse left a comment

amaury1093 left a comment

blushi commented Aug 26, 2020

Make JSONMarshaler methods require proto.Message #7054

Make JSONMarshaler methods require proto.Message #7054

Conversation

blushi commented Aug 14, 2020

Description

codecov bot commented Aug 14, 2020 • edited Loading

Codecov Report

lgtm-com bot commented Aug 17, 2020

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

anilcse left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

blushi commented Aug 26, 2020

codecov bot commented Aug 14, 2020 •

edited

Loading

amaury1093 left a comment •

edited

Loading

amaury1093 left a comment •

edited

Loading