Open 0xknwn opened 1 month ago
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:
0x419d921d19fbb356ebe8f57be613b8d4d1880c5ad18768ba4e12baa566ec06
[
"0xa1913d80b4b735ea849919d5636cb5e9",
"0x98fd57ced24ecea3e19b87540589788d",
"0x6a05a126fc09205b0a3a509ba0db9243",
"0x3e75ae4643235e4dad268f0a480ef3f5",
"0x0"
]
Now if you check this typescript program, you can see that,:
0x1efecf7ee1e25bb87098baf2aaab0406167aae0d5ea9ba0d31404bf01886bd0e
0x419d921d19fbb356ebe8f57be613b8d4d1880c5ad18768ba4e12baa566ec06
0x98fd57ced24ecea3e19b87540589788da1913d80b4b735ea849919d5636cb5e9
0x3e75ae4643235e4dad268f0a480ef3f56a05a126fc09205b0a3a509ba0db9243
[
"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
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!
sorry if I did not include the tests.
No worries, this was easy to miss
@andrew-fleming I have done the following to follow up with your feedback:
test_secp256r1
in src/tests/account.cairo
- obviously tests are failing, now! I am working on this...src/tests/account/test_p256_account.cairo
_u256
from src/tests/account/test_p256_account.cairo
as you are right, it is not neededPanicked with 0x4f7074696f6e3a3a756e77726170206661696c65642e ('Option::unwrap failed.').
I've also figured out that SIGNED_TX_DATA() is actually not referenced in the tests so is just here to document. I will let you know if I can get deeper into the get_curve_size
issue.
@andrew-fleming ,
I did some additional investigations and I've now fixed the remaining issues, afaik:
test_curve_size
to show it is the case. As a matter of fact, it is hardcoded in the corelib.secp256_ec_get_point_from_x_syscall
returns None
when used with curve_size as a private key. You would have to ask someone to understand why. TL;DR: (1) I have added test_get_point_from_x_syscall_on_curve_size_is_none
to document that case and (2) if that is not the expected behaviour, the issue is in the corelib and not in your code anywayfn P256_PRIVATEKEY_SAMPLE()
helper function has been added into the code for that purpose. All the tests from test_secp256r1.cairo
are now passing.Hopefully I did not miss anything else. Thank you for your patience.
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.
@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?
Fixes #987
PR Checklist