Add support for BIP39 (Mnemonic Seed Phrases) #89
Conversation
fubuloubu
force-pushed
the
bip39
branch
2 times, most recently
from
fe2b9e5
to
1e1118c
Compare
fubuloubu
force-pushed
the
bip39
branch
2 times, most recently
from
67fa449
to
5f9ab97
Compare
There was a problem hiding this comment.
@fubuloubu this is looking good to me. I left a few comments, the bulk of which point out that there are a bunch of methods that aren't getting used (I don't think), and I'd rather see them removed if we're not using them so they're not confusing for the next person. What do you think?
fubuloubu
force-pushed
the
bip39
branch
3 times, most recently
from
0bdafe6
to
a6e7c72
Compare
|
@kclowes @marcgarreau good to review! |
There was a problem hiding this comment.
Thanks for the updates @fubuloubu! I left a few comments around testing, but overall looks good.
@pipermerriam want to take a look?
|
I ended up pulling the fix for it here, see bad39f5 |
|
@kclowes @marcgarreau @pipermerriam ready for review! |
There was a problem hiding this comment.
Thanks for putting in the work in @fubuloubu. I'm still learning this domain; what I see looks good, but couple questions/suggestions:
There was a problem hiding this comment.
one thought I had was potentially exposing the various language APIs via something like.
from mnemonic.i10n.en import from_seed, validate_mnemonic, ...This would give us unambiguous access to the individual language specific functions.
Then, we could have a top level version of these functions which was designed to do language detection, or to require the caller to explicitly provide the language (or maybe both in some cases where we can do detection, but where there is a potential for ambiguity at which point we want to allow the user to provide an override.
|
|
||
| @combomethod | ||
| def list_languages(_Mnemonic): | ||
| return sorted(Path(f).stem for f in WORDLIST_DIR.rglob("*.txt")) |
There was a problem hiding this comment.
I'm fine with this pattern for now, but I want to suggest an alternative.
If we use a registry pattern then we can have a default registry which has the default wordlists. This allows for either extension via 3rd party wordlists, or even simpler testing via special wordlists... Also, it relies less on the filesystem as a database of the wordlists (though i think we would still store them as files, but register the parsed wordlists to eliminate the I/O component during runtime)
There was a problem hiding this comment.
Another option would be to instantiate the class with the wordlist itself and just have instances of the class for each supported language....
Or to have subclasses with the wordlist as a class property.
I think any of these would be an improvement over what is currently in place with the __init__ method doing I/O and using the filesystem as a sort of database of the available wordlists.
There was a problem hiding this comment.
I agree, we can make that happen. TBH I think a lot of this stuff doesn't even need to live in a class, the mnemonic words are only used for validation, not seed generation
There was a problem hiding this comment.
@fubuloubu were you going to update this now or in a separate PR?
There was a problem hiding this comment.
I'd like to do it as a separate update, if that's okay. I think what we have here is fine for now, we're unlikely to restructure how the lists work or to define alternative lists, and adding new lists from the official BIP39 registry is fairly easy. I added caching for now.
|
@pipermerriam I resolved everything except your series of comments on wordlist classes and coming up with a better structure. Feel free to re-arrange or restructure as you see fit. |
|
@pipermerriam #89 (comment) was resolved in 0f69fcb (I can't comment on it directly) |
|
Any updates with this? |
|
@fubuloubu I'll give it one more look-over tomorrow (Thursday) morning! |
There was a problem hiding this comment.
@fubuloubu I just added a couple last comments! Generally looks good! Thanks for adding all the testing!
|
|
||
| @combomethod | ||
| def list_languages(_Mnemonic): | ||
| return sorted(Path(f).stem for f in WORDLIST_DIR.rglob("*.txt")) |
There was a problem hiding this comment.
@fubuloubu were you going to update this now or in a separate PR?
Also moved several functions to _utils
|
@kclowes I merged it on down to 32 commits. There was a lot of suggestions in there, not sure how much more I should go down. |
|
LGTM! Thanks @fubuloubu! |
|
Would this also work with newer bitarray? |
Split off commits related to BIP32 implemented from #87
Based off of #88