integrate MM @scure/bip39 fork once released (#67)

* integrate @metamask/scure-bip39 * lower threshholds slightly * bump base node requiremnet to 14 * add node 18.x to test ci process * add test comparing new implementation with old to ensure account derivation is consistent * remove node version 18.x as a test flow from CI for now
MetaMask · Sep 27, 2022 · 29710a1 · 29710a1
1 parent 52391a2
commit 29710a1
Show file tree

Hide file tree

Showing 7 changed files with 1,032 additions and 1,192 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -7,6 +7,9 @@ module.exports = {
     {
       files: ['test/**/*.js'],
       extends: ['@metamask/eslint-config-jest'],
+      rules: {
+        'node/no-unpublished-require': 0,
+      },
     },
   ],
 

diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml
@@ -11,7 +11,7 @@ jobs:
     runs-on: ubuntu-20.04
     strategy:
       matrix:
-        node-version: [12.x, 14.x, 16.x]
+        node-version: [14.x, 16.x]
     steps:
       - uses: actions/checkout@v2
       - name: Use Node.js ${{ matrix.node-version }}

diff --git a/index.js b/index.js
@@ -1,6 +1,7 @@
 const { hdkey } = require('ethereumjs-wallet');
 const SimpleKeyring = require('eth-simple-keyring');
-const bip39 = require('@metamask/bip39');
+const bip39 = require('@metamask/scure-bip39');
+const { wordlist } = require('@metamask/scure-bip39/dist/wordlists/english');
 const { normalize } = require('@metamask/eth-sig-util');
 
 // Options:
@@ -16,17 +17,54 @@ class HdKeyring extends SimpleKeyring {
   }
 
   generateRandomMnemonic() {
-    this._initFromMnemonic(bip39.generateMnemonic());
+    this._initFromMnemonic(bip39.generateMnemonic(wordlist));
   }
 
-  serialize() {
-    const mnemonicAsBuffer =
-      typeof this.mnemonic === 'string'
-        ? Buffer.from(this.mnemonic, 'utf8')
-        : this.mnemonic;
+  uint8ArrayToString(mnemonic) {
+    const recoveredIndices = Array.from(
+      new Uint16Array(new Uint8Array(mnemonic).buffer),
+    );
+    return recoveredIndices.map((i) => wordlist[i]).join(' ');
+  }
+
+  stringToUint8Array(mnemonic) {
+    const indices = mnemonic.split(' ').map((word) => wordlist.indexOf(word));
+    return new Uint8Array(new Uint16Array(indices).buffer);
+  }
+
+  mnemonicToUint8Array(mnemonic) {
+    let mnemonicData = mnemonic;
+    // when encrypted/decrypted, buffers get cast into js object with a property type set to buffer
+    if (mnemonic && mnemonic.type && mnemonic.type === 'Buffer') {
+      mnemonicData = mnemonic.data;
+    }
 
+    if (
+      // this block is for backwards compatibility with vaults that were previously stored as buffers, number arrays or plain text strings
+      typeof mnemonicData === 'string' ||
+      Buffer.isBuffer(mnemonicData) ||
+      Array.isArray(mnemonicData)
+    ) {
+      let mnemonicAsString = mnemonicData;
+      if (Array.isArray(mnemonicData)) {
+        mnemonicAsString = Buffer.from(mnemonicData).toString();
+      } else if (Buffer.isBuffer(mnemonicData)) {
+        mnemonicAsString = mnemonicData.toString();
+      }
+      return this.stringToUint8Array(mnemonicAsString);
+    } else if (
+      mnemonicData instanceof Object &&
+      !(mnemonicData instanceof Uint8Array)
+    ) {
+      // when encrypted/decrypted the Uint8Array becomes a js object we need to cast back to a Uint8Array
+      return Uint8Array.from(Object.values(mnemonicData));
+    }
+    return mnemonicData;
+  }
+
+  serialize() {
     return Promise.resolve({
-      mnemonic: Array.from(mnemonicAsBuffer.values()),
+      mnemonic: this.mnemonicToUint8Array(this.mnemonic),
       numberOfAccounts: this.wallets.length,
       hdPath: this.hdPath,
     });
@@ -104,24 +142,19 @@ class HdKeyring extends SimpleKeyring {
         'Eth-Hd-Keyring: Secret recovery phrase already provided',
       );
     }
+
+    this.mnemonic = this.mnemonicToUint8Array(mnemonic);
+
     // validate before initializing
-    const isValid = bip39.validateMnemonic(mnemonic);
+    const isValid = bip39.validateMnemonic(this.mnemonic, wordlist);
     if (!isValid) {
       throw new Error(
         'Eth-Hd-Keyring: Invalid secret recovery phrase provided',
       );
     }
 
-    if (typeof mnemonic === 'string') {
-      this.mnemonic = Buffer.from(mnemonic, 'utf8');
-    } else if (Array.isArray(mnemonic)) {
-      this.mnemonic = Buffer.from(mnemonic);
-    } else {
-      this.mnemonic = mnemonic;
-    }
-
     // eslint-disable-next-line node/no-sync
-    const seed = bip39.mnemonicToSeedSync(this.mnemonic);
+    const seed = bip39.mnemonicToSeedSync(this.mnemonic, wordlist);
     this.hdWallet = hdkey.fromMasterSeed(seed);
     this.root = this.hdWallet.derivePath(this.hdPath);
   }

diff --git a/jest.config.js b/jest.config.js
@@ -5,8 +5,8 @@ module.exports = {
     global: {
       branches: 84,
       functions: 100,
-      lines: 96,
-      statements: 96,
+      lines: 95,
+      statements: 95,
     },
   },
   moduleFileExtensions: ['js', 'json', 'jsx', 'ts', 'tsx', 'node'],

diff --git a/package.json b/package.json
@@ -18,26 +18,28 @@
   "author": "Dan Finlay",
   "main": "index.js",
   "scripts": {
-    "setup": "yarn install && yarn allow-scripts",
-    "lint:eslint": "eslint . --cache --ext js,ts",
-    "lint:misc": "prettier '**/*.json' '**/*.md' '!CHANGELOG.md' '**/*.yml' --ignore-path .gitignore",
     "lint": "yarn lint:eslint && yarn lint:misc --check",
+    "lint:eslint": "eslint . --cache --ext js,ts",
     "lint:fix": "yarn lint:eslint --fix && yarn lint:misc --write",
+    "lint:misc": "prettier '**/*.json' '**/*.md' '!CHANGELOG.md' '**/*.yml' --ignore-path .gitignore",
+    "setup": "yarn install && yarn allow-scripts",
     "test": "jest"
   },
   "dependencies": {
-    "@metamask/bip39": "^4.0.0",
     "@metamask/eth-sig-util": "^4.0.0",
+    "@metamask/scure-bip39": "^2.0.3",
     "eth-simple-keyring": "^4.2.0",
-    "ethereumjs-util": "^7.0.9",
     "ethereumjs-wallet": "^1.0.1"
   },
   "devDependencies": {
+    "@ethereumjs/util": "^8.0.0-beta.3",
     "@lavamoat/allow-scripts": "^1.0.6",
     "@metamask/auto-changelog": "^2.5.0",
+    "@metamask/bip39": "^4.0.0",
     "@metamask/eslint-config": "^8.0.0",
     "@metamask/eslint-config-jest": "^9.0.0",
     "@metamask/eslint-config-nodejs": "^8.0.0",
+    "@metamask/eth-hd-keyring": "4.0.2",
     "@types/jest": "^27.4.1",
     "eslint": "^7.32.0",
     "eslint-config-prettier": "^8.3.0",
@@ -50,7 +52,7 @@
     "prettier-plugin-packagejson": "^2.2.12"
   },
   "engines": {
-    "node": ">= 12.0.0"
+    "node": ">= 14.0.0"
   },
   "publishConfig": {
     "access": "public",

diff --git a/test/index.js b/test/index.js
@@ -6,7 +6,10 @@ const {
   signTypedData,
   SignTypedDataVersion,
 } = require('@metamask/eth-sig-util');
-const ethUtil = require('ethereumjs-util');
+const { wordlist } = require('@metamask/scure-bip39/dist/wordlists/english');
+const oldMMForkBIP39 = require('@metamask/bip39');
+const { isValidAddress } = require('@ethereumjs/util');
+const OldHdKeyring = require('@metamask/eth-hd-keyring');
 const HdKeyring = require('..');
 
 // Sample account:
@@ -24,6 +27,28 @@ describe('hd-keyring', () => {
     keyring = new HdKeyring();
   });
 
+  describe('compare old bip39 implementation with new', () => {
+    it('should derive the same accounts from the same mnemonics', () => {
+      const mnemonics = [];
+      for (let i = 0; i < 99; i++) {
+        mnemonics.push(oldMMForkBIP39.generateMnemonic());
+      }
+
+      mnemonics.forEach(async (mnemonic) => {
+        const newHDKeyring = new HdKeyring({ mnemonic, numberOfAccounts: 3 });
+        const oldHDKeyring = new OldHdKeyring({
+          mnemonic,
+          numberOfAccounts: 3,
+        });
+        const newAccounts = await newHDKeyring.getAccounts();
+        const oldAccounts = await oldHDKeyring.getAccounts();
+        expect(newAccounts[0]).toStrictEqual(oldAccounts[0]);
+        expect(newAccounts[1]).toStrictEqual(oldAccounts[1]);
+        expect(newAccounts[2]).toStrictEqual(oldAccounts[2]);
+      });
+    });
+  });
+
   describe('constructor', () => {
     it('constructs with a typeof string mnemonic', async () => {
       keyring = new HdKeyring({
@@ -36,9 +61,9 @@ describe('hd-keyring', () => {
       expect(accounts[1]).toStrictEqual(secondAcct);
     });
 
-    it('constructs with a typeof array mnemonic', async () => {
+    it('constructs with a typeof buffer mnemonic', async () => {
       keyring = new HdKeyring({
-        mnemonic: Array.from(Buffer.from(sampleMnemonic, 'utf8').values()),
+        mnemonic: Buffer.from(sampleMnemonic, 'utf8'),
         numberOfAccounts: 2,
       });
 
@@ -47,9 +72,15 @@ describe('hd-keyring', () => {
       expect(accounts[1]).toStrictEqual(secondAcct);
     });
 
-    it('constructs with a typeof buffer mnemonic', async () => {
+    it('constructs with a typeof Uint8Array mnemonic', async () => {
+      const indices = sampleMnemonic
+        .split(' ')
+        .map((word) => wordlist.indexOf(word));
+      const uInt8ArrayOfMnemonic = new Uint8Array(
+        new Uint16Array(indices).buffer,
+      );
       keyring = new HdKeyring({
-        mnemonic: Buffer.from(sampleMnemonic, 'utf8'),
+        mnemonic: uInt8ArrayOfMnemonic,
         numberOfAccounts: 2,
       });
 
@@ -132,18 +163,34 @@ describe('hd-keyring', () => {
   });
 
   describe('#serialize mnemonic.', () => {
-    it('serializes mnemonic stored as a buffer in a class variable into a buffer array and does not add accounts', async () => {
-      keyring.generateRandomMnemonic();
+    it('serializes mnemonic stored as a buffer to a Uint8Array', async () => {
+      keyring.mnemonic = oldMMForkBIP39.generateMnemonic();
+      const mnemonicAsUint8Array = keyring.stringToUint8Array(
+        keyring.mnemonic.toString(),
+      );
       const output = await keyring.serialize();
       expect(output.numberOfAccounts).toBe(0);
-      expect(Array.isArray(output.mnemonic)).toBe(true);
+      expect(output.mnemonic).toStrictEqual(mnemonicAsUint8Array);
     });
 
-    it('serializes mnemonic stored as a string in a class variable into a buffer array and does not add accounts', async () => {
+    it('serializes keyring data with mnemonic stored as a Uint8Array', async () => {
+      keyring.generateRandomMnemonic();
+      const { mnemonic } = keyring;
+      const hdpath = keyring.hdPath;
+      keyring.addAccounts(1);
+      const output = await keyring.serialize();
+      expect(output.numberOfAccounts).toBe(1);
+      expect(output.hdPath).toStrictEqual(hdpath);
+      expect(output.mnemonic).toStrictEqual(mnemonic);
+    });
+
+    it('serializes mnemonic stored as a string', async () => {
       keyring.mnemonic = sampleMnemonic;
       const output = await keyring.serialize();
       expect(output.numberOfAccounts).toBe(0);
-      expect(Array.isArray(output.mnemonic)).toBe(true);
+      expect(output.mnemonic).toStrictEqual(
+        keyring.stringToUint8Array(sampleMnemonic),
+      );
     });
   });
 
@@ -160,7 +207,7 @@ describe('hd-keyring', () => {
       expect(accounts[1]).toStrictEqual(secondAcct);
       expect(accounts).toHaveLength(2);
       const serialized = await keyring.serialize();
-      expect(Buffer.from(serialized.mnemonic).toString()).toStrictEqual(
+      expect(keyring.uint8ArrayToString(serialized.mnemonic)).toStrictEqual(
         sampleMnemonic,
       );
     });
@@ -437,7 +484,7 @@ describe('hd-keyring', () => {
       );
 
       expect(address).not.toBe(appKeyAddress);
-      expect(ethUtil.isValidAddress(appKeyAddress)).toBe(true);
+      expect(isValidAddress(appKeyAddress)).toBe(true);
 
       const accounts = await keyring.getAccounts();
       expect(accounts[0]).toStrictEqual(firstAcct);
@@ -456,14 +503,14 @@ describe('hd-keyring', () => {
         'someapp.origin.io',
       );
 
-      expect(ethUtil.isValidAddress(appKeyAddress1)).toBe(true);
+      expect(isValidAddress(appKeyAddress1)).toBe(true);
 
       const appKeyAddress2 = await keyring.getAppKeyAddress(
         address,
         'anotherapp.origin.io',
       );
 
-      expect(ethUtil.isValidAddress(appKeyAddress2)).toBe(true);
+      expect(isValidAddress(appKeyAddress2)).toBe(true);
 
       expect(appKeyAddress1).not.toBe(appKeyAddress2);
     });
@@ -481,14 +528,14 @@ describe('hd-keyring', () => {
         'someapp.origin.io',
       );
 
-      expect(ethUtil.isValidAddress(appKeyAddress1)).toBe(true);
+      expect(isValidAddress(appKeyAddress1)).toBe(true);
 
       const appKeyAddress2 = await keyring.getAppKeyAddress(
         address,
         'someapp.origin.io',
       );
 
-      expect(ethUtil.isValidAddress(appKeyAddress2)).toBe(true);
+      expect(isValidAddress(appKeyAddress2)).toBe(true);
 
       expect(appKeyAddress1).toStrictEqual(appKeyAddress2);
     });