-
Notifications
You must be signed in to change notification settings - Fork 328
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
Conversation
Rebased. Just keeping things fresh 🥗 |
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! |
@dakom I saw that comment ;) The code comments were indeed stale, fixed now |
There was a problem hiding this 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:
-
maybe internal tests, including those in checksum, binary, etc. should all go though the
[to | from]_msgpack()
abstraction, rather than hittingrmp_serde
directly? -
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 usingrmp_serde
in other tests :)
There was a problem hiding this 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
Yeah, I see now that this is what the 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
Our msgpack API isn't meant to support non-named payloads, but always enforce the named variant. |
@webmaster128 Alright, seems we're good to merge when you're happy! |
This PR does two things:
cosmwasm_core::{Binary, HexBinary}
to (de)serialize directly as binary blobs when used together with a compact encoding (meaning one that returnsfalse
fromis_human_readable
), andcosmwasm_std::{from_msgpack, to_msgpack_vec, to_msgpack_binary}
.Breakage
If we're being rigorous, I think the changes tocosmwasm_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