Skip to content
This repository has been archived by the owner on Jan 21, 2022. It is now read-only.

Use HD Wallet for account generation #44

Merged
merged 2 commits into from
Mar 25, 2016
Merged

Use HD Wallet for account generation #44

merged 2 commits into from
Mar 25, 2016

Conversation

tcoulter
Copy link
Contributor

FYI: When this is pushed to npm, this needs to be testrpc 2.0 as it includes breaking changes.

Include in this PR:

  • allow -m/--mnemonic to input an HD wallet mnemonic
  • print out private keys
  • print out mnemonic
  • -d option uses a default mnemonic instead of default seed. Seed is used to create mnemonic.
  • Finally, create accounts through the HD wallet rather than generating them randomly like we do now.
  • TestRPC.startServer() has been moved to ./bin/testrpc as its sole purpose was to support the cli version.

@tcoulter
Copy link
Contributor Author

Here's an example of the -m option being used:

Tims-MacBook-Pro:ethereumjs-testrpc tim$ ./bin/testrpc -m "awake book subject inch gentle blur grant damage process float month clown" -a 2
EthereumJS TestRPC v1.2.1

Available Accounts
==================
0x4dcccf58c6573eb896250b0c9647a40c1673af44
0xf4a950dac2cf3a8101e375550eeb79a53b74335c

Private Keys
==================
f62a8ea4ab7025d151ccd84981c66278d0d3cd58ff837467cdc51229915a22d1
623c65841d16c331e997f3b7855fb9a5fbad0bdc4e2fcd0928d4ddb9ba708125

HD Wallet
==================
Mnemonic:      awake book subject inch gentle blur grant damage process float month clown
Base HD Path:  m/44'/60'/0'/0/{account_index}

Listening on localhost:8545

@tcoulter
Copy link
Contributor Author

@axic Mind giving it a review?

@tcoulter tcoulter mentioned this pull request Mar 24, 2016
5 tasks
@tcoulter
Copy link
Contributor Author

I should add that -s and -d options still work just fine, creating a mnemonic from a seed value.

@tcoulter
Copy link
Contributor Author

Hmm, I probably don't need the rng code anymore as the seed could just be passed as the entropy. Let me check that out.

@tcoulter
Copy link
Contributor Author

Oops, I take that back. I do need that as entropy needs to be a hex value. Leaving it in.

@tcoulter
Copy link
Contributor Author

@kumavis @FlySwatter - can you guys test and see if these mnemonics work with your lightwallet inside metamask?

@danfinlay
Copy link
Contributor

Absolutely, will get back to you later this afternoon, this will hugely improve the developing experience with test rpc and metamask!

@tcoulter
Copy link
Contributor Author

@FlySwatter According to Christian the lightwallet uses a different hd path than the default BIP44 hd path, above. He eventually wants to move to the BIP44 path, but he says the path is currently configurable in the lightwallet. You should talk to him about how to configure the hd path so that the lightwallet and the testrpc are using the same one.

Also, thanks!

@danfinlay
Copy link
Contributor

You're using the bip44 path in test rpc?

  • Dan

On Mar 24, 2016, at 12:30 PM, Tim Coulter [email protected] wrote:

@FlySwatter According to Christian the lightwallet uses a different hd path than the default BIP44 hd path, above. He eventually wants to move to the BIP44 path, but he says the path is currently configurable in the lightwallet. You should talk to him about how to configure the hd path so that the lightwallet and the testrpc are using the same one.

Also, thanks!


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@tcoulter
Copy link
Contributor Author

@FlySwatter Ya, it's m/44'/60'/0'/0/{account_index} and is output when the new version of the testrpc starts.

@axic
Copy link
Contributor

axic commented Mar 24, 2016

Oops, I take that back. I do need that as entropy needs to be a hex value. Leaving it in.

Can you elaborate? Why do you still need that rng?

@axic
Copy link
Contributor

axic commented Mar 24, 2016

You're also missing the account index in front of the addresses/private keys.

@tcoulter
Copy link
Contributor Author

So someone can specify a seed and still get an associated mnemonic. Bip39's
entropyToMnemonic function only takes in a hex string. So, we use the rng
to create a deterministic hex string, then use that to create the mnemonic.
On Mar 24, 2016 2:02 PM, "Alex Beregszaszi" [email protected]
wrote:

Oops, I take that back. I do need that as entropy needs to be a hex value.
Leaving it in.

Can you elaborate? Why do you still need that rng?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#44 (comment)

@tcoulter
Copy link
Contributor Author

Do we need to print the indexes? Indexes are zero based and are displayed
in order.
On Mar 24, 2016 2:05 PM, "Alex Beregszaszi" [email protected]
wrote:

You're also missing the account index in front of the addresses/private
keys.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#44 (comment)

@axic
Copy link
Contributor

axic commented Mar 24, 2016

You could simplify that by running any hash over the input and using only 128 or 256 bits (that's what bip39 wants). And to further simplify it, you can make it slightly insecure by eliminating BIP39 entirely and just feeding hdkey.fromMasterSeed() with a 512 bit input, such as an sha512 of your input.

Re indexes. With 2 accounts it is simple, but with 10 accounts it is a bit more work to line them up. It is only a UI nitpick.

@axic
Copy link
Contributor

axic commented Mar 24, 2016

To be precise, the input to fromMasterSeed() must be between 128 and 512 bits according to BIP32.

@axic
Copy link
Contributor

axic commented Mar 24, 2016

Never mind the avoid bip39 part, at least with using it, even when seeding with something defined in the command line, the user will still get the valid mnemonic displayed.

@axic
Copy link
Contributor

axic commented Mar 24, 2016

As a side note, I think you should submit your random.randomBytes() feature to the seedrandom package. It might be useful for others too.

@kumavis
Copy link
Contributor

kumavis commented Mar 24, 2016

@tcoulter this is really useful! i approve
most important thing for me is not generating the keys randomly ever time - if i can specify a seed it will be the same 👍

@tcoulter
Copy link
Contributor Author

@kumavis FYI: -d is for you as I'm assuming you don't care what the seed is, just that it's deterministic.

@tcoulter
Copy link
Contributor Author

@axic So I'm futzing over which numbered list style looks the best (I kinda think the numbers look "meh"). That said, they're important to have. What do you think of this?

EthereumJS TestRPC v1.2.1

Available Accounts
==================
(0) 0xd370d1a13e3148ff03ffd27d47ddabe8b2079d28
(1) 0x8c175af1a002c2988870c1782e10ae49c30574ad
(2) 0x086c5d80bfb465d9bb0cad958c8d83ae88ba566e
(3) 0xc1dbb84e96993851ea4caca9c4cba71262155e0f
(4) 0x8e880094578c353408fb94b4eaccccbf4d44c287
(5) 0x05fc794b82410fb3fa575a144489ba3a3fac9c9d
(6) 0xa4ce9f8b8148a0c140af32c3e1f5fb315d92a010
(7) 0xff17109597237a5df4f228f84284283441d438f2
(8) 0x6a5b0ac9ab42b58731647cd2755fdc4efae3e00f
(9) 0x62a48c8292632a0848f23c9920952eb60dcb36a7

Private Keys
==================
(0) 5cb3a1f5aa95d9c51da911837e35f6e12b8ab6bc579c9aa72fae176f23ca015f
(1) 1c39331941326c3ab4fc983e37357452fcec461d0d3fb7a86d778174d4daa041
(2) 56a8ac9721fe247a1ffe37ed3746106c6f0c4881f4700ecfc72ed58def8e8dd8
(3) a517092aa887f8a301b6dfd3c5a6cfcc80af4617d27066e6ea8da8540551a533
(4) 8e3ac8a4c8ccdbb40bcc429e8ca18f4acc2da65375d464af7e931b4528d2169a
(5) c92f3cd4615294b256527f59c523acc25be5cb94d87c33f3b86b9f1883811d6b
(6) 5a68470e3eaca9ebfb5d389c44b49b3079b0a01d26a7f408a200778f967274e1
(7) 96f86daa3d14ea21209f5bb308d7d956e6e04e7a40155911eac5e172e73445f9
(8) 15d61f98173cc7644d5caa232fc6c1db2eb8181c1bbb1b6366ed77f7961d8fad
(9) 6776d55fd1d30de918747b29dad9124990601de855556cb6fe32c0af812ce622

HD Wallet
==================
Mnemonic:      charge pair visit idle bronze bundle someone fresh approve ankle cousin around
Base HD Path:  m/44'/60'/0'/0/{account_index}

Listening on localhost:8545

@tcoulter
Copy link
Contributor Author

Alright, executive decision time, I'm gonna go ahead and merge to master. This is going to sit on master for awhile while I fix a really-hard-to-debug race condition. After that 2.0.0 will be released. File a new issue if you guys run into any errors or have changes to propose.

Thanks guys!

@tcoulter tcoulter merged commit 4063e5d into master Mar 25, 2016
@tcoulter tcoulter deleted the hdwallet branch March 25, 2016 15:26
@axic
Copy link
Contributor

axic commented Mar 25, 2016

I think it looks great! Very clean. The {account_index} part could be better, but have not idea how to improve it.

A small nit: the HD Wallet section shouldn't be displayed in case --account was used.

@tcoulter
Copy link
Contributor Author

he HD Wallet section shouldn't be displayed in case --account was used.

Good point. I'll add an issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants