Skip to content
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

Update x/gov to use Any #6147

Merged
merged 52 commits into from
May 19, 2020
Merged

Update x/gov to use Any #6147

merged 52 commits into from
May 19, 2020

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented May 5, 2020

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)

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

codecov bot commented May 8, 2020

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           

@aaronc
Copy link
Member Author

aaronc commented May 18, 2020

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

@aaronc aaronc requested review from alexanderbez and removed request for hschoenburg May 18, 2020 20:11
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
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

codec/amino.go Outdated
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think I understand what you mean and addressed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ups, should have been "call MarshalBinaryBare". sorry

codec/types/amino_compat.go Outdated Show resolved Hide resolved
// UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces
func (s SearchTxsResult) UnpackInterfaces(unpacker types.AnyUnpacker) error {
for _, tx := range s.Txs {
err := types.UnpackInterfaces(tx, unpacker)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Same as SearchTxsResult. UnpackInterfaces

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in general it will be implemented on Tx

@@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better approach you would suggest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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
}

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

func (p Proposal) GetContent() Content {
content, ok := p.Content.GetCachedValue().(Content)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as with MsgSubmitProposal.GetContent

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnpackAny will call those recursively

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
* **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`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a deprecated note to the godoc

types/result.go Show resolved Hide resolved
x/distribution/module.go Show resolved Hide resolved
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),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference of RegisterImplementations with RegisterInterface?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM 👍 thanks @aaronc!

@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label May 19, 2020
@mergify mergify bot merged commit 70767c8 into master May 19, 2020
@mergify mergify bot deleted the aaronc/x-gov-proto-any branch May 19, 2020 20:17
@clevinson clevinson added this to the v0.39 milestone Jun 11, 2020
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Encoding T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: Proto Breaking Protobuf breaking changes: generally don't do this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants