search

bitcoinjs / bip38

BIP38 is a standard process to encrypt Bitcoin and crypto currency private keys that is less susceptible to brute force attacks thus protecting the user.
http://cryptocoinjs.com/modules/currency/bip38/
MIT License
208 stars 100 forks source link

Use AES/createHash directly #8

Closed dcousens closed 9 years ago

dcousens commented 9 years ago

See https://github.com/crypto-browserify/browserify-aes/issues/19 for more information.

Essentially this change will dramatically reduce the number of dependencies this package will pull in, making browserify builds much smaller.

jprichardson commented 9 years ago

I understand the createHash part, but not AES? scryptsy uses the aes package directly, this was before crypto-browserify supported aes.

dcousens commented 9 years ago

scryptsy uses the aes package directly, this was before crypto-browserify supported aes.

What do you mean? scryptsy doesn't use AES at all?

this was before crypto-browserify supported aes.

This package uses crypto now and its AES implementation (aka, node w/ browserify-aes fallback).
There is no difference except that we "drop" the rest of the unused code by using browserify-aes directly (which includes the native fallback).

jprichardson commented 9 years ago

What do you mean? scryptsy doesn't use AES at all?

Right. I meant bip38.

Let's start with this...

Why drop crypto at all in place of createHash? I mean, I understand it's akin to the using the inherits package over require('util').inherits... but is it worth it in this case?

Pros:

  1. Probable decreased payload size. But by how much? I'd like this quantified (especially with using tools like Google Closure)

Cons:

  1. Another dependency. I do prefer the simplicity of just requiring crypto. [The irony doesn't escape me given the nature and mission of cryptocoinjs =) ] Browser side, I like browserify just handling the dependencies.
  2. Node.js side, it's not as clear to the reader of the code that it's actually using Node.js crypto. (Maybe grasping at straws here).

I'm not opposed to this, just don't want to make a rash decision.

dcousens commented 9 years ago

Probable decreased payload size. But by how much? I'd like this quantified (especially with using tools like Google Closure)

In Bitcoinjs-lib, we saw a 330kb decrease in size. I'm looking to save a similar amount once this package is updated in my applications. I don't personally use a tool like Google Closure, but even if I did, I see no reason why we can't have both.

Another dependency. I do prefer the simplicity of just requiring crypto. [The irony doesn't escape me given the nature and mission of cryptocoinjs =) ] Browser side, I like browserify just handling the dependencies.

Simple in that we know about crypto already, but crypto has many different facets in its API, and I don't know which version of crypto I'm using, is it node 0.12? Node 0.10? Iojs? All these versions, all have different subleties in their API. I want semver for my crypto, and using npm is one way to do that very easily.

Node.js side, it's not as clear to the reader of the code that it's actually using Node.js crypto. (Maybe grasping at straws here).

To the reader of the package, its impossible to know a lot of the assumptions, but at least with a package version and a package to go look up, I can see for myself and have some safety around what will happen. With require("crypto"), I just have to hope we're using the same standard library.

jprichardson commented 9 years ago

I don't personally use a tool like Google Closure, but even if I did, I see no reason why we can't have both.

It's not that we can't have both, I'm just trying to explore whether it's worth it or not.

I want semver for my crypto, and using npm is one way to do that very easily.

createHash isn't semver though for the Node.js crypto.createHash.

With require("crypto"), I just have to hope we're using the same standard library.

But it's not just require('crypto'), it's `require('crypto').createHash'.

I have actually put thought into this, while I'm leaning more towards the idea, I'm still not 100% just yet.

dcousens commented 9 years ago

createHash isn't semver though for the Node.js crypto.createHash.

True. That is a bogus benefit. I guess ideally though we could enforce that compatibility better at the one point though, because a change in the API would fry the specific tests for that in createHash, and we could choose to either upgrade or enforce a set API. Where as, it could have unintended side effects if its deep in your stack.

But you're right, that isn't entirely true.

I still think this is a worthwhile change, and as I mentioned, being able to drop all of "crypto" in my stack will save me a tonne of unnecessary code in my bundles.

dcousens commented 9 years ago

Closing, was implemented in https://github.com/cryptocoinjs/bip38/commit/eaf15b7d0df16bdaa06878c2264e5b4b0b063dd0