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

Leave serde as the only serialization method #110

Merged
merged 4 commits into from
Jan 15, 2023

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Jan 6, 2023

Fixes #109
Fixes #95 (or at least invalidates it)

See nucypher/nucypher-core#41 for the preview of how it will affect nucypher-core

Changes:

  • Remove HasTypeName trait, and use any::type_name() where some name is still required.
  • Add serde::Serialize/Deserialize implementations for Caspule, (Verified)CapsuleFrag, (Verified)KeyFrag, PublicKey, and Signature.
  • Remove VerifiedCapsuleFrag::from_verified_bytes() and VerifiedKeyFrag::from_verified_bytes(). For this behavior, deserialize into CapsuleFrag or KeyFrag and call skip_verification().
  • Remove DeserializableFromArray, RepresentableAsArray, SerializableToArray, SerializableToSecretArray.
  • Add DefaultSerialize/DefaultDeserialize for Umbral objects (Caspule, (Verified)CapsuleFrag, (Verified)KeyFrag) which uses MessagePack via serde (gated by default-serialization feature). This is what the serialization methods in the bindings use.
  • Add to_compressed_bytes()/try_from_compressed_bytes() for PublicKey to give access to non-serde representation (just 33 bytes). These are exposed in the bindings in lieu of the "default" methods (e.g. __bytes__() or toBytes()).
  • Add to_der_bytes()/try_from_der_bytes() for Signature to give access to non-serde representation. These are exposed in the bindings in lieu of the "default" methods (e.g. __bytes__() or toBytes()).
  • Add SecretKeyFactory::make_secret() to provide a more straightforward replacement of SecretKeyFactory::make_secret_key()->SecretKey::to_secret_bytes().
  • Capsule no longer implements Copy (it's ~128 bytes, I don't think it's wise to have it)

Internal changes:

  • Add Serialize/Deserialize implementations for CurveScalar and CurvePoint.
  • Hash in Python bindings is calculated just with SHA2(byte_representation), instead of bothering GIL and Python builtins.

Remains to be decided:

  • serde_bytes module may be better extracted into its own crate or added to some existing bytes-handling crate for serde

@fjarri fjarri force-pushed the serde-only branch 3 times, most recently from cef0307 to 073f472 Compare January 6, 2023 07:33
@fjarri fjarri added WASM Related to WASM bindings Python Related to Python bindings API Related to public API ABI Changes the format of serialized objects labels Jan 7, 2023
@fjarri fjarri marked this pull request as ready for review January 7, 2023 00:17
@codecov-commenter
Copy link

Codecov Report

Merging #110 (b3fd37f) into master (fbf2c03) will decrease coverage by 2.67%.
The diff coverage is 44.38%.

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
- Coverage   51.12%   48.44%   -2.68%     
==========================================
  Files          17       17              
  Lines        1160      997     -163     
==========================================
- Hits          593      483     -110     
+ Misses        567      514      -53     
Impacted Files Coverage Δ
umbral-pre/src/bindings_python.rs 0.00% <0.00%> (ø)
umbral-pre/src/bindings_wasm.rs 0.00% <0.00%> (ø)
umbral-pre/src/capsule_frag.rs 79.68% <0.00%> (-5.27%) ⬇️
umbral-pre/src/dem.rs 78.37% <ø> (ø)
umbral-pre/src/params.rs 100.00% <ø> (ø)
umbral-pre/src/secret_box.rs 80.00% <0.00%> (-20.00%) ⬇️
umbral-pre/src/serde_bytes.rs 41.86% <ø> (-16.48%) ⬇️
umbral-pre/src/traits.rs 0.00% <0.00%> (-58.00%) ⬇️
umbral-pre/src/keys.rs 70.10% <61.29%> (+1.17%) ⬆️
umbral-pre/src/key_frag.rs 84.94% <77.77%> (-1.52%) ⬇️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@theref
Copy link

theref commented Jan 9, 2023

what does der in to_der_bytes()/try_from_der_bytes() mean?

@fjarri
Copy link
Contributor Author

fjarri commented Jan 9, 2023

It's the standard format for serializing signatures or certificates, see https://en.wikipedia.org/wiki/X.690#DER_encoding

Comment on lines +52 to +53
// TODO: for a slightly better user experience we can remove qualifiers here,
// because the returned string will be something like "crate_name::module_name::TypeName"
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally have nothing against fully-qualified names in this case.

Copy link
Contributor

@piotr-roslaniec piotr-roslaniec left a comment

Choose a reason for hiding this comment

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

LGTM

serde_bytes module may be better extracted into its own crate or added to some existing bytes-handling crate for serde

Rings a bell, serde-bytes-repr

@fjarri
Copy link
Contributor Author

fjarri commented Jan 9, 2023

Yes, there is a bunch - see serde-rs/bytes#37 (comment). serde-bytes-repr in particular requires one to write boilerplate, which I would like to avoid.

@fjarri fjarri merged commit d215154 into nucypher:master Jan 15, 2023
@fjarri fjarri deleted the serde-only branch January 15, 2023 00:36
fjarri added a commit that referenced this pull request Jan 15, 2023
@fjarri fjarri mentioned this pull request Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI Changes the format of serialized objects API Related to public API Python Related to Python bindings WASM Related to WASM bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use serde exclusively for serialization Re-evaluate encoding choice for human-readable formats.
4 participants