-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Enable proto JSON for genesis #7000
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7000 +/- ##
==========================================
+ Coverage 59.07% 61.91% +2.84%
==========================================
Files 363 520 +157
Lines 21499 32151 +10652
==========================================
+ Hits 12701 19907 +7206
- Misses 7755 10629 +2874
- Partials 1043 1615 +572 |
…ronc/enable-proto-genesis
As long as types can be deserialized into the corresponding |
…ronc/enable-proto-genesis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, also not 100% clear on how the withProtoJSON
middleware works, but i'm okay if it's temporary.
app = simapp.Setup(false) | ||
appCodec, _ = simapp.MakeCodecs() | ||
app = simapp.Setup(false) | ||
appCodec, legacyAmino = simapp.MakeCodecs() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a refactor for another PR, but MakeCodecs is redundant since we have MakeEncodingConfig, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes more or less. It's a holdover from earlier versions
So it changes the JSON marshaler in the client context attached to the cobra command. Maybe worth studying that code a bit to understand how things work in the CLI |
…ronc/enable-proto-genesis
* Enable proto JSON for genesis * Test fixes * Cleanup * Cleanup * Cleanup * Update CHANGELOG.md * Protogen Co-authored-by: Federico Kunze <[email protected]> Co-authored-by: SaReN <[email protected]>
Closes #5917. (Was originally part of #6859 but split out into a smaller PR)
A few important notes:
GenesisDoc
now usesencoding/json
GenesisDoc
is now marshaled/unmarshaled using the golangencoding/json
instead of amino or proto JSON. Proto JSON will not work becauseGenesisDoc
is not aproto.Message
(cc @marbar3778).As I understand it, this has the side effect that certain
int64
/uint64
types will be encoded as actual JSON integers as opposed to strings. If this is not okay we can either just use amino JSON or we will need proto support from tm.Note that this affects both proto and amino configurations of the SDK (although currently only proto works).
ProposalStatus
now uses default protobuf string formattingProposalStatus
is now printed to JSON with its proto name, i.e.PROPOSAL_STATUS_DEPOSIT_PERIOD
vsDepositPeriod
NewSimApp
now takes a parameter forEncodingConfig
This is necessary to export genesis with protobuf and in general may be useful for testing.
Pointer types must be used for
GenesisState
everywhereThis is true for all
proto.Message
marshaling.JSONMarshaler
should just requireproto.Message
to avoid runtime errors, to be followed up in #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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes