arkworks-rs / poly-commit

A Rust library for polynomial commitments
Apache License 2.0
326 stars 128 forks source link

Simplify `PolynomialCommitment` trait: remove the generic on `S: CryptographicSponge` #145

Open mmagician opened 6 months ago

mmagician commented 6 months ago

Description

Quality of life improvement for downstream users of the library.


Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why.

autquis commented 6 months ago

I pushed this to make the tests pass. I can also open another PR to the master for this and then you can rebase this one on that; let me know.

mmagician commented 6 months ago

@autquis Thank you!

mmagician commented 6 months ago

It's a bit annoying with that stable/nightly discrepancy on imports

mmagician commented 6 months ago

@autquis I had to undo your changes, as no-std was failing. In the end I ended up following the same strategy as https://github.com/arkworks-rs/algebra/pull/790/files#diff-6035356f5adc30af6393d5492dbc1d9d8ec187272030fdd95709b1b30f1275fc

autquis commented 6 months ago

I wonder, isn't it clenaer if we use #[cfg(not(feature = "std"))] for these imports?

mmagician commented 6 months ago

That's worth exploring yes

mmagician commented 6 months ago

The checks in #130 are failing. They've already been addressed here, we can either try to cherry pick the relevant commit to Hyrax, or merge this PR first and adapt Hyrax accordingly to remove the generic S. I'm in favor of the latter @Pratyush

autquis commented 6 months ago

I added ark-std to the patch due to arkworks-rs/std#48 CI fails because of the same issue of imports in crypto-primitive. But poly-commit is fine now.

Also, I am doing a bit of cleaning up in the order of imports, as we are touching imports anyway.

autquis commented 6 months ago

This PR depends now on arkworks-rs/crypto-primitives#142 Once that is merged, we can update Cargo.toml.

autquis commented 5 months ago

The PR is ready to be merged. @z-tech @Pratyush @weikengchen

autquis commented 5 months ago

Ping! Once this gets merged, I can move to the other PRs (Hyrax, Brakedown) and prepare them for being merged.

autquis commented 4 months ago

Another ping. @weikengchen @Pratyush @z-tech

Also, it seems the cfg(not(feature("std"))) is no longer needed (rustc stable got upgraded). Once you review this, I'll remove them, and then, merge the PR, please. I'm not changing it now.

autquis commented 3 months ago

Gentle reminder @Pratyush