zkcrypto / pairing

Pairing-friendly elliptic curve library.
Other
341 stars 120 forks source link

Re-export ff and group? #107

Closed afck closed 3 years ago

afck commented 5 years ago

Currently the user has to explicitly add the matching versions of the ff and group crates to their Cargo.toml, to use e.g. Field or CurveAffine. Would it make sense to re-export those from pairing instead?

pub use ff;
pub use group;

That would avoid confusing errors (something like: G1 doesn't implement CurveProjective) whenever one dependency is upgraded without the other.

burdges commented 5 years ago

In this vein, there are still path = ../ff and path = ../group lines which prevent the library from building under the normal process, or when the cloned repos go out of sync, although the build works fine for anyone who clones all the crates together.

swasilyev commented 5 years ago

Same in bellman https://github.com/zkcrypto/bellman/blob/346d540507092bed87f8528b2fc6adcd44b1cc1c/Cargo.toml#L16-L22

str4d commented 3 years ago

I think it might make sense to add this re-export to group as well, since ff and group need to be kept internally in-sync as well. So you would then have imports like:

use bls12_381::{
    pairing::{
        group::{
            ff::{Field, PrimeField},
            Group,
        },
        Engine,
    },
    Bls12,
};
str4d commented 3 years ago

Hmm, I'm not sure the above tree strategy is ideal. e.g. for bls12_381 we have feature flags that enable group and pairing logic, so we couldn't rely on bls12_381::pairing::group::ff to access ff as pairing might not be available. We'd actually need bls12_381::{ff, group, pairing} re-exports, which would mean we'd be able to access ff via bls12_381::{ff, group::ff, pairing::group::ff}.

Alternatively we could explicitly prune the branches, e.g. at the bls12_381 level we could re-export everything except group::ff from group, and so on. That seems like it could easily miss things as underlying crates get updated though (since Rust can't do negative exports).

str4d commented 3 years ago

Thinking specifically about the traits, the tree strategy is fine. pairing will always depend on both group and ff, so it makes sense to have pairing::group::ff available. bls12_381 can conditionally depend on group or pairing, so if we want to re-export there, that's where we'd need to decide how to handle the duplicate re-exports.