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

Change elliptic curve new point function #970

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

Conversation

nicole-graus
Copy link
Contributor

Change elliptic curve new point function

Description

This PR changes the function new for every elliptic curve in Lambdaworks. In the previous version, this function creates a new curve point but doesn't check if it satisfies the curve equation. In this PR the function checks if the point is valid returning a Result.

@nicole-graus nicole-graus requested a review from a team as a code owner February 11, 2025 14:16
@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 90.76212% with 40 lines in your changes missing coverage. Please review.

Project coverage is 71.67%. Comparing base (d78d18a) to head (1a3e82f).

Files with missing lines Patch % Lines
math/src/elliptic_curve/short_weierstrass/point.rs 84.04% 15 Missing ⚠️
...ptic_curve/short_weierstrass/curves/stark_curve.rs 0.00% 9 Missing ⚠️
.../src/elliptic_curve/edwards/curves/tiny_jub_jub.rs 0.00% 7 Missing ⚠️
...c/elliptic_curve/montgomery/curves/tiny_jub_jub.rs 0.00% 7 Missing ⚠️
math/src/elliptic_curve/montgomery/traits.rs 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #970      +/-   ##
==========================================
+ Coverage   71.49%   71.67%   +0.17%     
==========================================
  Files         156      156              
  Lines       33969    34271     +302     
==========================================
+ Hits        24287    24564     +277     
- Misses       9682     9707      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -15,4 +17,16 @@ pub trait IsEdwards: IsEllipticCurve + Clone + Debug {
- FieldElement::<Self::BaseField>::one()
- Self::d() * x.pow(2_u16) * y.pow(2_u16)
}

// Edwards equation in projective coordinates.
// a * x^2 * z^2 + y^2 * z^2 - z^4 = d * x^2 * y^2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be more efficient just by factoring out z^2. We can update that in future PR

@jotabulacios jotabulacios requested a review from Oppen February 19, 2025 18:33
Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks correct, however:

  1. Did we check that the use of .unwrap_unchecked() actually provides any advantage over regular .unwrap()?
  2. I would suggest, regardless of which kind of unwrap we use, that we do something like the following:
let val = Curve::new(...);
debug_assert!(val.is_some());
val.unwrap()

That way we can make sure the condition holds in debug builds, without introducing any checking overhead in release builds.

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.

5 participants