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.

Sign up for GitHub

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

Affine coordinate are not correctly exposed from the underlying type. #116

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

Affine coordinate are not correctly exposed from the underlying type. #116

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f3a2b3
Affine coordinate are not correctly exposed from the underlying type.
FilipLaurentiu Jan 29, 2025
b53ca90
Merge branch 'main' into main
FilipLaurentiu Jan 29, 2025
9c0676e
Fix the `Add` implementation
FilipLaurentiu Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions crates/starknet-types-core/src/curve/affine_point.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,12 @@ impl AffinePoint {

/// Returns the `x` coordinate of the point.
pub fn x(&self) -> Felt {
Felt(*self.0.x())
Felt(*self.0.to_affine().x())
Copy link
Collaborator

Choose a reason for hiding this comment

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

here is the to_affine method you are calling

    /// Creates the same point in affine coordinates. That is,
    /// returns [x / z: y / z: 1] where `self` is [x: y: z].
    /// Panics if `self` is the point at infinity.
    pub fn to_affine(&self) -> Self {
        let [x, y, z] = self.coordinates();
        // If it's the point at infinite
        if z == &FieldElement::zero() {
            // We make sure all the points in the infinite have the same values
            return Self::new([
                FieldElement::zero(),
                FieldElement::one(),
                FieldElement::zero(),
            ]);
        };
        let inv_z = z.inv().unwrap();
        ProjectivePoint::new([x * &inv_z, y * inv_z, FieldElement::one()])
    }

From my understanding it can return values that are different form the one x() and y() are currently returning.

So this is a breaking change.
But I'm not sure it is a bug fix.

The methods documentation says it returens the coordinate, but it's not specified if it's in projective or affine coordinate.

You seems to think it should be affine, but currently it's projective

Copy link
Contributor Author

@FilipLaurentiu FilipLaurentiu Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify. The current behavior is to return the value of x from projective representation of a point, unchanged. The equivalent value in affine representation for x is x/z .

The current version works because if you create a point from (x,y) coordinate, the z value will be 1 and converting back to affine you need to do x/1 which is the same x, but if you perform some operations with the point and want to get back the affine coordinate, the z value will not be 1 anymore.

ProjectivePoint::new([x * &inv_z, y * inv_z, FieldElement::one()])

This changes should not break anything, the mul/add operations were added by me recently in e0eff28

}

/// Returns the `y` coordinate of the point.
pub fn y(&self) -> Felt {
Felt(*self.0.y())
Felt(*self.0.to_affine().y())
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't you want to add a z() method too?

Copy link
Contributor Author

@FilipLaurentiu FilipLaurentiu Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it's necessary. The type is AffinePoint, I don't see why it should have a z.
If you think it make sense to also add z, I could add it. Or maybe a more general method to_projective that returns (x,y,z) projective coordinates

// Returns the generator point of the StarkCurve
Expand All @@ -70,7 +70,7 @@ impl core::ops::Add<AffinePoint> for AffinePoint {
type Output = AffinePoint;

fn add(self, rhs: Self) -> Self::Output {
AffinePoint(self.0.operate_with_affine(&rhs.0))
AffinePoint(self.0.operate_with(&rhs.0))
}
}

Expand Down