metamask patch init

[adonesky1]

Metamask-extension currently uses a fork of bitcoinjs/bip39 (its used in eth-hd-keyring which is used by eth-keyring-controller which is used in the extension.

While seeking to remove extraneous wordlists from the extension bundle as part of the MV3 effort, I recalled that we had discussed switching our BIP39 library from bitcoinjs/bip39 to @scure/bip39.

This PR applies the required patches to this new MetaMask owned fork of @scure/bip39 to preserve the security requirement that we store and pass mnemonics in a format other than plain-text/string. As part of the transition to this implementation, mnemonics will be stored/passed as Uint8Arrays instead of Buffers. I have POC/tested this new interface in a string of PRs along the dependency tree back to the extension: this pr -> eth-hd-keyring -> eth-keyring-controller -> extension.

[paulmillr]

Seems fine. I am actually considering adding this feature upstream. Of course, without Buffers — huge aficionado of not using them because of security, lack of browser support, the fact uint8arrays are supported everywhere, etc.

[paulmillr]

Before, mnemonic was string. Currently, you're adding three new types: Uint8Array, Buffer, number[].

• Uint8Array represents array indexes — all good here, very efficient and effective
• Buffer, number[] represent string mapped over with new TextEncoder().encode(), basically converted to byte array. I would consider this as a bad API — why not just use Uint8Array and array indexes? Is your main concern a backwards compatibility? How about adding a wrapper function to metamask itself, that would convert from old Buffer mnemonics to new indexes and keeping the API lightweight?
• Buffer is often used as a substitution for Uint8Array — even more, Buffer instanceof Uint8Array is true. Which means, someone may think by mistake they can interchange them, while the logic is radically different
• number[] is another alternative for Buffer — not sure i'm seeing the point here — would rather just keep Buffer / Uint8Array. It would made sense to have this api to represent indexes - but not encoded strings.

[paulmillr]

Clarification: your current approach takes Buffer.from(str).length 73+ bytes to store some 12-word mnemonic; that's 147+ bytes for 24-word.

If you switch to the new way, which is coded in this pull request as a first if condition, it could instead consume just a 24-byte Uint8Array / 12-element number[] / 48-character hex string / 32-character base64 string

[adonesky1]

Is your main concern a backwards compatibility

Yes this was the primary concern. But I agree keeping the API lightweight makes sense.

Could you provide some guidance on the cleanest way in your view to make this conversion, and then I can apply on the consumer side.

Think I've got it ☝️ 👍

[adonesky1]

@paulmillr is there some trick to the test suite? I can't seem to get the err in assert.js to throw, so tests are passing no matter what I pass to deepStrictEqual?

[paulmillr]

Looking much better now!

Thinking about it, maybe we should just number[] a.k.a. array of indexes instead of Uint8Array? It would become:

entropy = getCoder(wordlist).decode(mnemonic.map((i) => wordlist[i]));

Having number[] is even better: it clearly says "I am waiting for array of numbers". If we keep Uint8Array, that would mean "we are waiting for some buffer, which is not really a buffer, but an array of numbers".

[adonesky1]

I think for now we will keep Uint8Array for uniformity and flexibility and consider migrating all to number[] at some point.

[paulmillr]

Understood, I will probably add the number[] to the main library and you won't need to maintain the fork when you'll decide to go number[] way.

[paulmillr]

Maybe keep the method as-is, and create a new one:

export function entropyToMnemonicIndexes(entropy: Uint8Array, wordlist: string[]): number[] {
  assertEntropy(entropy);
  const words = getCoder(wordlist).encode(entropy);
  return words.map((word) => wordlist.indexOf(word));
}

[adonesky1]

Sure we can do this, though we won't be using the old one with this fork...

[paulmillr]

Also "indexes" and "indices" both seem to be correct.

[adonesky1]

is there some trick to the test suite? I can't seem to get the err in assert.js to throw, so tests are passing no matter what I pass to deepStrictEqual?

Any ideas on this @paulmillr ?

[paulmillr]

Not sure what's the problem here. Assuming you're running built-in nodejs assertion library?

[adonesky1]

Assuming you're running built-in nodejs assertion library?

yes indeed

[adonesky1]

deepStrictEqual is async... so it always needs to be awaited... not sure why its not on main in the base repo? @paulmillr

[paulmillr]

Where? It's sync in docs

https://nodejs.org/api/assert.html

[adonesky1]

no I meant your implementation of deepStrictEqual is async

[paulmillr]

Sorry - not sure how i've missed async function. Removing async from it works just fine. I will do this now.

Still suggesting to change Uint8Array to number[] and passing 12-element array instead of 20+ element Uint8Array. What do you think?

update: saw your comment, all cool

[adonesky1]

@paulmillr thanks for all your help with this 🤝