Consensys / eth-lightwallet

Lightweight JS Wallet for Node and the browser
MIT License
1.47k stars 503 forks source link

Make salt user configurable #111

Closed danfinlay closed 8 years ago

danfinlay commented 8 years ago

Fixes #109

Added an optional salt attribute to relevant methods:

coder5876 commented 8 years ago

Thinking further about this I think it might be good to introduce a new "constructor" which is asynchronous and will accept a password as parameter. This can then generate a random salt and place it in the keystore. We could then have a deriveKeyFromPassword function as a member function of the keystore that would automatically use this salt when deriving the key.

Thoughts @flyswatter?

danfinlay commented 8 years ago

That sounds reasonable to me, my biggest priorities are:

  1. Add salting
  2. Don't break existing usage

So that fits nicely. We could easily show both strategies in the README, the "recommended" and the "old" ways.

Maybe that new constructor could generate the seed phrase at the same time, or at least optionally generate it? I think that could greatly simplify creating a new vault, you'd really only need a password, and some optional entropy and an optional seed phrase.

While we're considering adding a new, independent API, I'd like to encourage adoption of the opts object pattern, so we don't have to worry about argument order in the future. Ex:

var opts = {
  password: 'abc123',
  seedPhrase: 'an optional seed phrase could go here',
  entropy: 'optional additional entropy for generating the seed',
  salt: 'salt could even still be optionally user provided',
}
LightWallet.createVault(opts, function(err, vault) {
  // You've got a new vault now.
})

@christianlundkvist