Add support for BIP32 (Heirarchical Deterministic Wallets) #88
Conversation
|
@kclowes will you give this a first pass? |
There was a problem hiding this comment.
@fubuloubu Thanks for breaking up these PRs, it's really helpful to not think about mnemonics in this one :) On a high level this is looking good to me, I don't see anything major to change. I think it would be good to see some more testing around the error cases in the derive_child_key method, and any other corner cases you can think of. I only made it through the CKD section in the BIP32 spec/this code, so I'll pick up at the HD Path/key tree part of the specification on Wednesday!
|
@kclowes Made updates per your suggestions! |
There was a problem hiding this comment.
This looks good to me, @fubuloubu! I left a few comments, but nothing major sticks out I see.
Just to make sure I understand, it looks to me that this PR returns a private (right?) child key given a path and a seed. Is that correct/am I missing anything?
@pipermerriam do you want to take a look?
|
@kclowes that's correct. The seed gets generated using BIP39 |
|
Any updates on merging this PR? |
|
Lemme give it a pass and I'll leave the final merge up to @kclowes |
|
@fubuloubu I'll merge this shortly! I started going through BIP39 today and should have a review for that PR soon. As general feedback on the HD wallet feature: I spoke with @pipermerriam and before we release it, we'll want to put it behind a flag that makes this API inaccessible by default because it hasn't been audited. Here is an example of putting EthPM behind a similar sort of flag in web3: ethereum/web3.py#1216. I think we can get away with only adding the flag in eth-account, but we may also want to add it to web3. And when you're adding documentation, we'll also need to add a warning there so that users know to use at their own risk. |
|
Yep, that sounds good to me! |
* remove testall because it doesnt work
Split off commits related to BIP32 implemented from #87