Uh oh!
There was an error while loading. Please reload this page.
Remove bip39 dependency - #179
Conversation
b458c72 to
016f28cCompare028f4b8 to
243c027Compare243c027 to
55807bcCompare| 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
| ) | ||
| ) { | ||
| 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.
858c7e0 to
4eb0812Compare| ) | ||
| ) { | ||
| 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.
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