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 MarshalBinary/UnmarshalBinary interface support #300

Open
wants to merge 12 commits into
base: ismail/any_amino
Choose a base branch
from
Open

Add MarshalBinary/UnmarshalBinary interface support #300

wants to merge 12 commits into from

Conversation

jordaaash
Copy link
Contributor

This PR provides a mechanism to completely override the Amino encoding of a type by implementing the BinaryMarshaler and BinaryUnmarshaler interfaces.

Amino prefix bytes will still be prepended. Implementing MarshalAmino/UnmarshalAmino is not required. This should unblock tendermint/tendermint#4245

amino.go Outdated Show resolved Hide resolved
amino.go Outdated Show resolved Hide resolved
binary_encode_override_test.go Outdated Show resolved Hide resolved
binary_encode_override_test.go Show resolved Hide resolved
amino.go Show resolved Hide resolved
Copy link

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

testedACK -- I've tested this in Tendermint and it accomplishes what is desired. I've add some surface-level feedback, but ultimately I'd like an ACK from @jaekwon and @zmanian.

amino.go Show resolved Hide resolved
amino.go Outdated Show resolved Hide resolved
binary_encode_override_test.go Show resolved Hide resolved
amino.go Outdated Show resolved Hide resolved
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@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 -- pending approval by Jae & Zaki and CI fixing.

amino.go Outdated Show resolved Hide resolved
@jaekwon
Copy link
Contributor

jaekwon commented Dec 24, 2019

Relevant: #301 - "The binary and json were intentionally written to mirror the code structure, so this kind of translation is easy."

Given the current impl, the divergence in JSON hints at divergence in function, whereby this optimization doesn't work for embedded structs (whereas such does work for json). In #301 what I want to convey is that we should continue to maintain code structure parity between binary and json, so that we solve common issues for both and reduce potential bugs. Once the implementations diverge, it will become difficult or impossible to keep in sync.

Anyways, great case study; thank you for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants