Update x/gov to use Any #6147

aaronc · 2020-05-05T19:04:20Z

ref: #6081

Makes codec.Codec a wrapper around amino.Codec rather than an alias. This is to support protobuf Anys. All legacy code is required to use this rather than amino.Codec directly
Updates gov.Proposal and gov.MsgSubmitProposal types to use Any for Content
Implements InterfaceModule for x/gov, x/distribution, x/params, and x/upgrade
Makes gov.GenesisState proto compatible (Genesis Protobuf Migration #5917)

For contributor use:

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

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

…ov-proto-any

codecov · 2020-05-08T20:56:13Z

Codecov Report

Merging #6147 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6147   +/-   ##
=======================================
  Coverage   54.78%   54.78%           
=======================================
  Files         446      446           
  Lines       27079    27079           
=======================================
  Hits        14834    14834           
  Misses      11196    11196           
  Partials     1049     1049

…ov-proto-any

…onc/x-gov-proto-any

aaronc · 2020-05-18T20:11:28Z

@alexanderbez I think I've addressed all your review feedback. Let me know if there is anything else

codec/amino.go

types/result.go

x/gov/keeper/proposal.go

x/gov/types/msgs.go

alpe

Good work. Some nits, personal preference (pp) and small change requests (req) on the code. I did not look into tests or doc this time due to time constraints.

alpe · 2020-05-19T08:13:41Z

codec/amino.go

+}
+
+func (cdc *Codec) MustMarshalBinaryBare(o interface{}) []byte {
+	err := cdc.marshalAnys(o)


nit: it would be easier to read and maintain the code if you would just call MustMarshalBinaryBare as the name suggests and handle the error instead of duplicating the logic.
(Same for the other MustXXXX methods(

Okay, I think I understand what you mean and addressed

ups, should have been "call MarshalBinaryBare". sorry

codec/types/amino_compat.go

alpe · 2020-05-19T08:45:31Z

types/result.go

+// UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces
+func (s SearchTxsResult) UnpackInterfaces(unpacker types.AnyUnpacker) error {
+	for _, tx := range s.Txs {
+		err := types.UnpackInterfaces(tx, unpacker)


Q: types.UnpackInterfaces returns nil when the Tx does NOT implement UnpackInterfacesMessage. Do you need to handle this case to be on the safe side? Alternatively you could move UnpackInterfacesMessage into the Tx interface so that this scenario does not exist.

This question also applies to other UnpackInterfaces methods.

It makes sense to handle this because in general it will be implemented. We could update the Tx interface but I don't think we want to impose that requirement more generally right now.

alpe · 2020-05-19T08:45:50Z

types/result.go

+
+// UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces
+func (r TxResponse) UnpackInterfaces(unpacker types.AnyUnpacker) error {
+	return types.UnpackInterfaces(r.Tx, unpacker)


Q: Same as SearchTxsResult. UnpackInterfaces

Yes, in general it will be implemented on Tx

alpe · 2020-05-19T08:56:17Z

x/gov/client/cli/tx.go

@@ -326,7 +326,10 @@ $ %s tx gov submit-proposal --title="Test Proposal" --description="My awesome pr

 			content := types.ContentFromProposalType(proposal.Title, proposal.Description, proposal.Type)

-			msg := types.NewMsgSubmitProposal(content, amount, cliCtx.GetFromAddress())
+			msg, err := types.NewMsgSubmitProposal(content, amount, cliCtx.GetFromAddress())


PP: I am very happy to see errors returned. For bonus points you could add some context via errors.Wrap(err, "") to the naked errors. A single word helps already a lot when tracing down issues. Especially when there are multiple error return statements (not the case here).

alpe · 2020-05-19T09:16:32Z

x/gov/types/msgs.go

+func (m *MsgSubmitProposal) GetContent() Content {
+	content, ok := m.Content.GetCachedValue().(Content)
+	if !ok {
+		return nil


Req: The CachedValue can be nil (in tests for example). It would be consistent to unpack the Any type.
Q: I am wondering about the interface cast: When m.Content is nil then nil makes sense as a result but when "CachedValue" does not implement the Content interface why returning nil instead of an error. This case should not happen with proper use of SetContent but m.Content is public so can be anything.

Is there a better approach you would suggest?

🤔 Ah, you don't have access to the unpacker without changing the method signature....
I guess you don't want to add it as a parameter but rely on the stack to initialize the object proper. As a safety net I would add the following conditions so that the method can not return anything inconsistent.

func (m *MsgSubmitProposal) GetContent() Content { if m.Content == nil { return nil } cv := m.Content.GetCachedValue() if cv == nil { panic("content was not unpacked") } content, ok := cv.(Content) if !ok { panic(fmt.Sprintf("illegal type: %T", cv)) } return content }

alpe · 2020-05-19T09:23:25Z

x/gov/types/proposal.go

@@ -48,9 +50,49 @@ func (p Proposal) String() string {
 	return string(out)
 }

+func (p Proposal) GetContent() Content {
+	content, ok := p.Content.GetCachedValue().(Content)


Same comment as with MsgSubmitProposal.GetContent

alpe · 2020-05-19T09:28:22Z

x/gov/types/proposal.go

+// UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces
+func (p Proposal) UnpackInterfaces(unpacker types.AnyUnpacker) error {
+	var content Content
+	return unpacker.UnpackAny(p.Content, &content)


Req: content may implement UnpackInterfacesMessage that needs to be called.

UnpackAny will call those recursively

…ov-proto-any

alexanderbez

ACK

fedekunze · 2020-05-19T19:12:16Z

codec/amino.go

+
+// Codec defines a wrapper for an Amino codec that properly handles protobuf
+// types with Any's
+type Codec struct {


will this be deprecated in the future in favor of AminoCodec? are there any difference between the 2?

In the ADR updates in this PR I noted that we would rename this to LegacyAmino. It is more or less deprecated. The difference between the two is that Marshaler expects types that implement ProtoMarshaler (i.e. generated by protoc). So any code using Marshaler is more or less ensured to be proto compatible. This legacy Codec is pure amino.

codec/types/interface_registry.go

fedekunze · 2020-05-19T19:17:43Z

docs/architecture/adr-019-protobuf-state-encoding.md

+* **all new code should use `Marshaler` which is compatible with both amino and
+protobuf**
+
+Also, before v0.39, `codec.Codec` will be renamed to `codec.LegacyAmino`.


we should this as a deprecation note on the codec.Codec godoc too.

Adding a deprecated note to the godoc

types/result.go

x/distribution/module.go

fedekunze · 2020-05-19T19:24:36Z

x/gov/types/codec.go

 	cdc.RegisterConcrete(&MsgSubmitProposal{}, "cosmos-sdk/MsgSubmitProposal", nil)
 	cdc.RegisterConcrete(MsgDeposit{}, "cosmos-sdk/MsgDeposit", nil)
 	cdc.RegisterConcrete(MsgVote{}, "cosmos-sdk/MsgVote", nil)
 	cdc.RegisterConcrete(&TextProposal{}, "cosmos-sdk/TextProposal", nil)
 }

+func RegisterInterfaces(registry types.InterfaceRegistry) {
+	registry.RegisterImplementations((*sdk.Msg)(nil),


what's the difference of RegisterImplementations with RegisterInterface?

RegisterInterface also adds a public facing name to the interface. It is intended to be called the first time an interface is declared. The fact that it also takes some implementations was added as a convenience so now they're pretty similar.

x/gov/types/proposal.go

fedekunze

Changes LGTM 👍 thanks @aaronc!

* Update x/gov to use Any * Fixes * Remove MsgSubmitProposalLegacy * Update CHANGELOG.md * Add RegisterInterfaces for x/distribution, x/params, & x/upgrade * Fix query JSON issue * Fix gov tests * Revert custom Any Equals * Re-remove types * Rename receivers * Fix imports in gov * Sort imports * Make amino JSON signing work with Any * Run proto-gen * Create full amino wrapper * Fix errors * Fixes * Fix tests * Test fixes * Fix tests * Linting * Update ADR 019 and CHANGELOG * Updated ADR 019 * Extract Marshal/UnmarshalProposal * fix error * lint * linting * linting * Update client/keys/parse.go Co-authored-by: Marko <marbar3778@yahoo.com> * linting * Update docs/architecture/adr-019-protobuf-state-encoding.md Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> * Update docs/architecture/adr-019-protobuf-state-encoding.md Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> * Address review feedback * Add godocs * Fix errors * fix errors * revert file * Address review feedback * Address review feedback * Stacktrace debug flag * Fix tests * Address review feedback Co-authored-by: sahith-narahari <sahithnarahari@gmail.com> Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

aaronc added 4 commits May 5, 2020 15:03

Update x/gov to use Any

ce24456

Fixes

df49ab4

Merge branch 'master' of github.com:cosmos/cosmos-sdk into aaronc/x-g…

51240fa

…ov-proto-any

Remove MsgSubmitProposalLegacy

3e5a787

aaronc added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: Proto Breaking Protobuf breaking changes: never don't do this! labels May 6, 2020

aaronc added 7 commits May 6, 2020 17:18

Update CHANGELOG.md

5264182

Add RegisterInterfaces for x/distribution, x/params, & x/upgrade

b2ca3a4

Fix query JSON issue

8dcfffb

Fix gov tests

42092cf

Revert custom Any Equals

8ea6a44

Merge branch 'master' of github.com:cosmos/cosmos-sdk into aaronc/x-g…

06056b3

…ov-proto-any

Re-remove types

7531d14

aaronc and others added 16 commits May 8, 2020 17:15

Rename receivers

ba6b151

Fix imports in gov

eab723e

Sort imports

3777926

Merge branch 'master' of github.com:cosmos/cosmos-sdk into aaronc/x-g…

426c04f

…ov-proto-any

Make amino JSON signing work with Any

bf05676

Merge branch 'master' of github.com:cosmos/cosmos-sdk into aaronc/x-g…

a4a35df

…ov-proto-any

Run proto-gen

519e1b9

Create full amino wrapper

84b4f63

Fix errors

5b2700c

Fixes

06c2d03

Fix tests

ad482e4

Test fixes

0ff09a4

Merge branch 'master' of github.com:cosmos/cosmos-sdk into aaronc/x-g…

7b632fa

…ov-proto-any

Fix tests

33b04ff

Linting

72a89a1

Update ADR 019 and CHANGELOG

f6fa2ba

aaronc added 5 commits May 18, 2020 15:38

Merge branch 'master' into aaronc/x-gov-proto-any

600ac6a

Fix errors

22d8af7

Merge remote-tracking branch 'origin/aaronc/x-gov-proto-any' into aar…

5d289e1

…onc/x-gov-proto-any

fix errors

a005332

revert file

c1bad36

aaronc requested review from alexanderbez and removed request for hschoenburg May 18, 2020 20:11

alexanderbez reviewed May 18, 2020

View reviewed changes

codec/amino.go Show resolved Hide resolved

alexanderbez reviewed May 18, 2020

View reviewed changes

types/result.go Show resolved Hide resolved

alexanderbez suggested changes May 18, 2020

View reviewed changes

x/gov/keeper/proposal.go Show resolved Hide resolved

x/gov/keeper/proposal.go Outdated Show resolved Hide resolved

x/gov/keeper/proposal.go Outdated Show resolved Hide resolved

x/gov/keeper/proposal.go Outdated Show resolved Hide resolved

x/gov/types/msgs.go Show resolved Hide resolved

alpe reviewed May 19, 2020

View reviewed changes

aaronc added 2 commits May 19, 2020 14:06

Merge branch 'master' of github.com:cosmos/cosmos-sdk into aaronc/x-g…

b58c42a

…ov-proto-any

Address review feedback

c09e298

alexanderbez approved these changes May 19, 2020

View reviewed changes

aaronc added 3 commits May 19, 2020 14:31

Address review feedback

0e159e7

Stacktrace debug flag

5ed861d

Fix tests

2e14d48

fedekunze reviewed May 19, 2020

View reviewed changes

Address review feedback

72ead83

fedekunze approved these changes May 19, 2020

View reviewed changes

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

mergify bot merged commit 70767c8 into master May 19, 2020

mergify bot deleted the aaronc/x-gov-proto-any branch May 19, 2020 20:17

clevinson mentioned this pull request May 26, 2020

Update groups module to use Any in proto encoding regen-network/cosmos-modules#69

Closed

clevinson added this to the v0.39 milestone Jun 11, 2020

odeke-em mentioned this pull request Feb 12, 2021

x/gov/simulation: NewDecodeStore unmarshals wrong proposal due to typo in value source #8570

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update x/gov to use Any #6147

Update x/gov to use Any #6147

aaronc commented May 5, 2020 •

edited

Loading

codecov bot commented May 8, 2020 •

edited

Loading

aaronc commented May 18, 2020

alpe left a comment

alpe May 19, 2020

aaronc May 19, 2020

alpe May 20, 2020

alpe May 19, 2020

aaronc May 19, 2020

alpe May 19, 2020

aaronc May 19, 2020

alpe May 19, 2020

alpe May 19, 2020

aaronc May 19, 2020

alpe May 20, 2020

alpe May 19, 2020

alpe May 19, 2020

aaronc May 19, 2020

alexanderbez left a comment

fedekunze May 19, 2020

aaronc May 19, 2020

fedekunze May 19, 2020

aaronc May 19, 2020

fedekunze May 19, 2020

aaronc May 19, 2020

fedekunze left a comment

Update x/gov to use Any #6147

Update x/gov to use Any #6147

Conversation

aaronc commented May 5, 2020 • edited Loading

codecov bot commented May 8, 2020 • edited Loading

Codecov Report

aaronc commented May 18, 2020

alpe left a 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

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

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

Choose a reason for hiding this comment

alexanderbez left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

aaronc commented May 5, 2020 •

edited

Loading

codecov bot commented May 8, 2020 •

edited

Loading