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

Added support for secp256r1 in Cairo #3269

Merged
merged 1 commit into from
Jun 5, 2023
Merged

Added support for secp256r1 in Cairo #3269

merged 1 commit into from
Jun 5, 2023

Conversation

yuvalsw
Copy link
Contributor

@yuvalsw yuvalsw commented May 30, 2023

This change is Reviewable

@yuvalsw yuvalsw requested a review from orizi May 30, 2023 13:42
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 21 files at r1, all commit messages.
Reviewable status: 2 of 21 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)


corelib/src/starknet/secp256.cairo line 12 at r1 (raw file):

#[derive(Copy, Drop)]
extern type Secp256EcPoint;

re-separate to k1 and r1.

Code quote:

#[derive(Copy, Drop)]
extern type Secp256EcPoint;

corelib/src/starknet/secp256.cairo line 14 at r1 (raw file):

extern type Secp256EcPoint;

trait Secp256 {

and probably filename as well - just as to not make it look too usable for users.

Suggestion:

trait Secp256Generic

@yuvalsw yuvalsw force-pushed the yuval/secp256r1 branch 2 times, most recently from 699f876 to fd42607 Compare June 1, 2023 10:48
Copy link
Contributor Author

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 23 files reviewed, 2 unresolved discussions (waiting on @orizi)


corelib/src/starknet/secp256.cairo line 12 at r1 (raw file):

Previously, orizi wrote…

re-separate to k1 and r1.

Done.


corelib/src/starknet/secp256.cairo line 14 at r1 (raw file):

Previously, orizi wrote…

and probably filename as well - just as to not make it look too usable for users.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 21 files at r1, 11 of 16 files at r3, all commit messages.
Reviewable status: 18 of 23 files reviewed, 1 unresolved discussion (waiting on @yuvalsw)


corelib/src/starknet/secp256k1.cairo line 51 at r3 (raw file):

    ) -> Option<Secp256k1EcPoint> {
        super::secp256_trait::recover_public_key::<Secp256k1EcPoint>(msg_hash, r, s, y_parity)
    }

i think these should just be out of the trait and the impl (as using the function just through the module is much more neat)

Code quote:

    fn recover_public_key(
        msg_hash: u256, r: u256, s: u256, y_parity: bool
    ) -> Option<Secp256k1EcPoint> {
        super::secp256_trait::recover_public_key::<Secp256k1EcPoint>(msg_hash, r, s, y_parity)
    }

@yuvalsw
Copy link
Contributor Author

yuvalsw commented Jun 1, 2023

corelib/src/starknet/secp256k1.cairo line 51 at r3 (raw file):

Previously, orizi wrote…

i think these should just be out of the trait and the impl (as using the function just through the module is much more neat)

Why is it more neat?
Why specifically this?
This is part of the interface for secp, why not in the trait and impl?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 18 of 23 files reviewed, 1 unresolved discussion (waiting on @yuvalsw)


corelib/src/starknet/secp256k1.cairo line 51 at r3 (raw file):

Previously, yuvalsw wrote…

Why is it more neat?
Why specifically this?
This is part of the interface for secp, why not in the trait and impl?

Not just this one, all the outer ones, as the aim of this trait was to more easily implement the generics.
The usage itself from the user side would still be more fitting if it is just a normal module and not an impl.
Impl would have made more sense to me if some type would have carried all the secp information, and than a trait and impl would easily allow users to access it, which is not the case here.

Copy link
Contributor Author

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewable status: 18 of 23 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/starknet/secp256k1.cairo line 51 at r3 (raw file):

Previously, orizi wrote…

Not just this one, all the outer ones, as the aim of this trait was to more easily implement the generics.
The usage itself from the user side would still be more fitting if it is just a normal module and not an impl.
Impl would have made more sense to me if some type would have carried all the secp information, and than a trait and impl would easily allow users to access it, which is not the case here.

I think the interface should be consistent.
With your suggestion, a test would now look like:

    let public_key = secp256_trait::recover_public_key::<Secp256k1EcPoint>(msg_hash, r, s, y_parity).unwrap();
    let (x, y) = Secp256k1Impl::secp256_ec_get_coordinates_syscall(public_key).unwrap_syscall();

(adding to the inconsistency, with it now being called secp256_trait, it's even weirder to have function in it as a module)

I prefer leaving it as is, let me know if you strongly object.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 18 of 23 files reviewed, 1 unresolved discussion (waiting on @yuvalsw)


corelib/src/starknet/secp256k1.cairo line 51 at r3 (raw file):

Previously, yuvalsw wrote…

I think the interface should be consistent.
With your suggestion, a test would now look like:

    let public_key = secp256_trait::recover_public_key::<Secp256k1EcPoint>(msg_hash, r, s, y_parity).unwrap();
    let (x, y) = Secp256k1Impl::secp256_ec_get_coordinates_syscall(public_key).unwrap_syscall();

(adding to the inconsistency, with it now being called secp256_trait, it's even weirder to have function in it as a module)

I prefer leaving it as is, let me know if you strongly object.

What I want is to simply leave the original interface for the user as is, and use the trait just to reduce the code duplication between r1 and k1

@yuvalsw yuvalsw force-pushed the yuval/secp256r1 branch 3 times, most recently from 893e30a to 0d9cce6 Compare June 4, 2023 14:15
Copy link
Contributor Author

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewable status: 18 of 23 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/starknet/secp256k1.cairo line 51 at r3 (raw file):

Previously, orizi wrote…

What I want is to simply leave the original interface for the user as is, and use the trait just to reduce the code duplication between r1 and k1

Done.

@yuvalsw yuvalsw force-pushed the yuval/secp256r1 branch from 0d9cce6 to 1999bef Compare June 4, 2023 14:17
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 5 files at r4, 5 of 5 files at r5, 11 of 11 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

@yuvalsw yuvalsw force-pushed the yuval/secp256r1 branch 2 times, most recently from c92711e to 22b6bcc Compare June 5, 2023 10:19
Copy link
Contributor Author

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 6 files at r7.
Reviewable status: 23 of 25 files reviewed, all discussions resolved (waiting on @orizi)

@yuvalsw yuvalsw force-pushed the yuval/secp256r1 branch from 22b6bcc to 5d25d99 Compare June 5, 2023 10:31
Copy link
Contributor Author

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r7, all commit messages.
Reviewable status: 24 of 25 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor Author

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 11 files at r6, 1 of 1 files at r8.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @yuvalsw)

@yuvalsw yuvalsw enabled auto-merge June 5, 2023 10:32
@yuvalsw yuvalsw added this pull request to the merge queue Jun 5, 2023
Copy link
Collaborator

@orizi orizi left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

Merged via the queue into main with commit 76dbaee Jun 5, 2023
@orizi orizi deleted the yuval/secp256r1 branch June 28, 2023 05:36
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.

2 participants