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

Add serde feature. #87

Closed
wants to merge 1 commit into from
Closed

Add serde feature. #87

wants to merge 1 commit into from

Conversation

afck
Copy link

@afck afck commented Jun 11, 2018

Closes #86

Copy link
Collaborator

@ebfull ebfull left a comment

Choose a reason for hiding this comment

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

This is a great PR!

I'm a little bit worried when I see Vec<_> anywhere in this serialization code and I don't have time to review thoroughly right now. All of our buffers should be fixed length and should fit nicely on the stack. Will revisit this later.

Cargo.toml Outdated
@@ -15,9 +15,14 @@ repository = "https://github.com/ebfull/pairing"
rand = "0.4"
byteorder = "1"
clippy = { version = "0.0.200", optional = true }
serde = { version = "1.0.66", optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not 1.0?

Copy link
Author

Choose a reason for hiding this comment

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

I'll change it.

("1.0.66" has the same meaning as "1.0", except that it excludes versions earlier than 1.0.66. I usually write it that way because I haven't tried it with versions earlier than when I introduced the dependency, and I don't know how any of the bugs fixed since then would affect the crate.)

Cargo.toml Outdated

[features]
unstable-features = ["expose-arith"]
expose-arith = []
serialization-serde = ["serde"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the feature be called serde without violating recommended practices?

Copy link
Author

Choose a reason for hiding this comment

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

Then Cargo complains:

Features and dependencies cannot have the same name: `serde`

Maybe use-serde? serde-support? Not sure whether those are much better.

Choose a reason for hiding this comment

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

You don't need to have an explicit feature; when a crate is declared as an optional dependency it becomes an implicit feature: https://doc.rust-lang.org/cargo/reference/manifest.html#the-features-section

Copy link
Author

Choose a reason for hiding this comment

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

Thank you! I changed it to "serde".

@afck
Copy link
Author

afck commented Jun 12, 2018

I'm a little bit worried when I see Vec<_> anywhere

I tried to write generic group and field [de]serialization code, and then reuse it for the specific G1, G2, Fr and Fq. The generic code can't make assumptions about the length.

But I found a better solution for the fields, at least, since the bytes in FrRepr and FqRepr are public anyway.

For the groups it's not that easy, though, since [u8; 48] doesn't implement Serialize and Deserialize (the standard impls go only up to 32 and there's still no nice solution). I agree that it's annoying to waste eight (four?) bytes on the length of a constant-length array. But the only solution I can think of is to basically duplicate serde's own implementation for arrays. I'm happy to do that; what do you think?

@afck
Copy link
Author

afck commented Jun 20, 2018

I'd also like to require e.g. PrimeField: Serialize + for<'r> Deserialize<'r>, so that users can use serialization even in a generic E: Engine context. But since it's feature-gated I don't know if that's possible without duplicating the whole trait definition.

@ebfull
Copy link
Collaborator

ebfull commented Jul 3, 2018

I've been busy travelling but this PR is on my TODO list.

@ebfull
Copy link
Collaborator

ebfull commented Jul 3, 2018

Two things:

  1. We're splitting the field definitions and stuff away from this crate and into a separate crate called ff. See Use ff crate for traits and impls #90 for more information.
  2. One thing that's weird about adding supertraits to PrimeField or PointRepresentation based on a crate feature is that Rust features are supposed to only augment the API, whereas adding bounds to a trait is a reverse-incompatible change. I don't think we can use a Sealed trick to avoid this problem.

@ebfull
Copy link
Collaborator

ebfull commented Jul 3, 2018

It seems like it would be better to implement a wrapper around the types pairing provides and implement serialization for that stuff, rather than trying to build a generic interaction between these traits and serialization capabilities. The wrapper might even be more ergonomic to use downstream.

@afck
Copy link
Author

afck commented Jul 4, 2018

I agree, the conditionally compiled trait bounds were a bad idea.

Yes, we're currently using a wrapper. Another option is providing a serialization module for #[serde(with = "module")].

I still think the most convenient and straightforward way to provide serialization is to implement the serde traits directly for the types themselves. But feel free to close this PR if you'd prefer one of the other approaches. I'm happy to try and implement them.

@afck
Copy link
Author

afck commented Jul 4, 2018

The Rust API guidelines also recommend to just implement Serialize and Deserialize for the types directly, possibly behind a feature gate.

@bmerge
Copy link
Collaborator

bmerge commented Jul 5, 2018

☔ The latest upstream changes (presumably #90) made this pull request unmergeable. Please resolve the merge conflicts.

@afck
Copy link
Author

afck commented Aug 9, 2018

I resolved the merge conflict. Please let me know if you want me to make any other changes.

@quininer
Copy link

Can you add Serialize and Deserialize to Fq12? This is very useful in IBE.

@afck
Copy link
Author

afck commented Aug 24, 2018

For that it would be really convenient to use serde_derive. But then the feature can't be called serde anymore, I think. Which do you prefer:

  • You need to enable both features serde and serde_derive to get a serializable Fq12 (and Fq6 and Fq2).
  • Keep only serde and manually implement serialization for Fq12, too, which is a bit more complex than deriving it. (Slightly more code, but maybe not too bad.)
  • Rename the feature, e.g. use-serde = ["serde", "serde_derive"].
  • No feature: always support (and depend on) serde.

@Pratyush
Copy link

Serde has a feature "derive" which exposes the serde_derive macro from within serde itself; maybe that can help?

@afck
Copy link
Author

afck commented Aug 24, 2018

You're right, thanks! I added it.

@afck afck changed the title Add serialization-serde feature. Add serde feature. Apr 28, 2019
@ebfull ebfull closed this Sep 8, 2020
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.

Serde support
5 participants