cryptocoinjs / hdkey

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

update secp256k1 to v4.0 #32

Closed homura closed 4 years ago

homura commented 4 years ago

Since secp256k1 v4.0 use the N-API, this can avoid some unexpected situations similar to when worker_thread calls binding and use Pure JS version

Try the following code, and run with secp256k1@3.8 and secp256k1@4.0, you will find that in the worker_thread, the error caused by Module did not self-register will cause its performance to decrease due to the implementation of the JS version

const { Worker, isMainThread } = require("worker_threads");

process.env.DEBUG = "*";

if (isMainThread) {
  new Worker(__filename);
  const secp256k1 = require("secp256k1");

} else {
  const secp256k1 = require("secp256k1");
}
junderw commented 4 years ago

utACK it looks good to me.

junderw commented 4 years ago

Bonus: found #33 when verifying this code change.

holgerd77 commented 4 years ago

Any chance this gets merged and released soon?

holgerd77 commented 4 years ago

Hi, just wanted to give this a cautious iteration: we have an outstanding release - see https://github.com/ethereumjs/ethereumjs-wallet/issues/113 - and a merge and subsequent release here would substantially improve the installation experience since one main part was to update the secp256k1 dependencies to v4.0 coming with the pre-build binaries.

If not possible so be it but otherwise a release here would be super cool! 😄

jprichardson commented 4 years ago

@holgerd77 thanks. I had no idea this was used in ethereumjs-wallet. Will have this merged and updated.

holgerd77 commented 4 years ago

@jprichardson Thanks, that's so great! 😄

RyanZim commented 4 years ago

Published in v2.0.0 :tada:

holgerd77 commented 4 years ago

Thanks for the release, great! 😄

We have some tests failing now with the updated version. Didn't get a chance to have a look yet, but just to already raise some awareness here.