Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

winderica
Copy link
Collaborator

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 its BaseField to be a PrimeField, its ScalarField and BaseField to be Absorbable, and an in-circuit type CurveVar 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:

  • Two new traits, SonobeField and SonobeCurve, are introduced, in order to remove type parameters like GC{1, 2}: CurveVar and trait bounds like Absorb and PrimeField.
  • Traits and implementations related to sponges / transcripts are redesigned.
    • Type parameters for AbsorbNonNative are adjusted in order to fix to_native_sponge_field_elements cant be called with PrimeField #170.
    • Instead of implementing and calling 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 for CurveGroup because arkworks does not implement to_sponge_field_elements for it, and it is also impossible for us to do the implementation as both traits are foreign.)
  • The 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 either CurveVar or NonNativeAffineVar).
    Furthermore, Inputize for a type T is now guaranteed to return underlying field elements in the same order as how T::Var is allocated in-circuit. In comparison, arkworks' implementation of ToConstraintField::to_field_elements for EC group elements (which essentially returns x, y, infinity of the affine form) in fact does not match the allocation of in-circuit EC variables (which is handled in the order of x, y, z of the projective form).
  • To allow easy conversion to EVM calldata, this PR implements the ToEth trait for arkworks types, so that we can avoid verbosity when implementing prepare_calldata for on-chain deciders for HyperNova, ProtoGalaxy, etc. in the future.
  • The CycleFold-related code is also refactored. I think this is more related to Improve the design of circuit, config, and instances for CycleFold #158 which I am still working on (sorry for the long delay!), but let me keep it as is in this PR.

Thanks in advance for reviewing this (yet another) huge PR. It would be great to hear your feedback!

Copy link
Collaborator

@arnaucube arnaucube left a 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!

Comment on lines +311 to +321
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:
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

to_native_sponge_field_elements cant be called with PrimeField
2 participants