-
Notifications
You must be signed in to change notification settings - Fork 7
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
Generalize field in Circlepoint #99
base: main
Are you sure you want to change the base?
Generalize field in Circlepoint #99
Conversation
00f9516
to
843fb38
Compare
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.
Reviewed all commit messages.
Reviewable status: 0 of 9 files reviewed, 14 unresolved discussions (waiting on @atgrosso)
stwo_cairo_verifier/src/circle.cairo
line 8 at r1 (raw file):
pub const M31_CIRCLE_GEN: CirclePoint<M31> = CirclePoint::<M31> { x: M31 { inner: 2 }, y: M31 { inner: 1268011823 }, };
Nit: can this be implicit
stwo_cairo_verifier/src/circle.cairo
line 31 at r1 (raw file):
// Returns the neutral element of the circle. fn zero() -> CirclePoint<F> { CirclePoint::<F> { x: One::<F>::one(), y: Zero::<F>::zero() }
Same here?
Suggestion:
CirclePoint { x: One::one(), y: Zero::zero() }
stwo_cairo_verifier/src/circle.cairo
line 50 at r1 (raw file):
}; result }
Why does this have to change so much? Can't it be the same as before just change scalar to u128?
Code quote:
fn mul(
self: @CirclePoint<F>, initial_scalar: u128
) -> CirclePoint<
F
> {
let mut scalar = initial_scalar;
let mut result: CirclePoint<F> = Self::zero();
let mut cur: CirclePoint<F> = *self;
while scalar > 0 {
if scalar & 1 == 1 {
result = result + cur;
}
cur = cur + cur;
scalar = scalar / 2;
};
result
}
stwo_cairo_verifier/src/circle.cairo
line 54 at r1 (raw file):
impl CirclePointAdd<F, +Add<F>, +Sub<F>, +Mul<F>, +Drop<F>, +Copy<F>> of Add<CirclePoint<F>> { // The operation of the circle as a group with additive notation.
I know unrelated to your changes but mind chageing to docstring here please?
Suggestion:
/// Performs the operation of the circle as a group with additive notation.
stwo_cairo_verifier/src/circle.cairo
line 56 at r1 (raw file):
// The operation of the circle as a group with additive notation. fn add(lhs: CirclePoint<F>, rhs: CirclePoint<F>) -> CirclePoint<F> { CirclePoint::<F> { x: lhs.x * rhs.x - lhs.y * rhs.y, y: lhs.x * rhs.y + lhs.y * rhs.x }
Same here?
stwo_cairo_verifier/src/circle.cairo
line 72 at r1 (raw file):
CirclePoint { x: self.x.complex_conjugate(), y: self.y.complex_conjugate() } } }
Suggestion:
#[generate_trait]
pub impl ComplexConjugateImpl of ComplexConjugateTrait {
fn complex_conjugate(self: CirclePoint<QM31>) -> CirclePoint<QM31> {
CirclePoint { x: self.x.complex_conjugate(), y: self.y.complex_conjugate() }
}
}
stwo_cairo_verifier/src/circle.cairo
line 74 at r1 (raw file):
} /// Represents the coset initial + <step>.
Suggestion:
/// Represents the coset `initial + <step>`.
stwo_cairo_verifier/src/circle.cairo
line 112 at r1 (raw file):
} fn at(self: @Coset, index: usize) -> CirclePoint::<M31> {
Suggestion:
CirclePoint<M31>
stwo_cairo_verifier/src/circle.cairo
line 122 at r1 (raw file):
/// Creates a coset of the form G_2n + \<G_n\>. /// For example, for n=8, we get the point indices \[1,3,5,7,9,11,13,15\].
Suggestion:
/// Creates a coset of the form `G_2n + <G_n>`.
///
/// For example, for `n=8`, we get the point indices `[1,3,5,7,9,11,13,15]`.
stwo_cairo_verifier/src/circle.cairo
line 124 at r1 (raw file):
/// For example, for n=8, we get the point indices \[1,3,5,7,9,11,13,15\]. fn odds(log_size: u32) -> Coset { //CIRCLE_LOG_ORDER
Remove
stwo_cairo_verifier/src/circle.cairo
line 131 at r1 (raw file):
/// Creates a coset of the form G_4n + <G_n>. /// For example, for n=8, we get the point indices \[1,5,9,13,17,21,25,29\]. /// Its conjugate will be \[3,7,11,15,19,23,27,31\].
Suggestion:
/// Creates a coset of the form `G_4n + <G_n>`.
///
/// For example, for `n=8`, we get the point indices `[1,5,9,13,17,21,25,29]`.
/// Its conjugate will be `[3,7,11,15,19,23,27,31]`.
stwo_cairo_verifier/src/circle.cairo
line 156 at r1 (raw file):
#[test] fn test_add_1() { let i = CirclePoint::<M31> { x: m31(0), y: m31(1) };
Same here? and below?
Suggestion:
let i = CirclePoint { x: m31(0), y: m31(1) };
stwo_cairo_verifier/src/circle.cairo
line 160 at r1 (raw file):
let expected_result = CirclePoint::<M31> { x: -m31(1), y: m31(0) }; assert_eq!(result, expected_result);
I'd prefer no variable for expected_result
.
Same below
Suggestion:
let result = i + i;
assert_eq!(result, CirclePoint { x: -m31(1), y: m31(0) });
stwo_cairo_verifier/src/circle.cairo
line 285 at r1 (raw file):
x: qm31(1, 0, 478637715, 513582971), y: qm31(992285211, 649143431, 740191619, 1186584352), };
This will be needed outside of tests.
Can define below M31_CIRCLE_GEN
Suggestion:
/// A generator for the circle group over [`SecureField`].
pub const QM31_CIRCLE_GEN: CirclePointQM31 =
CirclePointQM31 {
x: QM31 {
a: CM31 { a: M31 { inner: 1 }, b: M31 { inner: 0 }, },
b: CM31 { a: M31 { inner: 478637715 }, b: M31 { inner: 513582971 } }
},
y: QM31 {
a: CM31 { a: M31 { inner: 992285211 }, b: M31 { inner: 649143431 } },
b: CM31 { a: M31 { inner: 740191619 }, b: M31 { inner: 1186584352 } }
},
};
e07dfc2
to
ca1ea82
Compare
ca1ea82
to
f71a174
Compare
Depends on #98
This change is