secp256r1 utils and is_valid_p256_signature

[0xknwn]

Fixes #987

PR Checklist

• [x] Tests
• [ ] Documentation <- From what we see there is currently no documentation associated with the secp256k1 version of those utilities and we do not know where or how to add those. We would suggest leave it that way. Feel free to comment this PR.
• [x] Added entry to CHANGELOG.md
• [x] Tried the feature on a public network

[0xknwn]

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:

• the transaction hash on sepolia is 0x419d921d19fbb356ebe8f57be613b8d4d1880c5ad18768ba4e12baa566ec06
• the transaction has succeeded from the receipt
• the signature is

[
    "0xa1913d80b4b735ea849919d5636cb5e9",
    "0x98fd57ced24ecea3e19b87540589788d",
    "0x6a05a126fc09205b0a3a509ba0db9243",
    "0x3e75ae4643235e4dad268f0a480ef3f5",
    "0x0"
  ]

Now if you check this typescript program, you can see that,:

• using the following private key: 0x1efecf7ee1e25bb87098baf2aaab0406167aae0d5ea9ba0d31404bf01886bd0e
• on that message (i.e. the tx hash): 0x419d921d19fbb356ebe8f57be613b8d4d1880c5ad18768ba4e12baa566ec06
• generates the following signature:
  • r: 0x98fd57ced24ecea3e19b87540589788da1913d80b4b735ea849919d5636cb5e9
  • s: 0x3e75ae4643235e4dad268f0a480ef3f56a05a126fc09205b0a3a509ba0db9243
• which are 2 uint256 that you can format: [r.low, r.high, s.low, s.high] to get:

[
  "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.

[andrew-fleming]

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

[0xknwn]

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

[andrew-fleming]

@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

[0xknwn]

@andrew-fleming I have done the following to follow up with your feedback:

1. rebase on main as it is right now (i.e. 0.13.0)
2. nitpick: remove the empty line
3. added test_secp256r1 in src/tests/account.cairo - obviously tests are failing, now! I am working on this...
4. remove the traits that are useless from src/tests/account/test_p256_account.cairo
5. removed the _u256 from src/tests/account/test_p256_account.cairo as you are right, it is not needed
6. I am working on the tests now: you are right, I have 5 failing and they are all failing with get_curve_size (as you've reproduced) on the following error:

Panicked 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.

[0xknwn]

@andrew-fleming ,

I did some additional investigations and I've now fixed the remaining issues, afaik:

• the p256 curve size is returned correctly: I have added 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 anyway
• I have added and used a sample private key to test the code remaining code. The fn 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.

[0xknwn]

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?

[ericnordelo]

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?

[ericnordelo]

Hey @0xknwn, thanks again for taking the time to contribute. Since this PR has been inactive, we are taking the issue for the final push. Superseded by #1189.