This repository has been archived by the owner on May 23, 2023. It is now read-only.
Switched to coincurve #713
Closed
Conversation
This file contains 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
|
@cag Thanks for the interest in coincurve.
Let me know want you'd like me to do :) |
|
To keep compatibility with the current API while promoting interoperability
with other APIs, I recommend allowing hashlib objects and None to be the
hasher parameter. A check to see that the result of this type juggling is a
32 long bytes should be fine. Let me know if you decide to update the
library so I can update this PR accordingly.
…On Apr 28, 2017 2:47 PM, "Ofek Lev" ***@***.***> wrote:
@cag <https://github.com/cag> Thanks for the interest in coincurve.
1. I'm open to accepting None for hasher param to indicate no hashing
is needed. However, please keep in mind that if we choose to do that then
it would be necessary to check that the message is already 32 bytes long.
It's an ECDSA invariant that must be maintained due to our particular
curve's group.
2. In the spirit of giving users as many options as possible, I won't
mandate hasher have any particular API e.g. hasher(data).digest().
Coincurve will continue assuming hasher(data) returns the necessary
hash as bytes. After all, users might want to use a non-stdlib hashing
function. Also fyi, hasher does default to sha256.
Let me know want you'd like me to do :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#713 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASNz-jD1BUwNcAw9jdBbFE4ReUmX5taks5r0kJjgaJpZM4NL3Ix>
.
|
|
@cag Travis will deploy 4.5.0 soon ofek/coincurve@6da9164 |
|
Thanks! I'll update my PR when I get a chance to sit down
…On Apr 28, 2017 3:07 PM, "Ofek Lev" ***@***.***> wrote:
@cag <https://github.com/cag> Travis will deploy 4.5.0 soon
***@***.***
<ofek/coincurve@6da9164>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#713 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASNzzJUsGACV9QjcV6sovb96Y3NwG4rks5r0kcJgaJpZM4NL3Ix>
.
|
|
I'll inform you when deploy is finished as Travis is choking today https://travis-ci.org/ofek/coincurve/builds/226951924 |
|
@cag 4.5.1 is ready |
|
Looks good. I've updated the PR accordingly.
…On Apr 29, 2017 1:46 PM, "Ofek Lev" ***@***.***> wrote:
@cag <https://github.com/cag> 4.5.1 is ready
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#713 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASNzwOBrN9GtTaCs6gIFiCgOHw9EDQgks5r04WTgaJpZM4NL3Ix>
.
|
|
All tests pass. |
|
@cag Hey could you please rebase when you get a chance? |
|
Good lord, I tried rebasing and got hit with a ton of py3.6 compat issues and other such things. :( Just confirmed |
|
What tests? |
|
This repo's tests. I am getting the same as TravisCI, and I can't really test any changes to until this gets resolved: |
|
@cag Could you please see if this works now? |
|
Been fairly busy, but I promise I'll get back to this soon
…On Jul 19, 2017 6:27 PM, "Ofek Lev" ***@***.***> wrote:
@cag <https://github.com/cag> Could you please see if this works now?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#713 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASNz4syvgaUAx-vuHnpDv_eBN3G9Ehrks5sPi5YgaJpZM4NL3Ix>
.
|
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
See #708
Basically
secp256k1-pylooks like it is not maintained anymore. I don't know where the original maintainer went, butcoincurveseems actively maintained, so here it is.@ofek I do have one gripe with this migration: I don't like having to use the identity function as the hasher parameter: that feels like a hack. As far as deviating from the original API goes, I feel like the right move would have been to accept hashlib objects, default to
hashlib.sha256, and haveNoneindicate a lack of a need for a hasher. Still, this is up for debate, and I don't want to throw the baby out with the bathwater, as this library is still overall an improvement.P.S. I had to regenerate my tox environment in order to test this with tox.