metamask patch init by adonesky1 · Pull Request #1 · MetaMask/scure-bip39

adonesky1 · 2022-08-23T21:01:08Z

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 · 2022-08-24T03:17:37Z

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.

src/index.ts

paulmillr · 2022-08-24T03:36:45Z

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 · 2022-08-24T03:44:11Z

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 · 2022-08-24T14:24:12Z

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 ☝️ 👍

…monic

adonesky1 · 2022-08-25T17:15:32Z

@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 · 2022-08-26T03:56:46Z

src/index.ts

+      Array.from(new Uint16Array(mnemonic.buffer)).map((i) => wordlist[i])
+    );
+  }
  assertEntropy(entropy);


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".

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

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.

src/index.ts

paulmillr · 2022-08-26T04:00:35Z

src/index.ts

 * // 'legal winner thank year wave sausage worth useful legal winner thank yellow'
 */
-export function entropyToMnemonic(entropy: Uint8Array, wordlist: string[]): string {
+export function entropyToMnemonic(entropy: Uint8Array, wordlist: string[]): Uint8Array {


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)); }

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

paulmillr · 2022-08-26T04:01:17Z

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

adonesky1 · 2022-09-06T14:02:57Z

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 · 2022-09-07T13:23:09Z

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

adonesky1 · 2022-09-07T15:41:40Z

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

yes indeed

adonesky1 · 2022-09-07T16:11:14Z

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

paulmillr · 2022-09-07T17:02:00Z

Where? It's sync in docs

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

adonesky1 · 2022-09-07T17:24:16Z

no I meant your implementation of deepStrictEqual is async

paulmillr · 2022-09-07T20:17:35Z

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 · 2022-09-07T21:07:40Z

@paulmillr thanks for all your help with this 🤝

metamask patch init

f378402

adonesky1 force-pushed the metamask-patch-cleanup branch from 8ce8f62 to f378402 Compare August 23, 2022 21:12

enable mnemonicToEntropy for number[]

2f53e0f

adonesky1 mentioned this pull request Aug 23, 2022

Migrate to one bip39 package. We use multiple right now, ethers and bip39, both of which have their own wordlists MetaMask/metamask-extension#14929

Open

paulmillr reviewed Aug 24, 2022

View reviewed changes

src/index.ts Show resolved Hide resolved

remove number[] as an allowed typed for mnemonicToEntropy/validateMne…

8cd4126

…monic

This was referenced Aug 24, 2022

integrating new eth-hd-keyring version with @scure/bip39 MetaMask/KeyringController#149

Closed

WIP adapting to new BIP39 package MetaMask/metamask-extension#15699

Closed

adonesky1 added 2 commits August 25, 2022 12:16

remove Buffer as an expected type from functions we use

8c31ef5

update @types/node

881c5e2

adonesky1 requested review from Gudahtt, kumavis and paulmillr and removed request for paulmillr August 25, 2022 17:22

adonesky1 marked this pull request as ready for review August 25, 2022 18:00

paulmillr reviewed Aug 26, 2022

View reviewed changes

swap out buffers for uint8arrays in mnemonicToSeedSync

f8bd3f5

adonesky1 force-pushed the metamask-patch-cleanup branch from eef6c67 to f8bd3f5 Compare September 6, 2022 16:13

adonesky1 requested a review from paulmillr September 6, 2022 16:23

fixing tests

a45888b

adonesky1 force-pushed the metamask-patch-cleanup branch from 9d1a513 to a45888b Compare September 7, 2022 18:57

remove unnecessary Buffer

6d4186a

paulmillr approved these changes Sep 7, 2022

View reviewed changes

kumavis approved these changes Sep 7, 2022

View reviewed changes

adonesky1 merged commit 47f348a into main Sep 7, 2022

kumavis deleted the metamask-patch-cleanup branch September 7, 2022 21:13

adonesky1 mentioned this pull request Sep 12, 2022

1.1.0 #4

Closed

adonesky1 mentioned this pull request Dec 22, 2022

Integrate KeyringController v10 MetaMask/metamask-extension#17056

Merged

Conversation

adonesky1 commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulmillr commented Aug 24, 2022

Uh oh!

Uh oh!

paulmillr commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulmillr commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adonesky1 commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adonesky1 commented Aug 25, 2022

Uh oh!

paulmillr Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

adonesky1 Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulmillr Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulmillr Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

adonesky1 Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

paulmillr commented Aug 26, 2022

Uh oh!

adonesky1 commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulmillr commented Sep 7, 2022

Uh oh!

adonesky1 commented Sep 7, 2022

Uh oh!

adonesky1 commented Sep 7, 2022

Uh oh!

paulmillr commented Sep 7, 2022

Uh oh!

adonesky1 commented Sep 7, 2022

Uh oh!

paulmillr commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adonesky1 commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adonesky1 commented Aug 23, 2022 •

edited

Loading

paulmillr commented Aug 24, 2022 •

edited

Loading

paulmillr commented Aug 24, 2022 •

edited

Loading

adonesky1 commented Aug 24, 2022 •

edited

Loading

adonesky1 Sep 7, 2022 •

edited

Loading

adonesky1 commented Sep 6, 2022 •

edited

Loading

paulmillr commented Sep 7, 2022 •

edited

Loading