-
Notifications
You must be signed in to change notification settings - Fork 74
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
Remove crypto API usage #41
base: master
Are you sure you want to change the base?
Conversation
Is this semver-minor or semver-patch? It doesn't change the API, but it does change the prerequisites. |
No strong feelings here, but IMHO changing deps like this can have some unintended consequences downstream that it should be semver-major to play it safe even though a new major was just released. That being said, if we don't go with semver-major, at minimum semvor-minor then. |
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.
utACK
@alcuadrado if you could take a look at this, that'd be great. |
Thanks for implementing this change, @junderw ! I'm really glad this is going to be implemented. It will greatly simplify the project I described in #40. I'm afraid I created this repo that reproduces the problem: https://github.com/alcuadrado/hdkey-consumed-with-rollup A possible solution is to use const {ripemd160} = require("hash.js/lib/hash/ripemd");
const Sha256 = require("hash.js/lib/hash/sha/256");
// ...
function hash160 (buf) {
var sha = Buffer.from(new Sha256().update(buf).digest())
return Buffer.from(new ripemd160().update(sha).digest())
}
|
From a logic standpoint your code makes sense and gets an ACK from me. However, hash.js buy-in as a dependency is one step above create-hash. Since the maintainers of hdkey don't have direct control / publish rights to hash.js So this will likely be a decision for @jprichardson and @RyanZim |
Travis is passing. So now the decision basically comes down to:
|
Also, note that at least one of the |
Sorry so long in getting back to this.
I cloned your repository, installed it, and it's working perfectly fine for me. What error are you getting? |
Resolves #40