-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
x/gov - Proto Migration #5737
x/gov - Proto Migration #5737
Conversation
…rything besides proposal)
Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
Blocked on #5738. |
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.
Mostly looks good. Just made a few small suggestions.
return nil, err | ||
} | ||
|
||
return c.Marshaler.MarshalBinaryLengthPrefixed(proposal) |
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.
return c.Marshaler.MarshalBinaryLengthPrefixed(proposal) | |
return c.Marshaler.MarshalBinaryBare(proposal) |
Why should we be length-prefixing everywhere? It would only be needed for streaming messages AFAIK.
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.
We don't, but it's consistent. We should probably do one fell swoop (of the entire SDK) instead of ad-hoc per PR.
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.
So you mean that we do it for every custom marshaler in this codec in one fell-swoop? That's fine. But we should do it when or before this goes into codec/std
because then at that point it becomes state machine breaking for zones.
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.
I mean we do it for the entire SDK (where applicable). It doesn't matter if it's before or after the std
move. This is all happening before the release (0.39) anyway.
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.
// to deserialize. | ||
func (c *Codec) UnmarshalProposal(bz []byte) (gov.Proposal, error) { | ||
proposal := &Proposal{} | ||
if err := c.Marshaler.UnmarshalBinaryLengthPrefixed(bz, proposal); err != nil { |
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.
if err := c.Marshaler.UnmarshalBinaryLengthPrefixed(bz, proposal); err != nil { | |
if err := c.Marshaler.UnmarshalBinaryBare(bz, proposal); err != nil { |
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.
See above.
Co-Authored-By: Aaron Craelius <aaron@regen.network>
… into bez/5444-gov-proto-enc
Co-Authored-By: Anil Kumar Kammari <anil@vitwit.com>
Codecov Report
@@ Coverage Diff @@
## master #5737 +/- ##
=======================================
Coverage 34.88% 34.88%
=======================================
Files 337 337
Lines 34647 34647
=======================================
Hits 12087 12087
Misses 21323 21323
Partials 1237 1237
|
TODO:
MsgSubmitProposal
simapp/codec
tocodec/std
(should be done in a subsequent PR)replaces: #5493
ref: #5444
/cc @aaronc
For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)