-
Notifications
You must be signed in to change notification settings - Fork 358
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
Add is_valid_secp256_signature
support
#1189
Add is_valid_secp256_signature
support
#1189
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1189 +/- ##
==========================================
+ Coverage 91.57% 91.61% +0.03%
==========================================
Files 47 47
Lines 1223 1228 +5
==========================================
+ Hits 1120 1125 +5
Misses 103 103
Continue to review full report in Codecov by Sentry.
|
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.
Good job! Left a couple of minor comments
|
||
(x.low.into(), xhigh_and_parity) | ||
(x.low.into(), xhigh_and_parity.try_into().unwrap()) |
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.
parity
could be represented by felt252 and x.high
can be also converted to felt with into
call. This way we could avoid having try_into
and unwrap
calls. 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.
I tried to avoid it, but since y is a u256, to make parity a felt252 we would also need to .try_into().unwrap()
y % 2. But we can make parity a u128 using y.low % 2 and also remove the .try_into().unwrap()
.
Updated!
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 was thinking about let parity: felt252 = y.low % 2 == 0 ? 0 : 1
. But this solution also works since we've got rid of try_into
!
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
…cairo-contracts into feat/secp256r1-support-#987
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.
LGTM
Fixes #987
PR Checklist