-
Notifications
You must be signed in to change notification settings - Fork 605
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
core: make StoreKey, StoreValue and FunctionArgs serde serialisable #8985
Conversation
Derive serialisation code for StoreKey, StoreVlaue and FunctionArgs primitive types and then use those types more in various structures which so far has been using raw `Vec<u8>` instead. This improves self-documentation of the code (the type of the struct field indicates what it is) and moves base64 format declaration to the primitive types rather than having to annotate individual structures with `#[serde(with = ...)]`. On the flip side, it does add a couple `.into()` calls when converting between one of the aforementioned types and `Vec<u8>`.
I'm honestly not sure if this is an improvement. I’ll leave it up for you guys to decide. ;) |
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.
I like the explicit typing! But let's wait for the data platform team's input regarding breaking changes in primitives.
pub key: StoreKey, | ||
pub value: StoreValue, |
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.
Note: This looks like a breaking change for the published primitives crate
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.
@khorolets would this be a problem? Serialized, the data should look the same, it's only a breaking change on the code level, as constructors needs to be updated.
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.
Doesn't seem like a problem for us. What nearcore version are these changes expected to land at? I want us to check the underlying stuff and ensure we do not lose data or prevent services from being stuck 😬
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.
@khorolets it will land in neard binary version 1.35 if we merge it within the next couple of weeks. For the crates version, I guess we will have to bump one from the current version and would end up with 0.17.
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.
Sounds good to me
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.
@mina86 please ACK if you want me to add the auto-merge label
Yes please. |
Derive serialisation code for StoreKey, StoreVlaue and FunctionArgs
primitive types and then use those types more in various structures
which so far has been using raw
Vec<u8>
instead.This improves self-documentation of the code (the type of the struct
field indicates what it is) and moves base64 format declaration to the
primitive types rather than having to annotate individual structures
with
#[serde(with = ...)]
.On the flip side, it does add a couple
.into()
calls when convertingbetween one of the aforementioned types and
Vec<u8>
.