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

Support deserializing from borrowed or owned bytes, too #668

Merged
merged 6 commits into from Aug 31, 2022

Conversation

jsdw
Copy link
Contributor

@jsdw jsdw commented Aug 26, 2022

Support deserializing from bytes, too (see #669).

Closes #669

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Could you also bump the patch minor version and update the changelog? Thanks!

primitive-types/impls/serde/src/serialize.rs Outdated Show resolved Hide resolved
@luketchang
Copy link

luketchang commented Aug 26, 2022

Could we also add support for deserializing from a serde sequence? When substrate chain returns a storage query result for a primitive_types::H256, it comes in the form of a sequence of Primitive::U128 meant to be a Vec<u8>

Roughly the encoding of this structure:

#[derive(Clone, Copy, Serialize, Deserialize)]
pub struct NomadBase {
    /// State
    pub state: NomadState,
    /// Local domain
    pub local_domain: u32,
    /// Committed root
    pub committed_root: H256,
    /// Updater
    pub updater: H160,
}


Value { value: Composite(Named([
  (\"state\", Value { value: Variant(Variant { name: \"Active\", values: Unnamed([]) }), context: TypeId(379) }), 
  
  (\"local_domain\", Value { value: Primitive(U128(0)), context: TypeId(4) }), 
  
  (\"committed_root\", Value { value: Composite(Unnamed([Value { value: Composite(Unnamed([
     Value { value: Primitive(U128(0)), context: TypeId(2) }, 
     Value { value: Primitive(U128(0)), context: TypeId(2) },
     ...
     Value { value: Primitive(U128(0)), context: TypeId(2) }, 
     Value { value: Primitive(U128(0)), context: TypeId(2) }]
  )), context: TypeId(1) }])), context: TypeId(10) }), 

   (\"updater\", Value { value: Composite(Unnamed([Value { value: Composite(Unnamed([
     Value { value: Primitive(U128(0)), context: TypeId(2) }, 
     Value { value: Primitive(U128(0)), context: TypeId(2) },
     ...
     Value { value: Primitive(U128(0)), context: TypeId(2) }, 
     Value { value: Primitive(U128(0)), context: TypeId(2) }
   ])), context: TypeId(77) }])), context: TypeId(76) })])), 

context: TypeId(378) }"}

@jsdw
Copy link
Contributor Author

jsdw commented Aug 31, 2022

I've added support for sequences of bytes, too. That seemed natural and reasonable to support (and indeed it is what scale_value::Values can work best with).

@ordian re version bumping and changelog:

The changelog has an unreleased seciton already and the version isn't bumped from that last change.

Should I either:

  1. Bump the version, add my entry and turn "unreleased" into changelog for that version, or
  2. Add my entry to "unreleased" and leave it to you or somebody else to bump the version and modify the changelog when they do a release?
  3. Other :)

@ordian
Copy link
Member

ordian commented Aug 31, 2022

Should I either:

1. Bump the version, add my entry and turn "unreleased" into changelog for that version, or

2. Add my entry to "unreleased" and leave it to you or somebody else to bump the version and modify the changelog when they do a release?

I was suggesting 1, but if you're more comfortable with 2, that works too :)

@jsdw
Copy link
Contributor Author

jsdw commented Aug 31, 2022

I'm happy with 1; happy to just publish the crate too once it's merged but wanted to check that there wasn't any particular release approach/policy to follow if so :)

Edit: Ah I found the CONTRIBUTING.md! One thing to note is that a publish of this crate (impl-serde) would also require a publish and update of a bunch of other crates that depend on older versions of this to actually put it into use, though. Perhaps I'll just publish this one first and can worry about updating the other crates in this repo that depend on it later; wdyt?

@ordian
Copy link
Member

ordian commented Aug 31, 2022

Perhaps I'll just publish this one first and can worry about updating the other crates in this repo that depend on it later; wdyt?

Yup, I can take care of the rest. It seems to be another semver-pocalypse which would necessitate a breaking bump for almost all crates.

@ordian ordian merged commit 706c989 into master Aug 31, 2022
@ordian ordian deleted the jsdw-deser-from-hex-or-bytes branch August 31, 2022 17:53
@jsdw
Copy link
Contributor Author

jsdw commented Sep 1, 2022

I was wondering whether we could have gotten away with a patch bump instead?

I've also been mulling over this and I think I'd like to add a followup PR to accomodate another deserialization woe (basically; the type info for types like H256 indicates that they are some newtype surrounding an array of bytes, and so it would be nice to try and support that, too, in the deserialization, rather than the current stuff which is all good but pretends that we're going straight to an array of bytes.)

@ordian
Copy link
Member

ordian commented Sep 1, 2022

I was wondering whether we could have gotten away with a patch bump instead?

I've bumped patch versions on MSRV bump previously and people were not happy about that: #620. The MSRV is not caused by your PR, but an unreleased change.

I understand you want to get this out quickly and easily, but we gotta do it the right way.

I've also been mulling over this and I think I'd like to add a followup PR to accomodate another deserialization woe (basically; the type info for types like H256 indicates that they are some newtype surrounding an array of bytes, and so it would be nice to try and support that, too, in the deserialization, rather than the current stuff which is all good but pretends that we're going straight to an array of bytes.)

It seems the crate hasn't been published, so feel free to submit additional PRs and change the date of the release in the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[primitive-types] support deserializing H256 et al from bytes as well as hex strings
4 participants