cryptocoinjs / hdkey

JavaScript component for Bitcoin hierarchical deterministic keys (BIP32)
MIT License
201 stars 74 forks source link

Remove crypto API usage #41

Open junderw opened 4 years ago

junderw commented 4 years ago

Resolves #40

RyanZim commented 4 years ago

Is this semver-minor or semver-patch? It doesn't change the API, but it does change the prerequisites.

jprichardson commented 4 years ago

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.

RyanZim commented 4 years ago

@alcuadrado if you could take a look at this, that'd be great.

alcuadrado commented 4 years ago

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 create-hash doesn't work well with Rollup either. As far as I can tell, it's a limitation in its implementation of Node.js' streams.

I created this repo that reproduces the problem: https://github.com/alcuadrado/hdkey-consumed-with-rollup You just have to install it and run npm test to see how it breaks.

A possible solution is to use hash.js instead of create-hash. Something like this:

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())
}

hash.js doesn't use Node builtin's by default, so this may mean a tiny performance regression. This can be avoided using the browser field of package.json and having two different versions.

junderw commented 4 years ago

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

junderw commented 4 years ago

Travis is passing. So now the decision basically comes down to:

  1. Which type of semver bump?
  2. Do we want to add hash.js as dependency? (notably hash.js is not managed by the same group as create-hash, but one of the group's members)
alcuadrado commented 4 years ago

notably hash.js is not managed by the same group as create-hash, but one of the group's members

Also, note that at least one of the cyptocoinjs' packages depends on Fedor Indutny's packages, as secp256k1 depends on elliptic.

RyanZim commented 4 years ago

Sorry so long in getting back to this.

I created this repo that reproduces the problem: https://github.com/alcuadrado/hdkey-consumed-with-rollup You just have to install it and run npm test to see how it breaks.

I cloned your repository, installed it, and it's working perfectly fine for me. What error are you getting?

alcuadrado commented 4 years ago

Thanks for getting back to this @RyanZim

The tests now pass because when running npm i you get the latest version of @junderw's branch, which already has the fix.

To reproduce the error you should run npm ci && npm test