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

Add amino compatibility layer for proto Any #6151

Merged
merged 9 commits into from
May 6, 2020
Merged

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented May 5, 2020

This PR adds backwards compatibility for Amino binary and JSON encoding for structs that choose to encode interfaces with google.protobuf.Any instead of plain go interfaces.

Structs that follow the guidelines laid out in #6081 (implementation of UnpackInterfaceMessages) should have identical Amino binary and JSON encoding as the previous structs (without Any) if AminoCodec is used to marshal/unmarshal them.

@auto-comment
Copy link

auto-comment bot commented May 5, 2020

👋 Thanks for creating a PR!

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.


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)

Thank you for your contribution to the Cosmos-SDK! 🚀

@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #6151 into master will increase coverage by 0.04%.
The diff coverage is 68.65%.

@@            Coverage Diff             @@
##           master    #6151      +/-   ##
==========================================
+ Coverage   54.67%   54.72%   +0.04%     
==========================================
  Files         441      443       +2     
  Lines       26592    26703     +111     
==========================================
+ Hits        14539    14612      +73     
- Misses      11034    11052      +18     
- Partials     1019     1039      +20     

@aaronc aaronc marked this pull request as ready for review May 6, 2020 19:13
@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label May 6, 2020
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.

utACK

codec/any_test.go Show resolved Hide resolved
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.

utACK. To be honest, and it could very well be just me, but I find this level of indirection of codec passing/usage extremely confusing. I had to go back and forth between files/types numerous times to really see what's happening.

But as long as it works (which it seems to) and the user-level API remains clean and minimal, that is the important part -- I doubt this code will change much.

@mergify mergify bot merged commit 9d022c1 into master May 6, 2020
@mergify mergify bot deleted the aaronc/amino-any branch May 6, 2020 20:47
@aaronc
Copy link
Member Author

aaronc commented May 6, 2020

Yeah, so a few things make it complex:

  • in order to marshal/unmarshal amino interfaces we need an actual amino codec instance
  • the Marshal/UnmarshalAmino and Marshal/UnmarshalJSON methods don't take any sort of context value that we could overload to pass through an amino codec
  • amino unmarshal actually requires an interface pointer to be passed to it (i.e. you can't just pass interface{} you need to pass something registered like sdk.Msg), so doing this via reflection would be hard

So this solution basically takes advantage of the fact that the UnpackInterface methods we're using for Any, pass a codec-like thing around that expects an interface pointer just like amino expects. So turned out I could overload that functionality to make Anys work with amino without any hacky things like a global amino or gls and without needing a separate set of amino-only types.

Does that help explain things at all?

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants