feat: twisted edwards curves #633

qalisander · 2025-04-22T17:30:35Z

Follows #589

PR Checklist

Tests
Documentation
Changelog

netlify · 2025-04-22T17:30:56Z

✅ Deploy Preview for contracts-stylus canceled.

Name	Link
🔨 Latest commit	`fcd9506`
🔍 Latest deploy log	https://app.netlify.com/projects/contracts-stylus/deploys/685e7ed86c11880008df7dbf

codecov · 2025-04-22T17:32:34Z

Codecov Report

Attention: Patch coverage is 54.13333% with 172 lines in your changes missing coverage. Please review.

Project coverage is 84.9%. Comparing base (e15e579) to head (bbcb612).

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
lib/crypto/src/curve/te/projective.rs	53.2%	78 Missing ⚠️
lib/crypto/src/curve/te/affine.rs	31.6%	69 Missing ⚠️
lib/crypto/src/curve/te/mod.rs	81.6%	18 Missing ⚠️
lib/crypto/src/field/fp.rs	22.2%	7 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/crypto/src/curve/mod.rs	`16.3% <ø> (ø)`
lib/crypto/src/field/fp.rs	`73.2% <22.2%> (-1.1%)`	⬇️
lib/crypto/src/curve/te/mod.rs	`81.6% <81.6%> (ø)`
lib/crypto/src/curve/te/affine.rs	`31.6% <31.6%> (ø)`
lib/crypto/src/curve/te/projective.rs	`53.2% <53.2%> (ø)`

... and 1 file with indirect coverage changes

…wisted-edwards-curves

lib/crypto/src/curve/te/affine.rs

0xNeshi · 2025-06-19T06:47:26Z

lib/crypto/src/curve/te/mod.rs

+    /// Checks that the current point is in the prime order subgroup given
+    /// the point on the curve.
+    fn is_in_correct_subgroup_assuming_on_curve(item: &Affine<Self>) -> bool {
+        Self::mul_affine(item, Self::ScalarField::characteristic()).is_zero()
+    }


Suggested change

/// Checks that the current point is in the prime order subgroup given

/// the point on the curve.

fn is_in_correct_subgroup_assuming_on_curve(item: &Affine<Self>) -> bool {

Self::mul_affine(item, Self::ScalarField::characteristic()).is_zero()

}

/// Checks that the current point is in the prime order subgroup given

/// the point on the curve.

fn is_in_prime_order_subgroup(item: &Affine<Self>) -> bool {

Self::mul_affine(item, Self::ScalarField::characteristic()).is_zero()

}

Judging by docs, it seems that this is what it's checking

lib/crypto/src/curve/te/mod.rs

lib/crypto/src/curve/te/affine.rs

0xNeshi · 2025-06-19T06:53:49Z

lib/crypto/src/curve/te/affine.rs

+    fn xy(&self) -> Option<(Self::BaseField, Self::BaseField)> {
+        (!self.is_zero()).then_some((self.x, self.y))
+    }


Why do you think it's better to return None for zero than to return actual zero tuple ((P::BaseField::ZERO, P::BaseField::ONE))?

0xNeshi · 2025-06-19T07:53:02Z

lib/crypto/src/curve/te/projective.rs

+        // See "Twisted Edwards Curves Revisited"
+        // Huseyin Hisil, Kenneth Koon-Ho Wong, Gary Carter, and Ed Dawson
+        // 3.3 Doubling in E^e
+        // Source: https://www.hyperelliptic.org/EFD/g1p/data/twisted/extended/doubling/dbl-2008-hwcd


Better to put this comment above as fn docs

0xNeshi · 2025-06-19T07:54:25Z

lib/crypto/src/curve/te/projective.rs

+    fn add_assign(&mut self, other: T) {
+        let other = other.borrow();
+        // See "Twisted Edwards Curves Revisited"
+        // Huseyin Hisil, Kenneth Koon-Ho Wong, Gary Carter, and Ed Dawson
+        // 3.1 Unified Addition in E^e
+        // Source: https://www.hyperelliptic.org/EFD/g1p/data/twisted/extended/addition/madd-2008-hwcd
+
+        // A = X1*X2
+        let a = self.x * other.x;
+        // B = Y1*Y2
+        let b = self.y * other.y;
+        // C = T1*d*T2
+        let c = P::COEFF_D * self.t * other.x * other.y;
+
+        // D = Z1
+        let d = self.z;
+        // E = (X1+Y1)*(X2+Y2)-A-B
+        let e = (self.x + self.y) * (other.x + other.y) - a - b;
+        // F = D-C
+        let f = d - c;
+        // G = D+C
+        let g = d + c;
+        // H = B-a*A
+        let h = b - P::mul_by_a(a);
+        // X3 = E*F
+        self.x = e * f;
+        // Y3 = G*H
+        self.y = g * h;
+        // T3 = E*H
+        self.t = e * h;
+        // Z3 = F*G
+        self.z = f * g;


Same question as before for comments

0xNeshi · 2025-06-19T07:55:25Z

lib/crypto/src/curve/te/projective.rs

+        // See "Twisted Edwards Curves Revisited" (https://eprint.iacr.org/2008/522.pdf)
+        // by Huseyin Hisil, Kenneth Koon-Ho Wong, Gary Carter, and Ed Dawson
+        // 3.1 Unified Addition in E^e
+
+        // A = x1 * x2
+        let a = self.x * other.x;
+
+        // B = y1 * y2
+        let b = self.y * other.y;
+


Same question as before for comments

0xNeshi · 2025-06-19T07:55:46Z

lib/crypto/src/curve/te/projective.rs

+
+impl_additive_ops_from_ref!(Projective, TECurveConfig);
+
+impl<'a, P: TECurveConfig> Add<&'a Self> for Projective<P> {


Would it make sense to implement ref + ref?

0xNeshi · 2025-06-19T07:59:31Z

lib/crypto/src/curve/te/affine.rs

+    ///
+    /// * If point is not on curve.
+    /// * If point is not in the prime-order subgroup.
+    pub fn new(x: P::BaseField, y: P::BaseField) -> Self {


Some general comments:

could any of the new functions be marked with inline(always) or must_use?

some flows are not covered with unit tests

could we implement any proptests for this?

missing CHANGELOG

Agree with @0xNeshi 💯

bidzyyys

Almost looks good, take care of @0xNeshi comments please.

bidzyyys · 2025-06-24T11:09:30Z

lib/crypto/src/curve/te/affine.rs

+    ///
+    /// * If point is not on curve.
+    /// * If point is not in the prime-order subgroup.
+    pub fn new(x: P::BaseField, y: P::BaseField) -> Self {


Agree with @0xNeshi 💯

lib/crypto/src/curve/te/affine.rs

bidzyyys · 2025-06-24T11:16:46Z

lib/crypto/src/curve/te/mod.rs

+    }
+
+    /// Default implementation of group multiplication for projective
+    /// coordinates


Suggested change

/// coordinates

/// coordinates.

bidzyyys · 2025-06-24T11:16:53Z

lib/crypto/src/curve/te/mod.rs

+    }
+
+    /// Default implementation of group multiplication for affine
+    /// coordinates


Suggested change

/// coordinates

/// coordinates.

bidzyyys · 2025-06-24T11:18:32Z

lib/crypto/src/curve/te/projective.rs

+
+        batch_inversion(&mut z_s);
+
+        // Perform affine transformations


Suggested change

// Perform affine transformations

// Perform affine transformations.

bidzyyys · 2025-06-24T11:18:41Z

lib/crypto/src/curve/te/projective.rs

+    fn normalize_batch(v: &[Self]) -> Vec<Self::Affine> {
+        // A projective curve element (x, y, t, z) is normalized
+        // to its affine representation, by the conversion
+        // (x, y, t, z) -> (x/z, y/z, t/z, 1)


Suggested change

// (x, y, t, z) -> (x/z, y/z, t/z, 1)

// (x, y, t, z) -> (x/z, y/z, t/z, 1).

bidzyyys · 2025-06-24T11:18:57Z

lib/crypto/src/curve/te/projective.rs

+        // (x, y, t, z) -> (x/z, y/z, t/z, 1)
+        // Batch normalizing N twisted edwards curve elements costs:
+        //     1 inversion + 6N field multiplications
+        // (batch inversion requires 3N multiplications + 1 inversion)


Suggested change

// (batch inversion requires 3N multiplications + 1 inversion)

// (batch inversion requires 3N multiplications + 1 inversion).

add twisted edwards curves

d5eb5c3

Merge branch 'main' into openzeppelin-crypto/twisted-edwards-curves

fe5f9e8

bidzyyys linked an issue May 27, 2025 that may be closed by this pull request

[Feature]: Twisted Edwards Curves #680

Open

1 task

qalisander added 3 commits May 29, 2025 15:13

add ed25519 test cases scratch

3a45cf1

add scalar_mul test case

dd870a2

add point_add and point_sub test cases

277f8ba

qalisander marked this pull request as ready for review June 11, 2025 12:23

qalisander requested review from bidzyyys and 0xNeshi as code owners June 11, 2025 12:23

qalisander added 3 commits June 11, 2025 15:23

Merge remote-tracking branch 'origin/main' into openzeppelin-crypto/t…

57c294e

…wisted-edwards-curves

clippy fixes

fabe9c4

++

511f3b3

0xNeshi reviewed Jun 19, 2025

View reviewed changes

bidzyyys and others added 2 commits June 23, 2025 11:12

Merge branch 'main' into openzeppelin-crypto/twisted-edwards-curves

bbcb612

++

fcd9506

bidzyyys reviewed Jun 24, 2025

View reviewed changes


		impl_additive_ops_from_ref!(Projective, TECurveConfig);

		impl<'a, P: TECurveConfig> Add<&'a Self> for Projective<P> {

	// Perform affine transformations
	// Perform affine transformations.

	// (x, y, t, z) -> (x/z, y/z, t/z, 1)
	// (x, y, t, z) -> (x/z, y/z, t/z, 1).

	// (batch inversion requires 3N multiplications + 1 inversion)
	// (batch inversion requires 3N multiplications + 1 inversion).

feat: twisted edwards curves #633

Are you sure you want to change the base?

feat: twisted edwards curves #633

Uh oh!

Conversation

qalisander commented Apr 22, 2025

PR Checklist

Uh oh!

netlify bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for contracts-stylus canceled.

Uh oh!

codecov bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xNeshi Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bidzyyys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

netlify bot commented Apr 22, 2025 •

edited

Loading

codecov bot commented Apr 22, 2025 •

edited

Loading

0xNeshi Jun 19, 2025 •

edited

Loading