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

Make MarshalBinary the non-length prefixed default #222

Merged
merged 3 commits into from
Oct 15, 2018

Conversation

liamsi
Copy link
Contributor

@liamsi liamsi commented Sep 4, 2018

resolves #219

as per discussion with @jaekwon I've added back the WIP in the title:

it seems like we want to migrate in 2 steps, marshalbinary -> marshalbinarylengthprefixed first, then marshalbinarybare -> marshalbinary … cuz otherwise during migration we can miss switching marshalbinary -> marshalbinarybare and cause nasty bugs.
or we can just keep marshalbinarybare and rename marshalbinary -> marshalbinarylengthprefixed only.

I think the 2 steps approach works fine; keeping marshalbinarybare is simpler but less explicit (which the default is).

Currently, this PR only does the marshalbinary -> marshalbinarylengthprefixed step as explained above.

 - and have a MarshalBinaryLengthPrefixed instead
@liamsi liamsi changed the title [WIP]: Make MarshalBinary the non-length encoded default Make MarshalBinary the non-length prefixed default Sep 4, 2018
@liamsi liamsi changed the title Make MarshalBinary the non-length prefixed default [WIP] Make MarshalBinary the non-length prefixed default Sep 5, 2018
liamsi added a commit to tendermint/tendermint that referenced this pull request Sep 8, 2018
liamsi added a commit to liamsi/cosmos-sdk that referenced this pull request Sep 8, 2018
@liamsi liamsi changed the title [WIP] Make MarshalBinary the non-length prefixed default Make MarshalBinary the non-length prefixed default Sep 8, 2018
Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM

@liamsi
Copy link
Contributor Author

liamsi commented Sep 10, 2018

OK to merge @jaekwon @ebuchman ?

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.

🍰 🌮 🍉

@liamsi liamsi merged commit 136ef14 into develop Oct 15, 2018
ebuchman pushed a commit to tendermint/tendermint that referenced this pull request Oct 25, 2018
* catch up with amino changes in
tendermint/go-amino#222

* WIP: update to amino v0.13.0

* update to fixed amino release
liamsi added a commit to liamsi/cosmos-sdk that referenced this pull request Oct 29, 2018
- initially this was also renamed but we decided to do this in an
incremental, two step approach

(see discussion: tendermint/go-amino#222)
@liamsi liamsi deleted the non_len_prefixed_default branch October 30, 2018 18:10
kfangw pushed a commit to kfangw/blockchain that referenced this pull request Jun 10, 2019
* catch up with amino changes in
tendermint/go-amino#222

* WIP: update to amino v0.13.0

* update to fixed amino release
NexZhu pushed a commit to crpt/go-merkle that referenced this pull request Dec 1, 2021
* catch up with amino changes in
tendermint/go-amino#222

* WIP: update to amino v0.13.0

* update to fixed amino release
cmwaters pushed a commit to celestiaorg/go-square that referenced this pull request Dec 19, 2023
* catch up with amino changes in
tendermint/go-amino#222

* WIP: update to amino v0.13.0

* update to fixed amino release
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.

3 participants