Remove bip39 dependency #179
Conversation
adonesky1
force-pushed
the
integrating-new-eth-hd-keyring/@metamask/scure-bip39
branch
2 times, most recently
from
b458c72 to
016f28c
Compare
adonesky1
force-pushed
the
integrating-new-eth-hd-keyring/@metamask/scure-bip39
branch
2 times, most recently
from
028f4b8 to
243c027
Compare
adonesky1
force-pushed
the
integrating-new-eth-hd-keyring/@metamask/scure-bip39
branch
from
243c027 to
55807bc
Compare
index.js
Outdated
| return new Uint8Array(new Uint16Array(indices).buffer); | ||
| } | ||
|
|
||
| _mnemonicToUint8Array(mnemonic) { |
There was a problem hiding this comment.
Suppose it could be useful to go back and make this a public static method on eth-hd-keyring to avoid this duplication. cc @Gudahtt
There was a problem hiding this comment.
Good point. It would be ideal to eliminate this duplication.
If we're using this code in both places, maybe it would be better to move this to a third place? e.g. https://github.com/MetaMask/utils or @metamask/scure-bip39. Just a thought; eth-hd-keyring would suffice for now.
There was a problem hiding this comment.
yeah maybe @metamask/scure-bip39 may make the most sense?
There was a problem hiding this comment.
Or we could drop this validation step. It seems to be redundant; eth-hd-keyring is doing it already here: https://github.com/MetaMask/eth-hd-keyring/blob/main/index.js#L284
There was a problem hiding this comment.
lol true. In which case KeyringController needn't depend directly on @metamask/scure-bip39 at all!
There was a problem hiding this comment.
Removed bip39 dependency altogether in latest commit: dea4b70
index.js
Outdated
| ) | ||
| ) { | ||
| throw new Error('Seed phrase is invalid.'); | ||
| if (!bip39.validateMnemonic(encodedSeedPhrase, wordlist)) { |
There was a problem hiding this comment.
We no longer validate against every wordlist.
adonesky1
force-pushed
the
integrating-new-eth-hd-keyring/@metamask/scure-bip39
branch
from
858c7e0 to
4eb0812
Compare
index.js
Outdated
| ) | ||
| ) { | ||
| throw new Error('Seed phrase is invalid.'); | ||
| if (!bip39.validateMnemonic(seedPhraseUint8Array, wordlist)) { |
There was a problem hiding this comment.
we only validate against the english wordlist now.
There was a problem hiding this comment.
Makes sense.
For the changelog, is this a breaking change? i.e. did eth-hd-keyring ever support non-english mnemonics?
There was a problem hiding this comment.
No it did not. Back when we were using bitcoinjs/bip39 and our fork we would validate against the default wordlist which was [set to english by default].(https://github.com/bitcoinjs/bip39/blob/f88d0dd4db96b3ccbba1c8bd6ec87bff54025949/src/_wordlists.js#L57).
| }, | ||
| ); | ||
| const [firstAccount] = await firstKeyring.getAccounts(); | ||
| await this.clearKeyrings(); |
There was a problem hiding this comment.
Interesting, another async call without an await. In this case the function wasn't doing anything async, so this should result in no functional change.
adonesky1
changed the title
adonesky1
deleted the
integrating-new-eth-hd-keyring/@metamask/scure-bip39
branch
Remove
bip39dependency. The validation removed here is now taken care of in@metamask/eth-hd-keyring: https://github.com/MetaMask/eth-hd-keyring/blob/main/index.js#L284