You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR removes serde support. This is following a conversation elsewhere about providing serde impls for byte slices. The argument for removing serde impls is:
If you derive the plain [T; N] serialization, or (T, T, ..., T) serialization, then you miss out on optimizations for formats with byteslice representations. This has a horrible slowdown, which I've experience with wasm_bindgen. This is why serde_bytes exists, and it cannot be merged into upstream (see above conversation)
The choice of whether or not to use a length tag on the serialization depends on the format you're serializing to (see above conversation)
The current p256 serde::Serialize implementation is the DER encoding of the public key. This is not something I noticed, and certainly doesn't match the raw byte representation of the other public key serializations. I should not have autoderived without looking. I don't think this is true. It's the bytes given by to_bytes().
In short, people have different needs when it comes to serializing HPKE values with serde, and any choice necessarily excludes other options. So rather than making that choice for the user, this crate should instead just expose its normal Serializable trait (with methods to_bytes and write_exact), and let the user implement what they need.
Looking for feedback on this, since it's removing a feature that might be in use currently.
The serde data model lacks the concept of a homogeneously-typed fixed-size array/tuple. This means there is no universally agreed upon compact representation of a fixed-size byte array. The best you can do is using serialize_bytes on the underlying serializer, but this will add a length prefix even if the length is known in advance.
The only way to avoid a length prefix is to use serialize_tuple, however for some message formats like MessagePack this will include type information with each element, leading to a [u8; N] serialization which is much larger than N bytes.
Ya exactly. I notice you said in the linked thread you were switching to serialize_bytes. In which crate is that happening? I don't think it's necessarily a good option here, but I'm curious to see.
It's designed specifically for constant-time bytes handling. Using serde_bytes was the only way to ensure encoding in MessagePack isn't data-dependent. The additional and often unnecessary length prefix is unfortunate.
It looks like there is one project that is using this, here and here.
I also realized, I don't think the p256 impl is DER-encoded. The serde impls appear to just be a thin wrapper around to_bytes, which is good. So this all drops to the GenericArray serde impls. So I guess the move here is to document everything and offer some sample code for doing the conversion. Shouldn't be hard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR removes serde support. This is following a conversation elsewhere about providing serde impls for byte slices. The argument for removing serde impls is:
[T; N]serialization, or(T, T, ..., T)serialization, then you miss out on optimizations for formats with byteslice representations. This has a horrible slowdown, which I've experience withwasm_bindgen. This is whyserde_bytesexists, and it cannot be merged into upstream (see above conversation)The current p256I don't think this is true. It's the bytes given byserde::Serializeimplementation is the DER encoding of the public key. This is not something I noticed, and certainly doesn't match the raw byte representation of the other public key serializations. I should not have autoderived without looking.to_bytes().In short, people have different needs when it comes to serializing HPKE values with
serde, and any choice necessarily excludes other options. So rather than making that choice for the user, this crate should instead just expose its normalSerializabletrait (with methodsto_bytesandwrite_exact), and let the user implement what they need.Looking for feedback on this, since it's removing a feature that might be in use currently.
cc @marmeladema @bwesterb @tarcieri