-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor and simplify the design of traits #189
base: main
Are you sure you want to change the base?
Refactor and simplify the design of traits #189
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Simplifies a lot the code (and the usage of the library!), and the Absorb related logic and Inputize is more coherent now! Very cool!
pub trait SonobeField: | ||
PrimeField<BasePrimeField = Self> + Absorb + AbsorbNonNative + Inputize<Self> | ||
{ | ||
type Var: FieldVar<Self, Self>; | ||
} | ||
|
||
impl<P: FpConfig<N>, const N: usize> SonobeField for Fp<P, N> { | ||
type Var = FpVar<Self>; | ||
} | ||
|
||
pub trait SonobeCurve: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you see naming them Field
and Curve
directly?
The reasoning I'm thinking is that it's shorter than SonobeField
and SonobeCurve
, which since it's a name that is used a lot across the code might be a bit tedious having always the prefix Sonobe
there. And also that the 'Sonobe' prefix can be assumed since those traits are defined inside the Sonobe crate context.
The current codebase has pretty complicated trait bounds / type parameters, and this is mostly due to the requirement of types that are more specific / powerful than the ones provided by arkworks.
For example, when using
CurveGroup
, we often need itsBaseField
to be aPrimeField
, itsScalarField
andBaseField
to beAbsorb
able, and an in-circuit typeCurveVar
to be associated with the curve.This PR aims to simplify the interfaces of complex traits, thereby improving the DX of sonobe before the incoming release. Specifically:
SonobeField
andSonobeCurve
, are introduced, in order to remove type parameters likeGC{1, 2}: CurveVar
and trait bounds likeAbsorb
andPrimeField
.AbsorbNonNative
are adjusted in order to fixto_native_sponge_field_elements
cant be called with PrimeField #170.to_constraint_field
to obtain the hash preimage that corresponds to an in-circuit variable,to_{native_}sponge_field_elements
is now preferred because it is semantically more meaningful.(Note that we cannot eliminate
to_constraint_field
calls forCurveGroup
because arkworks does not implementto_sponge_field_elements
for it, and it is also impossible for us to do the implementation as both traits are foreign.)Inputize
trait was originally added to replace arkworks'ToConstraintField::to_field_elements
, since we have too many methods similar to the latter, whose naming is also less clearer than the former.Now, this PR separates the "inputization" of non-native variables from native variables, resulting in a new trait
InputizeNonNative
. The benefit is that. we can now simultaneously handle types with two representations in-circuit (e.g.,CurveGroup
whose in-circuit version can be eitherCurveVar
orNonNativeAffineVar
).Furthermore,
Inputize
for a typeT
is now guaranteed to return underlying field elements in the same order as howT::Var
is allocated in-circuit. In comparison, arkworks' implementation ofToConstraintField::to_field_elements
for EC group elements (which essentially returnsx
,y
,infinity
of the affine form) in fact does not match the allocation of in-circuit EC variables (which is handled in the order ofx
,y
,z
of the projective form).ToEth
trait for arkworks types, so that we can avoid verbosity when implementingprepare_calldata
for on-chain deciders for HyperNova, ProtoGalaxy, etc. in the future.Thanks in advance for reviewing this (yet another) huge PR. It would be great to hear your feedback!