fix: serialized keyring comparison when updating the vault

packages/keyring-controller/CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- Fixed serialized keyring comparison when establishing whether a vault update is needed ([#5928](https://github.com/MetaMask/core/pull/5928))
+  - The vault update was being skipped when a keyring class returns an object shallow copy through `.serialize()`.
+
 ## [22.0.1]
 
 ### Changed

packages/keyring-controller/src/KeyringController.test.ts

@@ -41,7 +41,7 @@ import MockEncryptor, {
 } from '../tests/mocks/mockEncryptor';
 import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring';
 import { MockKeyring } from '../tests/mocks/mockKeyring';
-import MockShallowGetAccountsKeyring from '../tests/mocks/mockShallowGetAccountsKeyring';
+import MockShallowKeyring from '../tests/mocks/mockShallowKeyring';
 import { buildMockTransaction } from '../tests/mocks/mockTransaction';
 
 jest.mock('uuid', () => {
@@ -404,16 +404,14 @@ describe('KeyringController', () => {
       it('should not throw when `keyring.getAccounts()` returns a shallow copy', async () => {
         await withController(
           {
-            keyringBuilders: [
-              keyringBuilderFactory(MockShallowGetAccountsKeyring),
-            ],
+            keyringBuilders: [keyringBuilderFactory(MockShallowKeyring)],
           },
           async ({ controller }) => {
-            await controller.addNewKeyring(MockShallowGetAccountsKeyring.type);
+            await controller.addNewKeyring(MockShallowKeyring.type);
             // TODO: This is a temporary workaround while `addNewAccountForKeyring` is not
             // removed.
             const mockKeyring = controller.getKeyringsByType(
-              MockShallowGetAccountsKeyring.type,
+              MockShallowKeyring.type,
             )[0] as EthKeyring;
 
             jest
@@ -3534,6 +3532,28 @@ describe('KeyringController', () => {
           );
         });
 
+        it('should update the vault if the keyring is being updated but `keyring.serialize()` includes a shallow copy', async () => {
+          await withController(
+            { keyringBuilders: [keyringBuilderFactory(MockShallowKeyring)] },
+            async ({ controller, messenger }) => {
+              await controller.addNewKeyring(MockShallowKeyring.type);
+              const mockStateChange = jest.fn();
+              messenger.subscribe(
+                'KeyringController:stateChange',
+                mockStateChange,
+              );
+
+              await controller.withKeyring(
+                { type: MockShallowKeyring.type },
+                async ({ keyring }) => keyring.addAccounts(1),
+              );
+
+              expect(mockStateChange).toHaveBeenCalled();
+              expect(controller.state.keyrings[1].accounts).toHaveLength(1);
+            },
+          );
+        });
+
         it('should not update the vault if the keyring has not been updated', async () => {
           const mockAddress = '0x4584d2B4905087A100420AFfCe1b2d73fC69B8E4';
           stubKeyringClassWithAccount(MockKeyring, mockAddress);

packages/keyring-controller/src/KeyringController.ts

@@ -2705,9 +2705,9 @@ export class KeyringController extends BaseController<
     callback: MutuallyExclusiveCallback<Result>,
   ): Promise<Result> {
     return this.#withRollback(async ({ releaseLock }) => {
-      const oldState = await this.#getSessionState();
+      const oldState = JSON.stringify(await this.#getSessionState());
       const callbackResult = await callback({ releaseLock });
-      const newState = await this.#getSessionState();
+      const newState = JSON.stringify(await this.#getSessionState());
 
       // State is committed only if the operation is successful and need to trigger a vault update.
       if (!isEqual(oldState, newState)) {

packages/keyring-controller/tests/mocks/mockShallowGetAccountsKeyring.ts → packages/keyring-controller/tests/mocks/mockShallowKeyring.ts

@@ -3,17 +3,17 @@ import type { Json, Hex } from '@metamask/utils';
 
 /**
  * A test keyring that returns a shallow copy of the accounts array
- * when calling getAccounts().
+ * when calling `getAccounts()` and `serialize()`.
  *
  * This is used to test the `KeyringController`'s behavior when using this
  * keyring, to make sure that, for example, the keyring's
  * accounts array is not not used to determinate the added account after
  * an operation.
  */
-export default class MockShallowGetAccountsKeyring implements EthKeyring {
-  static type = 'Mock Shallow getAccounts Keyring';
+export default class MockShallowKeyring implements EthKeyring {
+  static type = 'Mock Shallow Keyring';
 
-  public type = MockShallowGetAccountsKeyring.type;
+  public type = MockShallowKeyring.type;
 
   public accounts: Hex[];
 
@@ -24,7 +24,10 @@ export default class MockShallowGetAccountsKeyring implements EthKeyring {
   }
 
   async serialize(): Promise<Json> {
-    return {};
+    return {
+      // Shallow copy
+      accounts: this.accounts,
+    };
   }
 
   async deserialize(state: { accounts: Hex[] }) {