fix(account-tree-controller/backup-and-sync): do not send extra metadata properties (#8300)

## Explanation

Since the introduction of `lastSelected` metadata (which should not be
synced), backup & sync was not accepting the data coming back from
user-storage. Mostly because the payload was like this:
```txt
"{\"name\":{\"value\":\"Account 1\",\"lastUpdatedAt\":0},\"pinned\":{\"value\":false,\"lastUpdatedAt\":0},\"hidden\":{\"value\":false,\"lastUpdatedAt\":0},\"lastSelected\":0,\"groupIndex\":0}"
"{\"name\":{\"value\":\"Account 2\",\"lastUpdatedAt\":0},\"pinned\":{\"value\":false,\"lastUpdatedAt\":0},\"hidden\":{\"value\":false,\"lastUpdatedAt\":0},\"lastSelected\":0,\"groupIndex\":1}"
```

The extra `lastSelected` shouldn't have been part of that payload. This
was making the `superstruct` validation to fail when getting those data
back, thus, preventing account syncing to finally restore accounts.

We now `mask` the extra properties that are not part of backup & sync
schema.

## References

N/A

## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md)
- [ ] I've introduced [breaking
changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md)
in this PR and have prepared draft pull requests for clients and
consumer packages to resolve them

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Touches backup/sync serialization and error handling; incorrect
masking/fallback could cause some group metadata (e.g., name/pin/hide)
not to persist or to be dropped in edge cases.
> 
> **Overview**
> Fixes backup & sync group serialization to **drop extra/invalid
metadata fields** before writing to user storage by masking group
metadata against `UserStorageSyncedWalletGroupSchema` (preventing fields
like `lastSelected` from breaking later reads/validation), with a safe
fallback to minimal `{ groupIndex }` plus logging.
> 
> Standardizes unknown-error stringification via a new `toErrorMessage`
utility and applies it across backup/sync error paths, alongside
new/updated unit tests and a changelog entry documenting the
user-storage filtering fix.
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
cfa55f2. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Author: ccharly

packages/account-tree-controller/CHANGELOG.md

@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- Add `lastSelected` (timestamp) to account group tree node metadata ([#8261](https://github.com/MetaMask/core/pull/8261))
+- Add `lastSelected` (timestamp) to account group tree node metadata ([#8261](https://github.com/MetaMask/core/pull/8261)), ([#8300](https://github.com/MetaMask/core/pull/8300))
   - `group.metadata.lastSelected` is set to `Date.now()` whenever a group becomes the selected group, either via `setSelectedAccountGroup` or `AccountsController:selectedAccountChange`.
   - The value is persisted in `accountGroupsMetadata` and restored on `init`/`reinit`.
   - The value is not synchronize through backup and sync.
@@ -25,6 +25,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Filter out extra properties from groups before sending to user storage ([#8300](https://github.com/MetaMask/core/pull/8300))
+  - The `lastSelected` metadata should not be persisted, to we automatically remove it from the payload.
+  - This uses the user storage schema to only keep what needs to be persisted.
 - Fix `AccountTreeControllerMessenger` type so that the union type for events is not `any` ([#8240](https://github.com/MetaMask/core/pull/8240))
 
 ## [5.0.1]

packages/account-tree-controller/src/backup-and-sync/service/index.ts

@@ -32,6 +32,7 @@ import {
   restoreStateFromSnapshot,
   getLocalEntropyWallets,
   getLocalGroupsForEntropyWallet,
+  toErrorMessage,
 } from '../utils';
 import type { StateSnapshot } from '../utils';
 
@@ -345,8 +346,7 @@ export class BackupAndSyncService {
               );
             }
           } catch (error) {
-            const errorMessage =
-              error instanceof Error ? error.message : String(error);
+            const errorMessage = toErrorMessage(error);
             const errorString = `Legacy syncing failed for wallet ${wallet.id}: ${errorMessage}`;
 
             backupAndSyncLogger(errorString);
@@ -400,8 +400,7 @@ export class BackupAndSyncService {
               walletProfileId,
             );
           } catch (error) {
-            const errorMessage =
-              error instanceof Error ? error.message : String(error);
+            const errorMessage = toErrorMessage(error);
             const errorString = `Error during multichain account syncing for wallet ${wallet.id}: ${errorMessage}`;
 
             backupAndSyncLogger(errorString);

packages/account-tree-controller/src/backup-and-sync/syncing/group.ts

@@ -17,6 +17,7 @@ import {
   pushGroupToUserStorageBatch,
 } from '../user-storage/network-operations';
 import { getLocalGroupsForEntropyWallet } from '../utils';
+import { toErrorMessage } from '../utils/errors';
 
 /**
  * Creates multiple multichain account groups in batch (from 0 to maxGroupIndex).
@@ -90,7 +91,7 @@ export const createMultichainAccountGroupsBatch = async (
     backupAndSyncLogger(
       `Failed to create account groups batch:`,
       // istanbul ignore next
-      error instanceof Error ? error.message : String(error),
+      toErrorMessage(error),
     );
   }
 };

packages/account-tree-controller/src/backup-and-sync/user-storage/format-utils.test.ts

@@ -11,10 +11,16 @@ import {
   assertValidLegacyUserStorageAccount,
 } from './validation';
 import type { AccountGroupMultichainAccountObject } from '../../group';
+import { backupAndSyncLogger } from '../../logger';
 import type { AccountWalletEntropyObject } from '../../wallet';
 import type { BackupAndSyncContext, UserStorageSyncedWallet } from '../types';
 
 jest.mock('./validation');
+jest.mock('../../logger');
+
+const mockBackupAndSyncLogger = backupAndSyncLogger as jest.MockedFunction<
+  typeof backupAndSyncLogger
+>;
 
 const mockAssertValidUserStorageWallet =
   assertValidUserStorageWallet as jest.MockedFunction<
@@ -109,6 +115,36 @@ describe('BackupAndSync - UserStorage - FormatUtils', () => {
         groupIndex: 0,
       });
     });
+
+    it('falls back to blank metadata and logs if mask throws due to invalid field types', () => {
+      mockContext.controller.state.accountGroupsMetadata[mockGroup.id] = {
+        // `name` should be an UpdatableField object, not a plain string
+        name: 'invalid-not-an-object' as never,
+      };
+
+      const result = formatGroupForUserStorageUsage(mockContext, mockGroup);
+
+      expect(result).toStrictEqual({ groupIndex: 0 });
+      expect(mockBackupAndSyncLogger).toHaveBeenCalledWith(
+        expect.stringContaining(
+          'Error trying to format group for user storage usage',
+        ),
+      );
+    });
+
+    it('strips fields not in the schema (e.g. lastSelected)', () => {
+      mockContext.controller.state.accountGroupsMetadata[mockGroup.id] = {
+        name: { value: 'Group Name', lastUpdatedAt: 123456 },
+        lastSelected: 999999,
+      };
+
+      const result = formatGroupForUserStorageUsage(mockContext, mockGroup);
+
+      expect(result).toStrictEqual({
+        name: { value: 'Group Name', lastUpdatedAt: 123456 },
+        groupIndex: 0,
+      });
+    });
   });
 
   describe('parseWalletFromUserStorageResponse', () => {

packages/account-tree-controller/src/backup-and-sync/user-storage/format-utils.ts

@@ -1,16 +1,21 @@
+import { mask } from '@metamask/superstruct';
+
 import {
   assertValidUserStorageWallet,
   assertValidUserStorageGroup,
   assertValidLegacyUserStorageAccount,
 } from './validation';
 import type { AccountGroupMultichainAccountObject } from '../../group';
+import { backupAndSyncLogger } from '../../logger';
 import type { AccountWalletEntropyObject } from '../../wallet';
 import type {
   BackupAndSyncContext,
   LegacyUserStorageSyncedAccount,
   UserStorageSyncedWallet,
   UserStorageSyncedWalletGroup,
 } from '../types';
+import { UserStorageSyncedWalletGroupSchema } from '../types';
+import { toErrorMessage } from '../utils/errors';
 
 /**
  * Formats the wallet for user storage usage.
@@ -51,11 +56,26 @@ export const formatGroupForUserStorageUsage = (
   // This can be null if the user has not manually set a name, pinned or hidden the group
   const persistedGroupMetadata =
     context.controller.state.accountGroupsMetadata[group.id];
+  const { groupIndex } = group.metadata.entropy;
 
-  return {
-    ...(persistedGroupMetadata ?? {}),
-    groupIndex: group.metadata.entropy.groupIndex,
-  };
+  try {
+    // We mask and we try catch, since `mask` will throw if the persisted metadata has
+    // fields with wrong types.
+    return mask(
+      {
+        ...(persistedGroupMetadata ?? {}),
+        groupIndex,
+      },
+      UserStorageSyncedWalletGroupSchema,
+    );
+  } catch (error) {
+    backupAndSyncLogger(
+      `Error trying to format group for user storage usage: ${toErrorMessage(error)}`,
+    );
+
+    // If anything goes wrong with this group, we use blank metadata for it.
+    return { groupIndex };
+  }
 };
 
 /**
@@ -76,7 +96,7 @@ export const parseWalletFromUserStorageResponse = (
     return walletData;
   } catch (error: unknown) {
     throw new Error(
-      `Error trying to parse wallet from user storage response: ${error instanceof Error ? error.message : String(error)}`,
+      `Error trying to parse wallet from user storage response: ${toErrorMessage(error)}`,
     );
   }
 };
@@ -99,7 +119,7 @@ export const parseGroupFromUserStorageResponse = (
     return groupData;
   } catch (error: unknown) {
     throw new Error(
-      `Error trying to parse group from user storage response: ${error instanceof Error ? error.message : String(error)}`,
+      `Error trying to parse group from user storage response: ${toErrorMessage(error)}`,
     );
   }
 };
@@ -122,7 +142,7 @@ export const parseLegacyAccountFromUserStorageResponse = (
     return accountData;
   } catch (error: unknown) {
     throw new Error(
-      `Error trying to parse legacy account from user storage response: ${error instanceof Error ? error.message : String(error)}`,
+      `Error trying to parse legacy account from user storage response: ${toErrorMessage(error)}`,
     );
   }
 };

packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts

@@ -22,6 +22,7 @@ import type {
   UserStorageSyncedWallet,
   UserStorageSyncedWalletGroup,
 } from '../types';
+import { toErrorMessage } from '../utils/errors';
 
 /**
  * Retrieves the wallet from user storage.
@@ -53,7 +54,7 @@ export const getWalletFromUserStorage = async (
       return parseWalletFromUserStorageResponse(walletData);
     } catch (error) {
       backupAndSyncLogger(
-        `Failed to parse wallet data from user storage: ${error instanceof Error ? error.message : String(error)}`,
+        `Failed to parse wallet data from user storage: ${toErrorMessage(error)}`,
       );
       return null;
     }
@@ -119,7 +120,7 @@ export const getAllGroupsFromUserStorage = async (
           return parseGroupFromUserStorageResponse(stringifiedGroup);
         } catch (error) {
           backupAndSyncLogger(
-            `Failed to parse group data from user storage: ${error instanceof Error ? error.message : String(error)}`,
+            `Failed to parse group data from user storage: ${toErrorMessage(error)}`,
           );
           return null;
         }
@@ -163,7 +164,7 @@ export const getGroupFromUserStorage = async (
       return parseGroupFromUserStorageResponse(groupData);
     } catch (error) {
       backupAndSyncLogger(
-        `Failed to parse group data from user storage: ${error instanceof Error ? error.message : String(error)}`,
+        `Failed to parse group data from user storage: ${toErrorMessage(error)}`,
       );
       return null;
     }
@@ -270,7 +271,7 @@ export const getAllLegacyUserStorageAccounts = async (
           return parseLegacyAccountFromUserStorageResponse(stringifiedAccount);
         } catch (error) {
           backupAndSyncLogger(
-            `Failed to parse legacy account data from user storage: ${error instanceof Error ? error.message : String(error)}`,
+            `Failed to parse legacy account data from user storage: ${toErrorMessage(error)}`,
           );
           return null;
         }

packages/account-tree-controller/src/backup-and-sync/utils/errors.test.ts

@@ -0,0 +1,17 @@
+import { toErrorMessage } from './errors';
+
+describe('toErrorMessage', () => {
+  it('returns the message property for Error instances', () => {
+    expect(toErrorMessage(new Error('something went wrong'))).toBe(
+      'something went wrong',
+    );
+  });
+
+  it('returns String() for non-Error values', () => {
+    expect(toErrorMessage('raw string error')).toBe('raw string error');
+    expect(toErrorMessage(42)).toBe('42');
+    expect(toErrorMessage(null)).toBe('null');
+    expect(toErrorMessage(undefined)).toBe('undefined');
+    expect(toErrorMessage({ code: 500 })).toBe('[object Object]');
+  });
+});

packages/account-tree-controller/src/backup-and-sync/utils/errors.ts

@@ -0,0 +1,9 @@
+/**
+ * Extracts a string message from an unknown error value.
+ *
+ * @param error - The caught error value.
+ * @returns The error message string.
+ */
+export function toErrorMessage(error: unknown): string {
+  return error instanceof Error ? error.message : String(error);
+}

packages/account-tree-controller/src/backup-and-sync/utils/index.ts

@@ -1 +1,2 @@
 export * from './controller';
+export { toErrorMessage } from './errors';