ZenGo-X / curv

Rust language general purpose elliptic curve cryptography.
MIT License
264 stars 111 forks source link

ECPoint improvement #97

Open survived opened 3 years ago

survived commented 3 years ago

I propose the following changes in ECPoint & ECScalar traits aimed at better user experience, reducing unsafe unwrap()s in curve implementations, based on feedback from people who built their solution on top of curv crate.

  1. from_coor(x, y) method should return Result<Self, ErrorKey>. Currently it returns Self and panics if point is not on the curve.
  2. Add is_on_curve(x, y) -> bool method. It might be helpful for early point validation.
    1. Add is_at_infinity(&self) -> bool method. Currently, the only way to check if the point is at infinity (as I understood) - call x_coor()/y_coor(). If any of them are None, then it's a point at infinity. It works, but it looks unclear in the code.
    2. Another way is to restrict ECPoint to never be at infinity. Then we should add methods add_point_checked, sub_point_checked and scalar_mul_checked which should return Option<Self> (as any of them may produce a point at infinity). Also add_point, sub_point and scalar_mulshould be explicitly marked that they will panic if point at infinity occurs. x_coor and y_coor methods will return BigInt instead of Option<BigInt> as they are always present (am I right?).
  3. pk_to_key_slice and bytes_compressed_to_big_int methods are a bit confusing. They both do the same thing - marshal point to bytes. But the first marshals in uncompressed form (which is unclear until you look at implementation). Also confusing thing is that one method returns Vec<u8>, another returns BigInt. I suggest making both methods to return Vec<u8>. The caller will have to convert to BigInt if it's necessary by calling BigInt::from(...). So I suggest to replace both methods with a single one:
     fn to_encoded_point(&self, compressed: bool) -> Vec<u8>
  4. Also we should probably restrict ECScalar not to ever be zero. In such a way, the following changes are required:
    1. from(bytes) -> Self method should return Result<Self>
    2. set_element(&self, ...) method should probably also return Result<()> (as we cannot generally guarantee that every PK is proven to be non-zero)
    3. zero() -> Self method should be removed
    4. Add add_checked, mul_checked, sub_checked methods which return Option<Self>. add, mul, sub methods should be explicitly marked that they will panic if zero scalar occurs.
  5. Should ECScalar trait leak more info about a curve? Currently, it exposes only the order of G (ECScalar::q()). I would add such enum:
    #[non_exhaustive]
    enum CurveParams {
     Weierstrass {
       a: BigInt, b: BigInt, q: BigInt, p: BigInt, ...
     },
     Montgomery {
       a: BigInt, ...
     }
    }

    And then add method to ECScalar:

    fn params() -> &'static CurveParams

It's a big list of suggestions, but every one may be considered independently.