hdkey icon indicating copy to clipboard operation
hdkey copied to clipboard

Remove crypto API usage

Open junderw opened this issue 4 years ago • 9 comments

Resolves #40

junderw avatar Jun 01 '20 00:06 junderw

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

RyanZim avatar Jun 01 '20 14:06 RyanZim

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.

jprichardson avatar Jun 01 '20 14:06 jprichardson

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

RyanZim avatar Jun 01 '20 15:06 RyanZim

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.

alcuadrado avatar Jun 01 '20 18:06 alcuadrado

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 avatar Jun 01 '20 22:06 junderw

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)

junderw avatar Jun 01 '20 22:06 junderw

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.

alcuadrado avatar Jun 03 '20 14:06 alcuadrado

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?

RyanZim avatar Jun 23 '20 18:06 RyanZim

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

alcuadrado avatar Jun 23 '20 19:06 alcuadrado