-
Notifications
You must be signed in to change notification settings - Fork 332
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
secp256r1 utils and is_valid_p256_signature #988
base: main
Are you sure you want to change the base?
Conversation
We've deployed an account on sepolia that can validate P256 signature. If you check the Scarb.toml from 0xknwn/starknet-modular-account, you can see it is based on this branch for now and that is has both the secp256k1 and secp256r1 utils/store from you project We have run a transaction on sepolia with that account:
[
"0xa1913d80b4b735ea849919d5636cb5e9",
"0x98fd57ced24ecea3e19b87540589788d",
"0x6a05a126fc09205b0a3a509ba0db9243",
"0x3e75ae4643235e4dad268f0a480ef3f5",
"0x0"
] Now if you check this typescript program, you can see that,:
[
"0xa1913d80b4b735ea849919d5636cb5e9",
"0x98fd57ced24ecea3e19b87540589788d",
"0x6a05a126fc09205b0a3a509ba0db9243",
"0x3e75ae4643235e4dad268f0a480ef3f5"
] So with OpenZeppelin/cairo-contracts with the store/utils for secp256r1 have allowed us to develop an account module that can validate a P256 signature. For the implementation, you can check the p256validator.cairo file in the project. |
Thanks for opening this PR, @0xknwn! I think this will be an awesome addition to the lib. We'll take a look as soon as we can |
2dbb797
to
d9a2744
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.
Thanks for the PR, @0xknwn! This looks very good
I left a few comments and suggestions—the most pressing being that test_secp256r1
tests aren't running
I will work on it tomorrow @andrew-fleming ! Thank you for the feedback and sorry if I did not include the tests. FYI, this is the transaction that uses the code on Sepolia https://sepolia.starkscan.co/tx/0x419d921d19fbb356ebe8f57be613b8d4d1880c5ad18768ba4e12baa566ec06 |
@0xknwn it's my pleasure and thank you for opening the PR!
No worries, this was easy to miss |
d9a2744
to
e6754f1
Compare
@andrew-fleming I have done the following to follow up with your feedback:
|
I did some additional investigations and I've now fixed the remaining issues, afaik:
Hopefully I did not miss anything else. Thank you for your patience. |
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.
I left a final suggestion. Otherwise, LGTM!
#[test] | ||
fn test_curve_size() { | ||
let curve_size = Secp256r1Impl::get_curve_size(); | ||
assert_eq!(curve_size, 0xffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551); | ||
assert_eq!(curve_size.low, 0xbce6faada7179e84f3b9cac2fc632551); | ||
assert_eq!(curve_size.high, 0xffffffff00000000ffffffffffffffff); | ||
} | ||
|
||
#[test] | ||
fn test_get_point_from_x_syscall_on_curve_size_is_none() { | ||
let curve_size = Secp256r1Impl::get_curve_size(); | ||
match Secp256r1Impl::secp256_ec_get_point_from_x_syscall(curve_size, true).unwrap_syscall() { | ||
Option::Some(_data) => { assert_eq!(true, false); }, | ||
Option::None => { assert_eq!(true, true); }, | ||
} | ||
|
||
match Secp256r1Impl::secp256_ec_get_point_from_x_syscall(curve_size, false).unwrap_syscall() { | ||
Option::Some(_data) => { assert_eq!(true, false); }, | ||
Option::None => { assert_eq!(true, true); }, | ||
} | ||
} |
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.
Using the curve size as x
in secp256k1 appears to have been a coincidence. It's also outside the scope of this PR, so we don't need to include these two tests. I appreciate the effort in adding them though!
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.
I have removed these 2 tests as requested.
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.
All good on my end!
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.
Hey @0xknwn! The PR is looking great, thanks again for taking the time. I left some small suggestions. Also, we are in the middle of a migration to the latest edition, which would conflict with this PR if we merge it now. It would be great if you wait for that one to be merged, and then update this PR. If not we will be happy to update it and merge it later.
@@ -0,0 +1,2 @@ | |||
[files] | |||
extend-exclude = ["test_p256_account.cairo"] |
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.
Why are we excluding this file?
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.
Because your tool that checks for syntax consider I do spell some hexa value wrong and I do not know any other ways to pass the associated test. see https://github.com/OpenZeppelin/cairo-contracts/actions/runs/9023095283/job/24794108410 for details. What is a better way to fix it?
// | ||
|
||
/// This signature was computed using @noble/curves. | ||
fn P256_PRIVATEKEY_SAMPLE() -> u256 { |
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.
Since we are not verifying signatures for this test, Is this function needed?
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.
see comment my next comment. I will remove it, once we agree on what to name it.
} | ||
|
||
fn get_points() -> (Secp256r1Point, Secp256r1Point) { | ||
let private_key = P256_PRIVATEKEY_SAMPLE(); |
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.
Do we need x to be a private key? The name seems a bit misleading.
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.
I can replace the x value on the curve by a const and remove the function. Whatever you want but tell me what you prefer otherwise we may spin around:
- do you want a const or simply the value?
- if you prefer a const or a function, how would you like I name it?
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.
In this case, I think the value is okay.
|
||
#[test] | ||
fn test_pack_big_secp256r1_points() { | ||
let (big_point_1, big_point_2) = get_points(); |
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.
Are these points big points? The idea of this test is to check that the packing is right even for big values, maybe it makes sense to use just big values even if they are not valid points.
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.
Can you define big points or point me in the right direction so that I can build values that would make sense for that test?
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.
A point with x big enough for the high part of the u256 to use as many bits as possible would be the best case, but making sure that at least the high part is not 0 should suffice.
@@ -0,0 +1,29 @@ | |||
use openzeppelin::account::interface::P256PublicKey; |
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.
Since we don't have this account in the library yet, I would move this into the test_secp256r1 test file.
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.
You do not but we do, see https://github.com/0xknwn/starknet-modular-account/blob/749a0485b0f50b9350f8f41eb1898e88f2f3a195/src/modules/p256validator/p256validator.cairo#L17 . The reason behind this PR is because we use your project as a library to develop a modular account. It makes sense that we use the same tool for secp256k1 and secp256r1 and that you provide the technology. For that purpose, it really makes sense types are kept in files that are not test and Eth and P256 public keys are in a parallel location
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.
Got it. I'm not recommending to move the type though, but the contents of this file (to move test_p256_account.cairo
contents into test_secp256r1.cairo
). Sorry if it was a bit confusing.
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.
Okay. I will do!
@ericnordelo, I guess it was a rhetorical question 🤣. jk! I am not in a rush: I have forked your repository and our project is using the fork. So let's proceed that way, I will rebase on 0.14 once out and will rework this PR. Can you please review my answers to your comments and let me know what you think? |
Hey @0xknwn! We've already finished migrating to foundry and workspaces, and we are more than happy to prioritize getting this merged since it's been stalled for a while. Would you happen to have the time to continue working on it with our feedback, or would you prefer us to take the lead on finishing it? |
Fixes #987
PR Checklist