From 836b5a44aecf88600f38fc2461e21678d58492a3 Mon Sep 17 00:00:00 2001
From: Mark Stacey <markjstacey@gmail.com>
Date: Mon, 17 Jul 2023 21:50:03 -0230
Subject: [PATCH] Restore compatibility with QR Keyring

The `KeyringController` method `addNewKeyring` was changed in v11 to
initialize the keyring options to an empty object. These options are
passed to the `deserialize` method of the keyring. This broke
compatibility with the QR keyring because its `deserialize` method does
not expect to be passed an empty object.

The `KeyringController` has been updated to no longer initialize the
`addNewKeyring` options to an empty object. It should now work the same
as it did prior to v11. The one functional difference is that it now
throws an error when `addNewKeyring` is used to add a simple keyring
without supplying any private keys, however that path would have
already thrown an error.
---
 jest.config.js                |  6 +++---
 src/KeyringController.test.ts | 29 +++++++++++++++++++++++++++++
 src/KeyringController.ts      |  9 ++++++---
 3 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/jest.config.js b/jest.config.js
index 7849338a..ab6571a5 100644
--- a/jest.config.js
+++ b/jest.config.js
@@ -41,10 +41,10 @@ module.exports = {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 72.09,
+      branches: 72.41,
       functions: 92.85,
-      lines: 90.81,
-      statements: 91.02,
+      lines: 90.87,
+      statements: 91.08,
     },
   },
   preset: 'ts-jest',
diff --git a/src/KeyringController.test.ts b/src/KeyringController.test.ts
index fdf14cd9..cd9a736f 100644
--- a/src/KeyringController.test.ts
+++ b/src/KeyringController.test.ts
@@ -473,6 +473,15 @@ describe('KeyringController', () => {
       expect(allAccounts).toStrictEqual(expectedAllAccounts);
     });
 
+    it('should throw an error when attempting to add simple key pair without private keys', async () => {
+      const keyringController = await initializeKeyringController({
+        password: PASSWORD,
+      });
+      await expect(async () =>
+        keyringController.addNewKeyring(KeyringType.Simple),
+      ).rejects.toThrow('Private keys missing');
+    });
+
     it('should add HD Key Tree without mnemonic passed as an argument', async () => {
       const keyringController = await initializeKeyringController({
         password: PASSWORD,
@@ -504,6 +513,26 @@ describe('KeyringController', () => {
       expect(allAccounts).toHaveLength(3);
     });
 
+    it('should add keyring that expects undefined serialized state', async () => {
+      let deserializedSpy = sinon.spy();
+      const mockKeyringBuilder = () => {
+        const keyring = new KeyringMockWithInit();
+        deserializedSpy = sinon.spy(keyring, 'deserialize');
+        return keyring;
+      };
+      mockKeyringBuilder.type = 'Mock Keyring';
+      const keyringController = await initializeKeyringController({
+        constructorOptions: {
+          keyringBuilders: [mockKeyringBuilder],
+        },
+        password: PASSWORD,
+      });
+      await keyringController.addNewKeyring('Mock Keyring');
+
+      expect(deserializedSpy.callCount).toBe(1);
+      expect(deserializedSpy.calledWith(undefined)).toBe(true);
+    });
+
     it('should call init method if available', async () => {
       const keyringController = await initializeKeyringController({
         password: PASSWORD,
diff --git a/src/KeyringController.ts b/src/KeyringController.ts
index 0c938e83..8aca0426 100644
--- a/src/KeyringController.ts
+++ b/src/KeyringController.ts
@@ -4,7 +4,7 @@ import HDKeyring from '@metamask/eth-hd-keyring';
 import { normalize as normalizeToHex } from '@metamask/eth-sig-util';
 import SimpleKeyring from '@metamask/eth-simple-keyring';
 import { ObservableStore } from '@metamask/obs-store';
-import { remove0x, isValidHexAddress } from '@metamask/utils';
+import { remove0x, isValidHexAddress, isObject } from '@metamask/utils';
 import type {
   Hex,
   Json,
@@ -586,11 +586,14 @@ class KeyringController extends EventEmitter {
    */
   async addNewKeyring(
     type: string,
-    opts: Record<string, unknown> = {},
+    opts?: Record<string, unknown>,
   ): Promise<Keyring<Json>> {
     let keyring: Keyring<Json>;
     switch (type) {
       case KeyringType.Simple:
+        if (!isObject(opts)) {
+          throw new Error('Private keys missing');
+        }
         keyring = await this.#newKeyring(type, opts.privateKeys);
         break;
       default:
@@ -602,7 +605,7 @@ class KeyringController extends EventEmitter {
       throw new Error(KeyringControllerError.NoKeyring);
     }
 
-    if (!opts.mnemonic && type === KeyringType.HD) {
+    if (type === KeyringType.HD && (!isObject(opts) || !opts.mnemonic)) {
       if (!keyring.generateRandomMnemonic) {
         throw new Error(
           KeyringControllerError.UnsupportedGenerateRandomMnemonic,