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

Removes ethereumjs-util and keccak dependencies. #357

Merged
merged 1 commit into from
Jul 4, 2019

Conversation

MicahZoltu
Copy link
Contributor

@MicahZoltu MicahZoltu commented May 5, 2019

Replaces both with js-sha3. ethereumjs-util and keccak NPM packages both have native bindings in their dependency tree which means they depend on node-gyp along the way. js-sha3 on the other hand is a JavaScript-native implementation of the sha3 family of hashing functions, which relieves a number of node-gyp and native binding related issues.

Fixes #356
Fixes #336
Part of #361

@MicahZoltu
Copy link
Contributor Author

Is there anything I can do to expedite this getting merged?

@MicahZoltu
Copy link
Contributor Author

I believe the coveralls coverage decrease is because I deleted some lines that were previously covered, so the number of covered lines as a percentage went down (uncovered lines have more weight as of this change).

@axic
Copy link
Member

axic commented Jul 4, 2019

Sorry for the delay. I see a reason to not use two packages for keccak256, but what's wrong with ethereumjs-util? It is supposed to be well maintained.

@MicahZoltu
Copy link
Contributor Author

  1. Depending on ethereumjs-utils brings in 30 transitive dependencies.
  2. Depending on js-sha3 brings in 0 transitive dependencies.
  3. The only function needed is keccak256.
  4. A couple of transitive dependencies of ethereumjs-utils depend on node-gyp, which causes no end of problems.

@axic
Copy link
Member

axic commented Jul 4, 2019

I'd prefer depending on keccak as that has a more optimised implementation, but apparently your opinion is it is riddled with node-gyp. Wish the JS ecosystem wasn't this broken.

@coveralls
Copy link

coveralls commented Jul 4, 2019

Coverage Status

Coverage decreased (-0.2%) to 57.267% when pulling f441441 on MicahZoltu:js-sha3 into ea40b79 on ethereum:master.

@MicahZoltu
Copy link
Contributor Author

Keccack256 is called in two places, nowhere near a hot path from what I can tell. Given the many orders of magnitude slower the compilation process is than hashing, I think choosing the native library for its performance benefits at the cost of increasing dev complexity is very much not worth it.

Replaces both with `js-sha3`.  `ethereumjs-util` and `keccak` NPM packages both have native bindings in their dependency tree which means they depend on node-gyp along the way.  `js-sha3` on the other hand is a native implementation of the sha3 family of hashing functions, which relieves a number of `node-gyp` and native binding related issues.
@axic
Copy link
Member

axic commented Jul 4, 2019

The use in downloadCurrentVersion doesn't matter, that is a publishing level tool.

The use in linker.js matters, because that is a helper to do linking of binaries and is used by Remix, Truffle, etc.

@axic axic merged commit bedf80d into ethereum:master Jul 4, 2019
@MicahZoltu MicahZoltu deleted the js-sha3 branch July 4, 2019 23:13
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.

Remove dependency on keccak NPM module. keccak issue while installing solc in ubuntu
3 participants