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

hybrid-array: Added serde impls for Array #979

Closed
wants to merge 9 commits into from

Conversation

rozbb
Copy link
Contributor

@rozbb rozbb commented Nov 1, 2023

I'd love to switch hpke over to hybrid-array, since it's becoming pretty hard to manually audit generic-array, and we expose it so it's not like we can get away with leaving it old forever.

One of the really convenient things that generic-array provides, though, is Serialize/Deserialize impls for GenericArray. We use this in our Xyber impl. The only other option for autoderiving these traits is serde-bigarray (upstream support is not coming any time soon), and I'd rather not bring it in.

This PR should bring hybrid-array a little closer to feature-parity with generic-array. I don't love the unsafe stuff here, but it's pretty minimal and well-documented, and also has a clear roadmap for removing it once certain functions stabilize.

@rozbb
Copy link
Contributor Author

rozbb commented Nov 1, 2023

It looks like some tests whose MSRV hasn't been bumped have a problem with this Cargo.toml

@rozbb rozbb changed the title Added serde impls for Array hybrid-array: Added serde impls for Array Nov 1, 2023
@tarcieri
Copy link
Member

tarcieri commented Nov 1, 2023

We've been there and back again with the various tradeoffs of serde support across formats. See the recently merged:

RustCrypto/formats#1111

RustCrypto/formats#1112

There is so much errata involved here across various serde-based format implementations that I have trouble keeping it all in my head. The array serializers look like this: https://github.com/RustCrypto/formats/blob/master/serdect/src/array.rs

We tried to use the tuple serializers initially to avoid the length prefix, but it turns out those have terrible interactions with MessagePack because serde lacks the ability to describe homogenously typed tuples. serialize_bytes exists specifically for handling the byte array case, which is far and away our biggest use case, but there is no general-case solution for fixed size arrays: serde-rs/serde#2120 (comment)

I guess the question is whether we should worry about these particular issues in the context of this crate, namely formats like MessagePack will do a lot of branching on the data and encode it oddly because they have to encode type information for each entry in the tuple, rather than storing it in one place.

p.s. I am very, very tired of reviewing serde-related code. Just when I think I understand what's happening I discover yet another fun new surprise.

@tarcieri
Copy link
Member

tarcieri commented Nov 2, 2023

We can definitely bump MSRV on fiat-constify, FWIW.

@rozbb
Copy link
Contributor Author

rozbb commented Nov 2, 2023

I'm kind of in agreement here. I think we don't necessarily have to expect [u8; N] to be serializable. Or at the very least I can live without it and just write my own impls for my use case.

If you want to keep ArrayExt::try_from_fn, then I can remove the rest of this PR. Otherwise, feel free to close outright.

@rozbb
Copy link
Contributor Author

rozbb commented Nov 2, 2023

In this same vein. I thing maybe hpke shouldn't be doing serde impls either, for the same reason. It has to_bytes and from_bytes. I think that should be sufficient. If someone wants a specific format then they can choose.

@tarcieri
Copy link
Member

hybrid-array has moved to https://github.com/RustCrypto/hybrid-array

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.

2 participants