You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
from_coor(x, y) method should return Result<Self, ErrorKey>.
Currently it returns Self and panics if point is not on the curve.
Add is_on_curve(x, y) -> bool method.
It might be helpful for early point validation.
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.
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?).
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:
Also we should probably restrict ECScalar not to ever be zero. In such a way, the following changes are required:
from(bytes) -> Self method should return Result<Self>
set_element(&self, ...) method should probably also return Result<()> (as we cannot generally guarantee that every PK is proven to be non-zero)
zero() -> Self method should be removed
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.
Should ECScalar trait leak more info about a curve? Currently, it exposes only the order of G (ECScalar::q()). I would add such enum:
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 ofcurv
crate.from_coor(x, y)
method should returnResult<Self, ErrorKey>
.Currently it returns
Self
and panics if point is not on the curve.is_on_curve(x, y) -> bool
method.It might be helpful for early point validation.
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 areNone
, then it's a point at infinity. It works, but it looks unclear in the code.add_point_checked
,sub_point_checked
andscalar_mul_checked
which should returnOption<Self>
(as any of them may produce a point at infinity). Alsoadd_point
,sub_point
andscalar_mul
should be explicitly marked that they will panic if point at infinity occurs.x_coor
andy_coor
methods will returnBigInt
instead ofOption<BigInt>
as they are always present (am I right?).pk_to_key_slice
andbytes_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 returnsBigInt
. I suggest making both methods to returnVec<u8>
. The caller will have to convert to BigInt if it's necessary by callingBigInt::from(...)
.So I suggest to replace both methods with a single one:
from(bytes) -> Self
method should returnResult<Self>
set_element(&self, ...)
method should probably also returnResult<()>
(as we cannot generally guarantee that every PK is proven to be non-zero)zero() -> Self
method should be removedadd_checked
,mul_checked
,sub_checked
methods which returnOption<Self>
.add
,mul
,sub
methods should be explicitly marked that they will panic if zero scalar occurs.ECScalar::q()
). I would add such enum:It's a big list of suggestions, but every one may be considered independently.
The text was updated successfully, but these errors were encountered: