Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove crypto API usage #41

wants to merge 2 commits into from

Conversation

junderw
Copy link
Member

@junderw junderw commented Jun 1, 2020

Resolves #40

@RyanZim RyanZim requested a review from jprichardson June 1, 2020 11:59
@RyanZim
Copy link
Member

RyanZim commented Jun 1, 2020

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

@jprichardson
Copy link
Member

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.

Copy link
Member

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@RyanZim
Copy link
Member

RyanZim commented Jun 1, 2020

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

@alcuadrado
Copy link
Contributor

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
Copy link
Member Author

junderw commented Jun 1, 2020

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
Copy link
Member Author

junderw commented Jun 1, 2020

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
Copy link
Contributor

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
Copy link
Member

RyanZim commented Jun 23, 2020

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replacing the module crypto
4 participants