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.

Sign up for GitHub

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

Jump to bottom

feat: serde trait for field and group elements #30

Merged
merged 4 commits into from
Mar 7, 2023
Merged

feat: serde trait for field and group elements #30

merged 4 commits into from
Mar 7, 2023

Conversation

zhenfeizhang
Copy link
Contributor

@zhenfeizhang zhenfeizhang commented Mar 6, 2023 •
edited
Loading

serde is quite useful for downstream libraries. would love to have this feature enabled

the user can enable it with feature = derive_serde

han0110
han0110 approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM! But I remember the reason to manually implement SerdeObject is to avoid dependency serde, so I think it'd be nice to have flag to enable/disable this.

CPerezz
CPerezz requested changes Mar 7, 2023
View reviewed changes
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Please, put this behind a serde feature. The derives and the imports as well as the serde dependency.

Users that don't need it should not pay for it in compilation time.

Cargo.toml Outdated
@@ -34,6 +35,7 @@ lazy_static = "1.4.0"
num-bigint = "0.4.3"
num-traits = "0.2"
paste = "1.0.11"
serde = { version = "1.0", default-features = false, features = ["derive"] }
Copy link
Member

Choose a reason for hiding this comment

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

I'd create a serde feature and import this under it.

@zhenfeizhang
Copy link
Contributor Author

cool. added in 5b97d11

@zhenfeizhang zhenfeizhang force-pushed the halo2-ecc-snark-verifier-0220 branch from 94dcabd to c897666 Compare March 7, 2023 15:59
@zhenfeizhang
Copy link
Contributor Author

Don't know why CI failed...

 cargo test --verbose --release --all --features asm

passed on my local machine

@CPerezz
Copy link
Member

CPerezz commented Mar 7, 2023

Don't know why CI failed...

 cargo test --verbose --release --all --features asm

passed on my local machine

No worries. It happens sometimes. Not sure what's the exact issue with the gh-runner TBH.
I triggered it again. Let's hope it works now.

CPerezz
CPerezz approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding the feature!!

@CPerezz CPerezz merged commit c4dce16 into privacy-scaling-explorations:main Mar 7, 2023
@han0110
Copy link
Contributor

han0110 commented Mar 10, 2023

Hi @zhenfeizhang is there any specific reason not re-export ff and group by default? I think it'd a little awkward for downstream libraries since all of those relying on halo2curves now need to specify the reexport feature (I think most of them need to import the traits in ff and group).

If no specific concern I'd suggest to make re-export it by default, what do you think?

@zhenfeizhang
Copy link
Contributor Author

Hi @zhenfeizhang is there any specific reason not re-export ff and group by default? I think it'd a little awkward for downstream libraries since all of those relying on halo2curves now need to specify the reexport feature (I think most of them need to import the traits in ff and group).

If no specific concern I'd suggest to make re-export it by default, what do you think?

Happy to do so. Want to double check @CPerezz before preceeding

The derives and the imports as well as the serde dependency.

Users that don't need it should not pay for it in compilation time.

Is re-export enabled by default okay with you?

@zhenfeizhang zhenfeizhang mentioned this pull request Mar 13, 2023
Merged
@huitseeker huitseeker mentioned this pull request Sep 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@han0110 han0110 han0110 approved these changes

@CPerezz CPerezz CPerezz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhenfeizhang @CPerezz @han0110