-
-
Notifications
You must be signed in to change notification settings - Fork 2
Remove ecdsa dependency, use cryptography only II #4
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
base: master
Are you sure you want to change the base?
Conversation
e0b0f49 to
ecd81f3
Compare
b05ea8b to
a9c7320
Compare
a9c7320 to
16c914e
Compare
| return 0 < key < self.curve.order | ||
| return 0 < key < self.order | ||
|
|
||
| def add_points(self, first: bytes, second: bytes) -> bytes: |
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.
How does this function handle if one of the inputs is a point at infinity? Does from_encoded_point() raise an exception? I think the only way to get infinity as an input is if the tweak payload[:32] comes out zero, which is practically impossible to observe. Maybe privkey_to_pubkey() already raises an exception in that case. We should at least have a comment explaining how this function behaves for infinity inputs and why it's OK.
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.
Besides that, I also realized that privkey_to_pubkey() raises an exception if privkey is greater than the curve's order, which would break this test vector, if we had it included in our tests.
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.
We have that test vector included in our tests, but it tests retry in master key generation. We should generate a test vector for retry in child key derivation too and add it to the spec.
This replaces #2. Unlike that, I've tried to use the cryptography package as much as possible.