Skip to content

feat: serde trait for field and group elements#30

Merged
CPerezz merged 4 commits into
privacy-ethereum:mainfrom
scroll-tech:halo2-ecc-snark-verifier-0220
Mar 7, 2023
Merged

feat: serde trait for field and group elements#30
CPerezz merged 4 commits into
privacy-ethereum:mainfrom
scroll-tech:halo2-ecc-snark-verifier-0220

Conversation

@zhenfeizhang

@zhenfeizhang zhenfeizhang commented Mar 6, 2023

Copy link
Copy Markdown
Contributor

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 left a comment

Copy link
Copy Markdown
Contributor

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 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread Cargo.toml Outdated
num-bigint = "0.4.3"
num-traits = "0.2"
paste = "1.0.11"
serde = { version = "1.0", default-features = false, features = ["derive"] }

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor Author

Don't know why CI failed...

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

passed on my local machine

@CPerezz

CPerezz commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

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 left a comment

Copy link
Copy Markdown
Contributor

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-ethereum:main Mar 7, 2023
@han0110

han0110 commented Mar 10, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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?

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.

3 participants