zcash / pasta_curves

Rust implementation for zcash/pasta
Other
80 stars 49 forks source link

Implement Serde for fields and curves #48

Closed vmx closed 1 year ago

vmx commented 2 years ago

Currently Serde is only implemented for Fp, Fq, EpAffine and EqAffine. Support can be enabled with the serde feature.

It's based on the implementation in blstrs: https://github.com/filecoin-project/blstrs/blob/master/src/serde_impl.rs

Please let me know if that's the correct approach, or e.g. you'd like to serialize them to four u64s.

I've only implemented it for the things I currently need, but I'm happy to provide an implementation for all curves.

codecov-commenter commented 2 years ago

Codecov Report

Base: 66.15% // Head: 72.35% // Increases project coverage by +6.20% :tada:

Coverage data is based on head (d9b9fe1) compared to base (682a0e6). Patch coverage: 95.18% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #48 +/- ## ========================================== + Coverage 66.15% 72.35% +6.20% ========================================== Files 12 13 +1 Lines 1427 1530 +103 ========================================== + Hits 944 1107 +163 + Misses 483 423 -60 ``` | [Impacted Files](https://codecov.io/gh/zcash/pasta_curves/pull/48?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash) | Coverage Δ | | |---|---|---| | [src/lib.rs](https://codecov.io/gh/zcash/pasta_curves/pull/48/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2xpYi5ycw==) | `62.50% <ø> (-37.50%)` | :arrow_down: | | [src/serde\_impl.rs](https://codecov.io/gh/zcash/pasta_curves/pull/48/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL3NlcmRlX2ltcGwucnM=) | `95.18% <95.18%> (ø)` | | | [src/vesta.rs](https://codecov.io/gh/zcash/pasta_curves/pull/48/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL3Zlc3RhLnJz) | `74.07% <0.00%> (-25.93%)` | :arrow_down: | | [src/pallas.rs](https://codecov.io/gh/zcash/pasta_curves/pull/48/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL3BhbGxhcy5ycw==) | `84.48% <0.00%> (-15.52%)` | :arrow_down: | | [src/macros.rs](https://codecov.io/gh/zcash/pasta_curves/pull/48/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL21hY3Jvcy5ycw==) | `51.61% <0.00%> (-1.73%)` | :arrow_down: | | [src/fields/fp.rs](https://codecov.io/gh/zcash/pasta_curves/pull/48/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2ZpZWxkcy9mcC5ycw==) | `84.65% <0.00%> (+4.37%)` | :arrow_up: | | [src/fields/fq.rs](https://codecov.io/gh/zcash/pasta_curves/pull/48/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2ZpZWxkcy9mcS5ycw==) | `84.09% <0.00%> (+4.37%)` | :arrow_up: | | [src/curves.rs](https://codecov.io/gh/zcash/pasta_curves/pull/48/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash#diff-c3JjL2N1cnZlcy5ycw==) | `54.66% <0.00%> (+15.89%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zcash)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

parazyd commented 2 years ago

@vmx Would you also consider adding borsh support or is that out of scope?

vmx commented 2 years ago

@parazyd I won't have time working on borsh support.

parazyd commented 2 years ago

@vmx Alright, I will then add it after this PR is merged. Thank you.

FrankC01 commented 1 year ago

Is this going to be merged in?

str4d commented 1 year ago

This PR needs to be rebased (GitHub's UI indicates there is a merge conflict).

vmx commented 1 year ago

As I needed to rebase, I did merged all commits into a single one. It should now include all the things mentioned in the review.

Notable things are:

samuelburnham commented 1 year ago

Following this model, I have also implemented Serde for the Ep and Eq types. Should I open a separate PR or wait till this one is merged?