Enable proto JSON generally and remove HybridCodec #6859

aaronc · 2020-07-27T14:43:21Z

A few notes:

all of the legacy REST and querier endpoints need to use LegacyAmino explicitly. That is a lot of what is happening here
x/params is still using amino JSON and requires it explicitly now because HybridCodec is gone (to be addressed in Migrate x/params JSON state #6983)
EmitDefaults was turned on for proto JSON. This should overall produce a better UX but sometimes adds empty fields which aren't too informative. We may need to experiment with this.
AccountRetriever still uses amino JSON (to be cleaned up in Migrate AccountRetriever to proto #6985)
vesting accounts simply can't have all that custom JSON marshaling and work with jsonpb, therefore it was removed with a CHANGELOG entry added

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

…ronc/6837-final

codecov · 2020-08-07T20:03:47Z

Codecov Report

Merging #6859 into master will increase coverage by 0.92%.
The diff coverage is 46.77%.

@@            Coverage Diff             @@
##           master    #6859      +/-   ##
==========================================
+ Coverage   61.02%   61.95%   +0.92%     
==========================================
  Files         381      520     +139     
  Lines       24844    32036    +7192     
==========================================
+ Hits        15162    19848    +4686     
- Misses       8486    10559    +2073     
- Partials     1196     1629     +433

…ronc/6837-final

tac0turtle · 2020-08-10T16:34:56Z

One important thing to note: GenesisDoc is now marshaled/unmarshaled using the golang encoding/json instead of amino or proto JSON. Proto JSON will not work because GenesisDoc is not a proto.Message (cc @marbar3778).

the genesis is encoded with a similar json style as amino. is the current approach a problem?

aaronc · 2020-08-10T17:00:37Z

One important thing to note: GenesisDoc is now marshaled/unmarshaled using the golang encoding/json instead of amino or proto JSON. Proto JSON will not work because GenesisDoc is not a proto.Message (cc @marbar3778).

the genesis is encoded with a similar json style as amino. is the current approach a problem?

Is it okay if we just use encoding/json instead of amino for this?

tac0turtle · 2020-08-10T17:06:04Z

Is it okay if we just use encoding/json instead of amino for this?

Applications should be free to use whatever format they want with Tendermint because the application state is json.RawMessage.

…ronc/6837-final

aaronc · 2020-08-11T22:03:13Z

Not sure how to debug the failing sims. Any thoughts @alexanderbez ? Quite sure it has to do with amino being used somewhere for genesis, just not sure where

alexanderbez · 2020-08-12T13:36:49Z

Seems only to be involved with export/import, I reran one of the seeds from CI:

$ go test ./simapp -run TestAppImportExport -Enabled=true -NumBlocks=50 -Genesis= -Verbose=true -Commit=true -Seed=7 -Period=5 -v -timeout 24h
...
exporting genesis...
importing genesis...
--- FAIL: TestAppImportExport (89.33s)
panic: unknown value "NoWithVeto" for enum cosmos.gov.v1beta1.VoteOption [recovered]
        panic: unknown value "NoWithVeto" for enum cosmos.gov.v1beta1.VoteOption

goroutine 50 [running]:
testing.tRunner.func1.1(0x5027600, 0xc00273ee90)
        /usr/local/go/src/testing/testing.go:1057 +0x30d
testing.tRunner.func1(0xc0005d2d80)
        /usr/local/go/src/testing/testing.go:1060 +0x41a
panic(0x5027600, 0xc00273ee90)
        /usr/local/go/src/runtime/panic.go:969 +0x175
github.com/cosmos/cosmos-sdk/codec.(*ProtoCodec).MustUnmarshalJSON(0xc000e36aa0, 0xc00572e000, 0xc9d6b, 0xca000, 0x51b8d20, 0xc003d9be60)
        /Users/aleksbez/code/cosmos/cosmos-sdk/codec/proto_codec.go:132 +0x8a
github.com/cosmos/cosmos-sdk/x/gov.AppModule.InitGenesis(0x55afce0, 0xc0013f0320, 0x0, 0x0, 0x0, 0x5588280, 0xc00247cd80, 0x559e000, 0xc0001ad200, 0x2df4c710, ...)
        /Users/aleksbez/code/cosmos/cosmos-sdk/x/gov/module.go:162 +0xa4
github.com/cosmos/cosmos-sdk/types/module.(*Manager).InitGenesis(0xc004476a10, 0x559cf40, 0xc0000400b0, 0x55adae0, 0xc00302ed80, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /Users/aleksbez/code/cosmos/cosmos-sdk/types/module/module.go:293 +0x2b5
github.com/cosmos/cosmos-sdk/simapp.TestAppImportExport(0xc0005d2d80)
        /Users/aleksbez/code/cosmos/cosmos-sdk/simapp/sim_test.go:145 +0xe52
testing.tRunner(0xc0005d2d80, 0x5406428)
        /usr/local/go/src/testing/testing.go:1108 +0xef
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:1159 +0x386
FAIL    github.com/cosmos/cosmos-sdk/simapp     90.985s
FAIL

…ronc/6837-final

aaronc · 2020-08-12T15:41:00Z

Thanks @alexanderbez that gave me a helpful clue. Seems related to #7010. How did you get that seed command to run btw?

alexanderbez · 2020-08-12T15:50:44Z

@aaronc it's in the original comment:

Run the following from the root dir:

$ go test ./simapp -run TestAppImportExport -Enabled=true -NumBlocks=50 -Genesis= -Verbose=true -Commit=true -Seed=7 -Period=5 -v -timeout 24h

If you take a look at the CI output, it'll actually show you the command to run for all the failed seeds.

The issue makes sense though. We need to remove custom JSON marshaling for types?

aaronc · 2020-08-12T15:55:27Z

If you take a look at the CI output, it'll actually show you the command to run for all the failed seeds.

Okay thanks.

The issue makes sense though. We need to remove custom JSON marshaling for types?

Well, it appears to work in some cases. For instance AccAddress is getting JSON marshaled as bech32 and we haven't run into any problems. That seems desirable, but it doesn't conform to the specification. That's probably okay though - we just need to have docs somewhere that describe our extensions to the default proto3 JSON. We were having the problems with enums mainly and there I don't think we should have custom JSON. But bech32 addresses make sense. The alternative would be encoding addresses as string instead of bytes in proto.

alexanderbez · 2020-08-12T16:01:29Z

Yeah makes sense. Can we just address enums for now?

aaronc · 2020-08-12T19:43:34Z

Yeah makes sense. Can we just address enums for now?

That's the only thing this PR takes care of, just for the failing test cases.

aaronc · 2020-08-12T19:51:50Z

Okay, I think this is ready. Unit tests are passing again and sims were fixed in the last build

server/start.go

simapp/app.go

sahith-narahari

lgtm

amaury1093

Ah, @sahith-narahari beat me to it. LGTM.

amaury1093 · 2020-08-13T13:21:13Z

x/distribution/types/codec.go

@@ -41,7 +41,7 @@ var (
 	//
 	// The actual codec used for serialization should be provided to x/distribution and
 	// defined at the application level.
-	ModuleCdc = codec.NewHybridCodec(amino, types.NewInterfaceRegistry())
+	ModuleCdc = codec.NewAminoCodec(amino)


Are these used in queriers or in REST?

* Remove HybridCodec * WIP on fixing proto JSON issues * WIP on fixing proto JSON issues * WIP on fixing proto JSON issues * WIP on fixing proto JSON issues * WIP on fixing proto JSON issues * Test fixes * Delete hybrid_codec.go * Fixes * Fixes * Fixes * Test fixes * Test fixes * Test fixes * Lint * Sim fixes * Sim fixes * Revert * Remove vesting account JSON tests * Update CHANGELOG.md * Lint * Sim fixes * Sim fixes * Docs * Migrate more amino usages * Remove custom VoteOption String() and json marshaling * Fix tests * Add comments, update CHANGELOG.md Co-authored-by: Jonathan Gimeno <jgimeno@gmail.com> Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

aaronc and others added 4 commits July 27, 2020 10:41

Remove HybridCodec

e20e717

Merge branch 'master' into aaronc/6837-final

6788a6c

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

46bf0ee

…ronc/6837-final

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

3732302

…ronc/6837-final

aaronc changed the title ~~DNM: Remove HybridCodec~~ Enable proto JSON generally and remove HybridCodec Aug 7, 2020

aaronc added 3 commits August 7, 2020 13:45

WIP on fixing proto JSON issues

6bd19c4

WIP on fixing proto JSON issues

4b41af5

WIP on fixing proto JSON issues

675d9f7

WIP on fixing proto JSON issues

ce3ae55

anilcse mentioned this pull request Aug 7, 2020

Migrate remaining CLI cmd's to proto #6987

Closed

8 tasks

aaronc added 3 commits August 7, 2020 17:56

WIP on fixing proto JSON issues

c208575

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

4c067a2

…ronc/6837-final

Test fixes

81beade

This was referenced Aug 10, 2020

Enable proto JSON for genesis #7000

Merged

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

Merged

Account Query Failure #7007

Closed

aaronc added 7 commits August 11, 2020 10:28

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

0acdd2a

…ronc/6837-final

Delete hybrid_codec.go

63ebec3

Fixes

48adb63

Fixes

8cc6084

Fixes

06b012b

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

42bd0a8

…ronc/6837-final

Test fixes

722da0b

aaronc added the C:Encoding label Aug 11, 2020

aaronc added 2 commits August 11, 2020 13:51

Test fixes

053b77f

Test fixes

f651177

aaronc added 2 commits August 11, 2020 14:47

Update CHANGELOG.md

fab24c6

Lint

11d5c04

aaronc added C:x/auth C:x/params labels Aug 11, 2020

aaronc added 3 commits August 11, 2020 15:13

Sim fixes

9e8a745

Sim fixes

b52ab51

Docs

d0c1263

aaronc and others added 4 commits August 12, 2020 10:17

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

66ca469

…ronc/6837-final

Migrate more amino usages

e98c8d2

Merge branch 'master' into aaronc/6837-final

55d681d

Remove custom VoteOption String() and json marshaling

7a91e69

Merge branch 'master' into aaronc/6837-final

0d929fa

Fix tests

2b0f52b

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

aaronc mentioned this pull request Aug 12, 2020

simd: init fails #7026

Closed

4 tasks

alexanderbez reviewed Aug 12, 2020

View reviewed changes

server/start.go Show resolved Hide resolved

simapp/app.go Show resolved Hide resolved

Add comments, update CHANGELOG.md

d6643ba

sahith-narahari approved these changes Aug 13, 2020

View reviewed changes

mergify bot merged commit 816c5a3 into master Aug 13, 2020

mergify bot deleted the aaronc/6837-final branch August 13, 2020 13:20

amaury1093 approved these changes Aug 13, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable proto JSON generally and remove HybridCodec #6859

Enable proto JSON generally and remove HybridCodec #6859

aaronc commented Jul 27, 2020 •

edited

Loading

codecov bot commented Aug 7, 2020 •

edited

Loading

tac0turtle commented Aug 10, 2020

aaronc commented Aug 10, 2020

tac0turtle commented Aug 10, 2020

aaronc commented Aug 11, 2020 •

edited

Loading

alexanderbez commented Aug 12, 2020 •

edited

Loading

aaronc commented Aug 12, 2020

alexanderbez commented Aug 12, 2020

aaronc commented Aug 12, 2020

alexanderbez commented Aug 12, 2020

aaronc commented Aug 12, 2020

aaronc commented Aug 12, 2020

sahith-narahari left a comment

amaury1093 left a comment

amaury1093 Aug 13, 2020

Enable proto JSON generally and remove HybridCodec #6859

Enable proto JSON generally and remove HybridCodec #6859

Conversation

aaronc commented Jul 27, 2020 • edited Loading

codecov bot commented Aug 7, 2020 • edited Loading

Codecov Report

tac0turtle commented Aug 10, 2020

aaronc commented Aug 10, 2020

tac0turtle commented Aug 10, 2020

aaronc commented Aug 11, 2020 • edited Loading

alexanderbez commented Aug 12, 2020 • edited Loading

aaronc commented Aug 12, 2020

alexanderbez commented Aug 12, 2020

aaronc commented Aug 12, 2020

alexanderbez commented Aug 12, 2020

aaronc commented Aug 12, 2020

aaronc commented Aug 12, 2020

sahith-narahari left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Aug 13, 2020

Choose a reason for hiding this comment

aaronc commented Jul 27, 2020 •

edited

Loading

codecov bot commented Aug 7, 2020 •

edited

Loading

aaronc commented Aug 11, 2020 •

edited

Loading

alexanderbez commented Aug 12, 2020 •

edited

Loading