Skip to content

Add MarshalBinary/UnmarshalBinary interface support#300

Open
jordaaash wants to merge 12 commits intotendermint:ismail/any_aminofrom
jordaaash:jordansexton/any_amino/binary_marshaler
Open

Add MarshalBinary/UnmarshalBinary interface support#300
jordaaash wants to merge 12 commits intotendermint:ismail/any_aminofrom
jordaaash:jordansexton/any_amino/binary_marshaler

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

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.

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.

@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