zkcrypto / jubjub

Implementation of the Jubjub elliptic curve group
Other
122 stars 48 forks source link

Expose Fr::MODULUS #34

Closed znewman01 closed 4 years ago

znewman01 commented 4 years ago

As jubjub::MODULUS:

use jubjub::{Fr, MODULUS};

let modulus: Fr = MODULUS;
// do something with it

IMO the current docs ("Constant representing the modulus r = 0x...") are okay, and it doesn't need a rename or anything.

I found this useful, not sure if others will.

str4d commented 4 years ago

The comment https://github.com/zkcrypto/bls12_381/pull/39#issuecomment-617994330 applies here:

The reason this is not exposed is because it's not an element of the field, so it breaks the contract of the Scalar type. We should expose the underlying [u64; 4] representation though.

If you change your PR to expose the [u64; 4] portion of the modulus instead, and replace MODULUS with Scalar(MODULUS) elsewhere in the file, that would be fine.

znewman01 commented 4 years ago

Makes sense; just force-pushed.

Couple notes:

znewman01 commented 4 years ago

@str4d

anything else to do here?

znewman01 commented 4 years ago

@str4d ping

ebfull commented 4 years ago

Hey! This change is being done (indirectly) as part of a cross-crate refactoring effort happening in this repository. So, soon this will make its way into the API somehow. The changes are slightly different, though, and I want to avoid messy conflicts.

znewman01 commented 4 years ago

Sweet, looking forward! Feel free to close out in favor of that