bitcoinjs / bip39

JavaScript implementation of Bitcoin BIP39: Mnemonic code for generating deterministic keys
ISC License
1.11k stars 447 forks source link

nodejs dependence undocumented #45

Closed mitra42 closed 7 years ago

mitra42 commented 7 years ago

It appears that this code is nodejs dependent - using "require" as far as I'm aware is specific to Nodejs.

If nodejs dependence is intended then maybe documenting that in README.md would be good. (I'd do a pull request, but I'm not sure if that dependence is intended/fixable).

It might be good also to point at non-nodejs versions. I see https://github.com/ggozad/mnemonic.js/blob/master/mnemonic.js on GIT but there might be others.

dcousens commented 7 years ago

http://browserify.org/ - I use this module in several applications on the web, all https://github.com/bitcoinjs modules are designed for use when browserified.

mitra42 commented 7 years ago

Thanks - but it would be good to comment on the README since this repository comes up if you google for mnemonic without any reference to somewhere that says it needs "browserify". Note that browserify is itself dependent on npm which many people (myself included) dont run under OSX because there has been a tendency for npm to break things.

Would it make sense to browserify the modules via a Make or something like that?

dcousens commented 7 years ago

I suppose, it almost seems foreign to use JS on the web without browserify somewhere

prayerslayer commented 7 years ago

Hi, I also spend some time today figuring it out. Please document that you expect your users to do the browser-targeting transpile step themselves, especially because "all bitcoinjs modules are designed for use when browserified" is not obvious from the outside :)

(Even better would be if you already published sources that work in the browser.)

mitra42 commented 7 years ago

Understood - that some people are in the NodeJS environment. From outside that environment, doing the browserify step is non obvious. Browserify and the dependencies also turns a 4k piece of JS that could be quickly loaded into 340k (+word files), if every component needed for the project added similar amounts it becomes a non-starter. I'm guessing that far from all those dependencies are actually needed.

The code looks pretty simple and clearly written but I haven't had time to did into it yet and see if it can be turned into something pure-javascript, nor have I been able to find a pure-javascript version of the bip39.

prayerslayer commented 7 years ago

nor have I been able to find a pure-javascript version of the bip39

Me neither... Not promising anything here, but would you be interested in merging a PR that makes this library "universal" in the sense that it runs in modern Node.js and browser environments?

Edit: Though I cannot guarantee that the API would remain the same. E.g. creating a sha256 with window.crypto is asynchronous.

dcousens commented 7 years ago

if every component needed for the project added similar amounts it becomes a non-starter

Thankfully, that isn't the case. The largest dependencies we have are Buffer, create-hash and pbkdf2 - which all use Buffer internally (for good reason - performance!).

window.crypto is shimmed where needed within those modules, so if its available, it will be used.

If you're worried about size, I'm not against a change (pull request accepted) to enforce explicit parameterization of the word lists. That is, you'd need to do:

var bip39 = require('bip39')
var bip39ENG = require('bip39/english')

// ...

This would drop the "bundle" size by about 110kB.

prayerslayer commented 7 years ago

window.crypto is shimmed where needed within those modules, so if its available, it will be used.

You'd still ship the shim to your users unnecessarily, no?

dcousens commented 7 years ago

You'd still ship the shim to your users unnecessarily, no?

Unless you 100% know your users have window.crypto - then yes, that is how a shim works. If you can afford to do that, then you could easily drop that code manually out from the bundles by patching in your own modules (aka, just delete the shims).

dcousens commented 7 years ago

In dropping the implicit usage of hex strings and only supporting Buffer, the API is even closer to being "JS" only if you can replace the universal PBKDF2 with something you're happy with (window.crypto etc).

Buffer is a UInt8Array with extras, and those extras are used throughout PBKDF2, unless we drop that part of our stack, we can't remove Buffer use (nor do we want to). But, if you can replace PBKDF2... then you remove Buffer nearly.

You will need UTF8 normalization. You will need a UTF8 to UInt8 parser. You will need a non-Buffer SHA256 implementation. You will need a non-Buffer PBKDF2 implementation. You will want to ensure they don't have bugs. You will probably have a hex convertor in there, or 3.

You're bundle will probably become similar to a browserify build as its complexity increases... and you have to do all that extra work.

Closing this in favour of #48 as that will drop the word lists as a primary export, decreasing the default bundle size immensely.