bitpay / bitcore-mnemonic

BIP39 Mnemonics implemented for Bitcore
http://bitcore.io
MIT License
155 stars 212 forks source link

Added method Mnemonic.fromSeed which enables to create the object based ... #18

Closed mi-ro closed 9 years ago

mi-ro commented 9 years ago

...on an externally gathered entropy.

Basically I extended the constructor to additionally allow an entropy of type Buffer as the "data" argument and I added a method fromSeed which takes advantage of it. Of course I have updated the units tests too.

This change will allow to use the library with an externally created entropy.

This is my first pull request to an Open Source project - feedback is very welcome.

Thanks for your work!

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-2.44%) to 93.96% when pulling 1aa06da3f47ce00d3f5cfb2c4510e6e3191e0fa6 on mi-ro:mnemonic-from-seed into b2f0ce7cba44e7b3fbfa8664a928c73a93f56caf on bitpay:master.

maraoz commented 9 years ago

hey, thanks a lot for your work, and congrats on your first open-source contribution!! :)

code looks great, but unfortunately we cannot merge Pull Requests that decrease test coverage. Can you see which lines of your code are not tested? My guess is that you didn't test the preconditions you added in the fromSeed method. You can check test coverage locally by running: istanbul cover _mocha -- -R min (if you don't have it, you'll need to install istanbul by running sudo npm i -g istanbul).

Another suggestion: for preconditions, we usually use something like:

var $ = bitcore.util.preconditions;
...
Mnemonic.fromSeed = function(seed, wordlist) {
  $.checkArgument(Buffer.isBuffer(seed)), 'seed must be a Buffer.');
  $.checkArgument(_.isArray(wordlist) || _.isString(wordlist), 'wordlist must be a string or an array.');
  return new Mnemonic(seed, wordlist);
};

This style is not required, so feel free to keep your current version, but it looks more concise/clear IMO.

maraoz commented 9 years ago

btw, I didn't mention it, but since you're new to open-source: you can update your PR by pushing new commits to the same branch. You can then comment on this github PR to get us notified of your update

mi-ro commented 9 years ago

Thank you maraoz! I've pushed the new changes. Coverage should be up again.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-1.81%) to 94.59% when pulling 33c3b173f1ff3e2604f5cda852c69a50853c9346 on mi-ro:mnemonic-from-seed into b2f0ce7cba44e7b3fbfa8664a928c73a93f56caf on bitpay:master.

mi-ro commented 9 years ago

oh, coveralls seems not to agree with my local setup.

With istanbul I get the following values: master: Statements : 95.77% ( 136/142 ) Branches : 82.76% ( 48/58 ) Functions : 100% ( 12/12 ) Lines : 96.4% ( 134/139 )

mnemonic-from-seed: Statements : 96.03% ( 145/151 ) Branches : 84.38% ( 54/64 ) Functions : 100% ( 13/13 ) Lines : 96.62% ( 143/148 )

Should be okay, right?

maraoz commented 9 years ago

I ran the tests locally and got your results. Weird. I'll re-run travis and see if it reports correctly

maraoz commented 9 years ago

OK, found the source of the problem: https://github.com/bitpay/bitcore-mnemonic/blob/master/test/mnemonic.unit.js#L301

Some tests are not run on travis. I'll merge this, and fix the other problem in a separate PR. Congrats for your first open source contrib @mi-ro !!!

mi-ro commented 9 years ago

Awesome, thank you for guiding me! Should I delete this branch now?

maraoz commented 9 years ago

That's up to you, as it's part of your fork. My recommendation is to remove the branch though, given bitpay/master contains all the commits