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

Provide MessagePack serialization #2118

Merged
merged 14 commits into from
Aug 5, 2024
Merged

Provide MessagePack serialization #2118

merged 14 commits into from
Aug 5, 2024

Conversation

uint
Copy link
Contributor

@uint uint commented Apr 18, 2024

This PR does two things:

  • modify cosmwasm_core::{Binary, HexBinary} to (de)serialize directly as binary blobs when used together with a compact encoding (meaning one that returns false from is_human_readable), and
  • provides a set of new MessagePack-related functions that mirror the JSON API: cosmwasm_std::{from_msgpack, to_msgpack_vec, to_msgpack_binary}.

Breakage

If we're being rigorous, I think the changes to cosmwasm_core::{Binary, HexBinary} are breaking if used for storage with a binary encoding. Maybe we should release this with a major version bump or deprecate these types in favor of some new ones. @webmaster128 you're the decision-maker here.

This is breaking if we assume someone out there has used the binary/checksum types with binary encodings.

We're not bumping the major version for this since as far as we're concerned, that use case has always been unsupported. Still, we're going to release this with a minor version bump, and before it's released we're going to provide some "guard rails" via #2125, #2126, #2127.

TODO

  • changelog

@uint uint requested a review from webmaster128 April 18, 2024 18:09
@uint uint changed the base branch from main to prep-msgpack April 24, 2024 13:06
Base automatically changed from prep-msgpack to main April 25, 2024 09:01
@uint
Copy link
Contributor Author

uint commented Jun 12, 2024

Rebased.

Just keeping things fresh 🥗

@webmaster128
Copy link
Member

Hey Tom, could you update this, add a CHANGELOG entry and mark it ready for review? We can merge it to main over the next couple of weeks. No rush though

@uint uint marked this pull request as ready for review August 5, 2024 12:26
@uint
Copy link
Contributor Author

uint commented Aug 5, 2024

Hey Tom, could you update this, add a CHANGELOG entry and mark it ready for review? We can merge it to main over the next couple of weeks. No rush though

All done!

@uint
Copy link
Contributor Author

uint commented Aug 5, 2024

@dakom I saw that comment ;) The code comments were indeed stale, fixed now

Copy link
Contributor

@dakom dakom left a comment

Choose a reason for hiding this comment

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

LGTM, left a couple minor comments.

Also a couple high-level callouts, but nothing that should prevent merging imho:

  1. maybe internal tests, including those in checksum, binary, etc. should all go though the [to | from]_msgpack() abstraction, rather than hitting rmp_serde directly?

  2. I don't totally follow the to_vec vs. to_vec_named distinction... not sure if it needs more test coverage to handle msgpack payloads in the wild that may use the non-named version (though there are some tests that cover this, as it happens, due to point (1) above - i.e. manually using rmp_serde in other tests :)

packages/std/src/msgpack.rs Outdated Show resolved Hide resolved
packages/std/src/msgpack.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

Looks good to me too, just one small question, not a blocker

packages/std/src/binary.rs Show resolved Hide resolved
@uint
Copy link
Contributor Author

uint commented Aug 5, 2024

  1. maybe internal tests, including those in checksum, binary, etc. should all go though the [to | from]_msgpack() abstraction, rather than hitting rmp_serde directly?

Yeah, I see now that this is what the serde_works test does and I'm being inconsistent.

I'm sort of not a fan of that though. It unnecessarily couples two of our internal modules. The point of these unit tests isn't to test our msgpack API, but how Checksum and co behave with "compact" and "human readable" serde encodings.

  1. I don't totally follow the to_vec vs. to_vec_named distinction... not sure if it needs more test coverage to handle msgpack payloads in the wild that may use the non-named version (though there are some tests that cover this, as it happens, due to point (1) above - i.e. manually using rmp_serde in other tests :)

to_vec serializes structs as arrays, to_vec_named serializes them as something like JSON objects. This means the "named" variant encodes field names, so then reordering or adding a field in the middle is possible.

Our msgpack API isn't meant to support non-named payloads, but always enforce the named variant.

@uint
Copy link
Contributor Author

uint commented Aug 5, 2024

@webmaster128 Alright, seems we're good to merge when you're happy!

@webmaster128 webmaster128 merged commit fcc0334 into main Aug 5, 2024
32 checks passed
@webmaster128 webmaster128 deleted the message-pack branch August 5, 2024 20:24
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.

4 participants