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

Replacing the module crypto #40

Open
alcuadrado opened this issue May 29, 2020 · 8 comments · May be fixed by #41
Open

Replacing the module crypto #40

alcuadrado opened this issue May 29, 2020 · 8 comments · May be fixed by #41

Comments

@alcuadrado
Copy link
Contributor

alcuadrado commented May 29, 2020

Hi,

I'm opening this issue to know if you are open to replacing the use of crypto for more browser-friendly libraries like create-hmac and randombytes.

I've had a hard time using this library with Rollup, which led me to bundling my own version here: https://github.com/ethereum/js-ethereum-cryptography/tree/master/hdkey-without-crypto-config

I'd love not to have to do that, and the needed change is really small. I can prepare a PR for this, making sure that the builtin implementations are used on node.

@junderw
Copy link
Member

junderw commented May 30, 2020

bip32 library works with browsers.

Is there any functionality that bip32 doesn't offer that you need? Besides not having bitcoin in the name... lol

@alcuadrado
Copy link
Contributor Author

alcuadrado commented May 30, 2020

My main issue with bip32 is that it depends on tiny-secp256k1 which needs to be recompiled every time you install it. I used it in my main project until very recently.

Besides that, I don't see how your comment is related. I'm proposing a small change that would improve this lib's portability.

@junderw
Copy link
Member

junderw commented May 31, 2020

I don't see how your comment is related.

I don't see how tiny-secp256k1 compilation step is related to browsers, since we fallback to pure JS when packaging for browsers. If the compilation fails you will use pure JS in node environment too.


Since you were talking about browsers, I suggested a bip32 library that works in browsers out of the box. Asking why you don't use that helps me understand exactly what you are aiming for with this suggestion to hdkey library.

I don't know why hdkey doesn't use browser friendly dependencies, but by asking you why you didn't use one that did (bip32), it gives us more context and information for considering your request.

@alcuadrado
Copy link
Contributor Author

I don't see how tiny-secp256k1 compilation step is related to browsers, since we fallback to pure JS when packaging for browsers. If the compilation fails you will use pure JS in node environment too.

I'm leading an effort to remove native dependencies (except N-API based ones) from the Ethereum js ecosystem. Why?

  1. They are degrade the development experience by being slow to install.
  2. They tend to break with new Node.js major versions
  3. Many people, especially Windows users, don't have a proper setup to use them. This is incredibly common in educational contexts (e.g. a workshop at a conference). In those cases, many people just desist.

As part of this process, I'm also making sure that the dependencies being used work well with web bundlers and don't produce unnecessarily large bundles.

The problems with the crypto module are:

  1. It is normally be replaced by crypto-browserify, which is HUGE.
  2. It doesn't work with Rollup.

Why hdkey and not bip32? hdkey is more popular in the Ethereum ecosystem, so migrating to a portable version of it is easier than migrating to bip32. This is especially important because this effort requires the buy-in of multiple open source projects.

@junderw junderw linked a pull request Jun 1, 2020 that will close this issue
@coolaj86
Copy link
Contributor

coolaj86 commented Feb 22, 2022

In package.json you can specify a broswer key and then put your replacements in it. This is the de facto solution for this problem and supported by virtually every bundler system.

Example:

{
  "main": "hdkey.js",
  "browser": {
    "./lib/thingy.js": "./lib-browser/thingy.js",
    "foo-dep": "foo-browser-dep"
  }
}

Anything that installs this as a dependency would get the correct bundle for that bundle system.

@junderw
Copy link
Member

junderw commented Feb 22, 2022

@alcuadrado Also FYI tiny-secp256k1 has since moved to WASM, no more compilation, but now you have to deal with bundlers WASM support. (webpack works pretty well in my experience)

@alcuadrado
Copy link
Contributor Author

Thanks for the heads up, @junderw!

@coolaj86
Copy link
Contributor

coolaj86 commented Jan 26, 2023

New Breaking Version 3.0

I'm rewriting this today, in about 10 minutes (for probably 4-6 hours).

$DASH Code Hangout #6: WebCrypto & ArrayBuffers for HD Keys. Full Send.

The stream will be live at https://twitch.tv/dashincubator and stored at https://youtube.com/@dashincubator.

Changes I'm making include:

  • use Prettier, which adheres to JavaScript best practices
    (rather than feross' personal preferences - though genius to call it "standard style")
  • removing use of prototype, this, defineProperty
    • privateKey => getPrivateKey(), setPrivateKey()
    • privateExtendedKey => getPrivateExtendedKey()
    • publicExtendedKey => getPublicExtendedKey()
    • cleanup redundant properties (_identifier, identifier, pubKeyHash)
  • and adding types - as in jsdoc + tsc (https://jsjwithtypes.com), NOT ts
  • update tests
  • remove dependency on node assert
  • update var to let
    • we need to switch forEach to for due to async WebCrypto
  • Replacing legacy node Buffer with current Node LTS Uint8Array, ArrayBuffer, DataView
  • update ripemd160 to work in browsers (Uint8Array)
    (fork is @dashincubator/ripemd160)
  • update bs58check to work in browsers (Uint8Array)
    (rewrite is @dashincubator/base58check)

Node v18-only changes

  • Replacing the legacy node crypto with current Node LTS WebCrypto
  • secp256k1 => @noble/secp256k1
  • works in Node and Browsers no transpiling
  • works in modern Bundlers (Vite, WebPack), but should work in all that follow the specs

These defineProperty / prototype changes are necessary because they don't work with async functions, which are required by the current-gen APIs.

I have the prerequisite PR up at dashhive/DashHD.js#4.

I'm considering wether to try to get the old mocha tests going or to migrate them to modern table-driven tests.

I will submit the PRs here too, but we'll be maintaining our own fork either way.

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 a pull request may close this issue.

3 participants