From ae1a759aa9fa4b31ff2684b353e245b98dc4c212 Mon Sep 17 00:00:00 2001
From: Mark Stacey <markjstacey@gmail.com>
Date: Wed, 12 Feb 2025 15:59:33 -0330
Subject: [PATCH] chore: Improve error handling related to keyring metadata

This is a suggestion on how to improve error handling for keyring metadata in
the PR #5112.
---
 .../src/KeyringController.ts                  | 66 +++++++++++++++----
 1 file changed, 54 insertions(+), 12 deletions(-)

diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts
index 576c957608..cbf6934245 100644
--- a/packages/keyring-controller/src/KeyringController.ts
+++ b/packages/keyring-controller/src/KeyringController.ts
@@ -2297,18 +2297,48 @@ export class KeyringController extends BaseController<
    * using the given `opts`. The keyring is built using the keyring builder
    * registered for the given `type`.
    *
+   * The internal keyring and keyring metadata arrays are updated with the new
+   * keyring as well.
+   *
    * @param type - The type of keyring to add.
-   * @param data - The data to restore a previously serialized keyring.
+   * @param data - Keyring initialization options.
    * @returns The new keyring.
    * @throws If the keyring includes duplicated accounts.
    */
   async #newKeyring(type: string, data?: unknown): Promise<EthKeyring<Json>> {
-    this.#assertControllerMutexIsLocked();
+    const keyring = await this.#createKeyring(type, data);
 
-    const newKeyringMetadata: KeyringMetadata = {
-      id: ulid(),
-      name: '',
-    };
+    this.#keyrings.push(keyring);
+    this.#keyringsMetadata.push(getDefaultKeyringMetadata());
+    if (this.#keyrings.length !== this.#keyringsMetadata.length) {
+      throw new Error('Keyring metadata missing');
+    }
+
+    return keyring;
+  }
+
+  /**
+   * Instantiate, initialize and return a keyring of the given `type` using the
+   * given `opts`. The keyring is built using the keyring builder registered
+   * for the given `type`.
+   *
+   * The keyring might be new, or it might be restored from the vault. This
+   * function should only be called from `#newKeyring` or `#restoreKeyring`,
+   * for the "new" and "restore" cases respectively.
+   *
+   * The internal keyring and keyring metadata arrays are *not* updated, the
+   * caller is expected to update them.
+   *
+   * @param type - The type of keyring to add.
+   * @param data - Keyring initialization options.
+   * @returns The new keyring.
+   * @throws If the keyring includes duplicated accounts.
+   */
+  async #createKeyring(
+    type: string,
+    data?: unknown,
+  ): Promise<EthKeyring<Json>> {
+    this.#assertControllerMutexIsLocked();
 
     const keyringBuilder = this.#getKeyringBuilderForType(type);
 
@@ -2348,11 +2378,6 @@ export class KeyringController extends BaseController<
       this.#subscribeToQRKeyringEvents(keyring as unknown as QRKeyring);
     }
 
-    this.#keyrings.push(keyring);
-    if (this.#keyringsMetadata.length < this.#keyrings.length) {
-      this.#keyringsMetadata.push(newKeyringMetadata);
-    }
-
     return keyring;
   }
 
@@ -2382,7 +2407,15 @@ export class KeyringController extends BaseController<
 
     try {
       const { type, data } = serialized;
-      return await this.#newKeyring(type, data);
+      const keyring = await this.#createKeyring(type, data);
+      this.#keyrings.push(keyring);
+      // If metadata is missing, assume the data is from an installation before
+      // we had keyring metadata.
+      if (this.#keyringsMetadata.length < this.#keyrings.length) {
+        console.log(`Adding missing metadata for '${type}' keyring`);
+        this.#keyringsMetadata.push(getDefaultKeyringMetadata());
+      }
+      return keyring;
     } catch (_) {
       this.#unsupportedKeyrings.push(serialized);
       return undefined;
@@ -2616,4 +2649,13 @@ async function withLock<Result>(
   }
 }
 
+/**
+ * Generate a new keyring metadata object.
+ *
+ * @returns Keyring metadata.
+ */
+function getDefaultKeyringMetadata(): KeyringMetadata {
+  return { id: ulid(), name: '' };
+}
+
 export default KeyringController;