Fix: saving serialized keyring for which corresponding keyring class is not present (MetaMask#169)

Author: jpuri

index.js

@@ -48,6 +48,7 @@ class KeyringController extends EventEmitter {
 
     this.encryptor = opts.encryptor || encryptor;
     this.keyrings = [];
+    this._unsupportedKeyrings = [];
 
     // This option allows the controller to cache an exported key
     // for use in decrypting and encrypting data without password
@@ -562,6 +563,8 @@ class KeyringController extends EventEmitter {
       }),
     );
 
+    serializedKeyrings.push(...this._unsupportedKeyrings);
+
     let vault;
     let newEncryptionKey;
 
@@ -679,7 +682,9 @@ class KeyringController extends EventEmitter {
    */
   async restoreKeyring(serialized) {
     const keyring = await this._restoreKeyring(serialized);
-    await this._updateMemStoreKeyrings();
+    if (keyring) {
+      await this._updateMemStoreKeyrings();
+    }
     return keyring;
   }
 
@@ -690,12 +695,16 @@ class KeyringController extends EventEmitter {
    * On success, returns the resulting keyring instance.
    *
    * @param {Object} serialized - The serialized keyring.
-   * @returns {Promise<Keyring>} The deserialized keyring.
+   * @returns {Promise<Keyring|undefined>} The deserialized keyring or undefined if the keyring type is unsupported.
    */
   async _restoreKeyring(serialized) {
     const { type, data } = serialized;
 
     const Keyring = this.getKeyringClassForType(type);
+    if (!Keyring) {
+      this._unsupportedKeyrings.push(serialized);
+      return undefined;
+    }
     const keyring = new Keyring();
     await keyring.deserialize(data);
     // getAccounts also validates the accounts for some keyrings

test/index.js

@@ -94,6 +94,19 @@ describe('KeyringController', function () {
     });
   });
 
+  describe('persistAllKeyrings', function () {
+    it('should persist keyrings in _unsupportedKeyrings array', async function () {
+      const unsupportedKeyring = 'DUMMY_KEYRING';
+      keyringController._unsupportedKeyrings = [unsupportedKeyring];
+      await keyringController.persistAllKeyrings();
+
+      const { vault } = keyringController.store.getState();
+      const keyrings = await mockEncryptor.decrypt(password, vault);
+      expect(keyrings.indexOf(unsupportedKeyring) > -1).toBe(true);
+      expect(keyrings).toHaveLength(2);
+    });
+  });
+
   describe('createNewVaultAndKeychain', function () {
     it('should create a new vault', async function () {
       keyringController.store.updateState({ vault: null });
@@ -245,6 +258,13 @@ describe('KeyringController', function () {
       const accounts = await keyring.getAccounts();
       expect(accounts[0]).toBe(walletOneAddresses[0]);
     });
+    it('should return undefined if keyring type is not supported.', async function () {
+      const unsupportedKeyring = { type: 'Ledger Keyring', data: 'DUMMY' };
+      const keyring = await keyringController.restoreKeyring(
+        unsupportedKeyring,
+      );
+      expect(keyring).toBeUndefined();
+    });
   });
 
   describe('getAccounts', function () {
@@ -330,6 +350,16 @@ describe('KeyringController', function () {
         expect(keyring.wallets).toHaveLength(1);
       });
     });
+    it('add serialized keyring to _unsupportedKeyrings array if keyring type is not known', async function () {
+      const _unsupportedKeyrings = [{ type: 'Ledger Keyring', data: 'DUMMY' }];
+      mockEncryptor.encrypt(password, _unsupportedKeyrings);
+      await keyringController.setLocked();
+      const keyrings = await keyringController.unlockKeyrings(password);
+      expect(keyrings).toHaveLength(0);
+      expect(keyringController._unsupportedKeyrings).toStrictEqual(
+        _unsupportedKeyrings,
+      );
+    });
   });
 
   describe('verifyPassword', function () {