fix: add HD keyring check to persistAllKeyrings (#4168)

MetaMask · Apr 18, 2024 · 8c7e1e0 · 8c7e1e0
1 parent 1536fce
commit 8c7e1e0
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 3 deletions.
diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts
@@ -1111,12 +1111,20 @@ describe('KeyringController', () => {
        * If there is only HD Key Tree keyring with 1 account and removeAccount is called passing that account
        * It deletes keyring object also from state - not sure if this is correct behavior.
        * https://github.com/MetaMask/core/issues/801
+       *
+       * Update: The behaviour is now modified to never remove the HD keyring as a preventive and temporal solution to the current race
+       * condition case we have been seeing lately. https://github.com/MetaMask/mobile-planning/issues/1507
+       * Enforcing this behaviour is not a 100% correct and it should be responsibility of the consumer to handle the accounts
+       * and keyrings in a way that it matches the expected behaviour.
        */
-      it('should remove HD Key Tree keyring from state when single account associated with it is deleted', async () => {
+      it('should not remove HD Key Tree keyring nor the single account from state', async () => {
         await withController(async ({ controller, initialState }) => {
           const account = initialState.keyrings[0].accounts[0] as Hex;
-          await controller.removeAccount(account);
-          expect(controller.state.keyrings).toHaveLength(0);
+          await expect(controller.removeAccount(account)).rejects.toThrow(
+            KeyringControllerError.NoHdKeyring,
+          );
+          expect(controller.state.keyrings).toHaveLength(1);
+          expect(controller.state.keyrings[0].accounts).toHaveLength(1);
         });
       });
 

diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts
@@ -953,6 +953,12 @@ export class KeyringController extends BaseController<
 
       serializedKeyrings.push(...this.#unsupportedKeyrings);
 
+      if (
+        !serializedKeyrings.some((keyring) => keyring.type === KeyringTypes.hd)
+      ) {
+        throw new Error(KeyringControllerError.NoHdKeyring);
+      }
+
       const updatedState: Partial<KeyringControllerState> = {};
 
       if (this.#cacheEncryptionKey) {

diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts
@@ -26,4 +26,5 @@ export enum KeyringControllerError {
   ExpiredCredentials = 'KeyringController - Encryption key and salt provided are expired',
   NoKeyringBuilder = 'KeyringController - No keyringBuilder found for keyring',
   DataType = 'KeyringController - Incorrect data type provided',
+  NoHdKeyring = 'KeyringController - No HD Keyring found',
 }