Use rust-bip39 instead of tiny-bip39 #462
Conversation
dependency updated from tiny-bip39 to rust-bip39
|
Need to make a small fix in Mnemonic::parse_in(
Language::English, "jelly crash boy whisper mouse ecology tuna soccer memory million news short"
)?Then all tests should pass. |
|
LGTM! just need to fix the little CI issue. |
rajarshimaitra
force-pushed
the
bip39
branch
2 times, most recently
from
dc163d4 to
8f26d34
Compare
|
Ugh, sorry approved but then I noticed you should also be able to remove the |
src/keys/bip39.rs
Outdated
| type Seed = [u8; 64]; | ||
|
|
||
| /// Type describing entropy length (aka word count) in the mnemonic | ||
| pub enum WordCount { |
There was a problem hiding this comment.
Is there a reason we don't want to name this MnemonicType?
There was a problem hiding this comment.
I ask because it kind of used to exist earlier: https://docs.rs/bip39/0.5.1/bip39/enum.MnemonicType.html
There was a problem hiding this comment.
Right point. Reason is, WordCount seems more intuitive to me than MnemonicType. I had to look it up to see what it was doing.
MnemonicType is an enum used in tiny-bip39 which does more than simple word or entropy counting. It makes sense for the context of tiny-bip39 impls, but it seemed too much for rust-bip39 to use directly. It just takes an usize value as "word count".
So, I decided to make our own simple enum for it and it made sense to give it an easier name.
There was a problem hiding this comment.
PS: I think it should be WordsCount instead of WordCount.
There was a problem hiding this comment.
I agree with @artfuldev that MnemonicType is less of a code change for downstream users but if you want to change it to something more meaningful I like your original name WordCount better. If we're talking about how many words are in an article or book or something we generally say "the word count is 20" .. not "the words count is 20". See also: https://trends.google.com/trends/explore?q=%22words%20count%22,%22word%20count%22
There was a problem hiding this comment.
oops, it's already merged. WordsCount will be fine. 😉
There was a problem hiding this comment.
Ah, thats a nice google trick to sort this kinda disputes.. 😂
My bad, I will sneak the change back in some future PR.
There was a problem hiding this comment.
BTW that reminds me that this is probably a downstream API change. Even if we kept the name MnemonicType the bip39 API still changes. Maybe we should better document the API change somewhere?
There was a problem hiding this comment.
Since it does change the API we probably should mention it in the changelog. We can add that in another PR or when we make the next release branch we can add it then.
There was a problem hiding this comment.
Updated in #464
|
@notmandatory thanks, yes that should be removed. Updated.. |
Use rust-bip39 for mnemonic derivation everywhere. This requires our own WordCount enum as rust-bip39 doesn't have explicit mnemonic type definition.
|
Updated |
Description
Fixes #399. This can solve the current version conflict issues of
tiny-bip39dependencies and replace that withrust-bip39.Few small refactoring changes were required for that.
rust-bip39doesn't have dedicatedMnemonicTypeenum to denote mnemonic entropy (aka word count). To make the impl consistent with existing traits I have wrote a simpleWordCountenum to capture that.Notes to the reviewers
All previous tests are passing.
Checklists
All Submissions:
cargo fmtandcargo clippybefore committing