-
Notifications
You must be signed in to change notification settings - Fork 12.4k
Implement P256 verification via RIP-7212 precompile with Solidity fallback #4881
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
82 commits
Select commit
Hold shift + click to select a range
5e82076
Add P256 implementation and testing
Amxx da0f27e
enable optimizations by default
Amxx aa59c67
test recovering address
Amxx 9512947
improved testing
Amxx a60bf48
spelling
Amxx 9185026
fix lint
Amxx 025e360
expose imports tick
Amxx 803e735
fix lint
Amxx 57fcecd
fix lint
Amxx 4dae298
add changeset
Amxx 6cf039d
improve doc
Amxx c094fa1
add envvar to force allowUnlimitedContractSize
Amxx 20a03df
fix lint
Amxx 15f1a6b
fix stack too deep error in coverage
Amxx e2040e4
reoder arguments to match ecrecover and EIP-7212
Amxx 695b732
reduce diff
Amxx 41aaf71
Merge branch 'master' into feature/P256
Amxx 3bf4557
Update contracts/utils/cryptography/P256.sol
Amxx 3cbf426
Merge branch 'master' into feature/P256
Amxx bba7fa3
update pseudocode reference
Amxx 2812ed8
Update contracts/utils/cryptography/P256.sol
Amxx e0ef63b
refactor neutral element in jAdd
Amxx 61a244d
add EIP-7212 support
Amxx 910bc71
Merge branch 'master' into feature/P256
Amxx 2e9d04d
Apply PR suggestions
ernestognw 9062633
move invModPrime to Math.sol
Amxx a44bb71
update
Amxx 3a6e1f5
update
Amxx 3e71fad
codespell
Amxx 887272b
test signature maleability
Amxx 433548f
Iterate
ernestognw 4f80ca0
Add more comments
ernestognw be69f5c
remove P256 public key to address derivation
Amxx fcde23f
Move publicKey from privateKey derivation function to tests
ernestognw 5828566
Remove unnecessary test
ernestognw 9362936
add wycheproof test
cairoeth 921745b
Readd malleability check and rename
ernestognw 2c113f4
Change arguments to bytes32
ernestognw fb7dc6f
remove unused malleable version
Amxx f264dae
Update contracts/utils/cryptography/P256.sol
Amxx 2c9a137
Update contracts/utils/cryptography/P256.sol
Amxx f4cbf51
up
Amxx 0227656
recovery malleability
Amxx e3a8338
fix bug (inverse return values)
Amxx cbd2ff5
better private key gen
cairoeth 194f19a
Update contracts/utils/cryptography/P256.sol
ernestognw 704a12e
Fix hardhat tests and add documentation
ernestognw 61d52a5
Update test/utils/cryptography/P256.test.js
Amxx 242c796
Ensure lower s in Foundry tests
ernestognw 787834d
Lint
ernestognw d8f4f7e
fix bug for valid signatures with large `r` values
cairoeth fc54017
run original wycheproof in hardhat
Amxx 5a7887b
Merge remote-tracking branch 'amxx/feature/P256' into feature/P256
Amxx cc82c17
Update test/utils/cryptography/P256.test.js
Amxx a67e5a2
Almost fix tests
ernestognw 4c93009
Bound r to N so for lower s values
ernestognw 046463c
Remove unnecessary comment
ernestognw e4df1d1
Remove foundry wycheproof
ernestognw 1bddcf5
Tests nit
ernestognw e5ba358
Update .changeset/odd-lobsters-wash.md
ernestognw 2eecacf
Update test/utils/cryptography/P256.t.sol
ernestognw ced4fb8
Update P256.t.sol
Amxx b82af11
Merge branch 'master' into feature/P256
ernestognw c6a86d9
Add more docs and nit
ernestognw 9b24014
Manage to compile without via-ir
ernestognw 3616771
Improve comments
ernestognw be078b1
Remove unnecessary CI flag
ernestognw ecd3aa2
cleanup _jAdd with memory
Amxx d83e707
up
Amxx fbc11f5
Update contracts/utils/cryptography/P256.sol
Amxx 9c88101
Apply suggestions from code review
Amxx b5e6bd7
Update hardhat.config.js
Amxx db76353
Update hardhat.config.js
Amxx 0722d93
Update hardhat.config.js
Amxx 306a5f6
Revert all changes to hardhat.config.js
Amxx e67a456
uniform style
Amxx 1a8cb63
add bound checks to isOnCurve
Amxx 3c3fa27
rename isOnCurve -> isValidPublicKey + add _isProperSignature helper
Amxx 2fe4a16
Update contracts/utils/cryptography/P256.sol
ernestognw 2420d13
Update contracts/utils/cryptography/P256.sol
ernestognw 49f3ad9
Merge branch 'master' into feature/P256
ernestognw 5314727
Enable --ir-minimum in forge coverage
ernestognw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
update
- Loading branch information
commit 3a6e1f59307b357b8b10b8383a04ceed5ab0cb85
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Any reason for representing the hash as
uint256? I think we're generally more used tobytes32There 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.
No real reason. All params could be either uint256 or bytes32.
I think that is since the verification is math heavy, we'll need to cast everything into uint256 anyway.
Uh oh!
There was an error while loading. Please reload this page.
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.
My concern is that users may need to cast in the frontend using Javascript where the encoding can be a mess. Although it's trivial to cast using ethers or viem, I don't think it's as straightforward as doing it natively.
We can hide the interface and expose only
bytes32What do you think?
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.
That really depend on the library you are using. In ethers.js, uint256 is BigNumberish which accepts many format, including hex string and buffers. It is actually less restictive than bytes32.
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'm be ok with changing the types, but not through one more variant of the function (override). We have enough already. More function is more complex testing, more confusion.
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.
Yes, agree on not adding more functions. Let's change only the types. I'll push a commit
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.
IMO should be
bytes32.uinttypes have arithmetic operations which should not be done on these values.