w3f / schnorrkel

Schnorr VRFs and signatures on the Ristretto group
BSD 3-Clause "New" or "Revised" License
314 stars 93 forks source link

Use `serde_bytes` #67

Closed ghost closed 3 years ago

ghost commented 3 years ago

This PR addresses the issue outlined in #66.

2 new dependencies were added: serde_bytes to actually use when serializing and deserializing, and cfg_if, so deserialization can use Bytes when in no alloc context and BytesBuf otherwise, since the latter allows to deserialize from more data types where the size is not known or it is not borrowable, such as sequences.

serde has also been renamed in the manifest as serde_crate, similar to curve25519-dalek's manifest, to allow bundling serde_bytes and serde in a single feature flag.

Tests were added to check compatibility with serde_json

Closes #66

burdges commented 3 years ago

Appears we give up the ability to work without serde_bytes, but this should be no sacrifice because its designed to handle all the corner cases?

If so, that what do we benefit by the multiple features? We just cannot invoke multiple optional dependencies without three features?

ghost commented 3 years ago

Yeah serde_bytes handles more cases than the previous implementation, so we are good with using it always.

We need the feature because otherwise to properly enable serde support we need to specify 2 features, serde and serde_bytes, which I don't think is ideal, and would also break the current documentation.

I don't see the 3 features you mention? You mean serde, serde_crate and serde_bytes, cfg_if? Well the user are not supposed to enable serde_crate, serde_bytes or cfg_if manually, but as far as I know this is the only way to group up optional dependencies together.

I use cfg_if to provide different deserialization based on the enabled features, since otherwise we'd drop support for no alloc

burdges commented 3 years ago

It's ready for merge?

ghost commented 3 years ago

I'd say so, I ran cargo test with different feature sets and it's working :)

Let me know if you have any other concern to address before merging

burdges commented 3 years ago

LGTM