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.
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
Implement P256 verification via RIP-7212 precompile with Solidity fallback #4881
base: master
Are you sure you want to change the base?
Implement P256 verification via RIP-7212 precompile with Solidity fallback #4881
Changes from 21 commits
5e82076
da0f27e
aa59c67
9512947
a60bf48
9185026
025e360
803e735
57fcecd
4dae298
6cf039d
c094fa1
20a03df
15f1a6b
e2040e4
695b732
41aaf71
3bf4557
3cbf426
bba7fa3
2812ed8
e0ef63b
61a244d
910bc71
2e9d04d
9062633
a44bb71
3a6e1f5
3e71fad
887272b
433548f
4f80ca0
be69f5c
fcde23f
5828566
9362936
921745b
2c113f4
fb7dc6f
f264dae
2c9a137
f4cbf51
0227656
e3a8338
cbd2ff5
194f19a
704a12e
61d52a5
242c796
787834d
d8f4f7e
fc54017
5a7887b
cc82c17
a67e5a2
4c93009
046463c
e4df1d1
1bddcf5
e5ba358
2eecacf
ced4fb8
b82af11
c6a86d9
9b24014
3616771
be078b1
ecd3aa2
d83e707
fbc11f5
9c88101
b5e6bd7
db76353
0722d93
306a5f6
e67a456
1a8cb63
3c3fa27
2fe4a16
2420d13
49f3ad9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 tobytes32
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.
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.
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
bytes32
What 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
.uint
types have arithmetic operations which should not be done on these values.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.
So here the if is optional.
If we remove the if, we are going to load
points[0]
which is(0,0,0)
... and the_jAdd
will skip that as the "neutral element". The if here as a cost. 15/16 we pay it for no real reason (and we still pay the check in _jAdd). 1/16 the if avoids the overhead of a function call.I'm going to benchmark which one is better and comment that so we don't go back and forward.
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 checked, skipping the mloads in 1/16 cases is a bigger gain than the loss of the if in the other 15/16 cases. Keeping the if is the more effective solution here