refactor: update account group name uniqueness checks

packages/account-tree-controller/CHANGELOG.md

@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- Account group name uniqueness validation now scoped to wallet level instead of global ([#6550](https://github.com/MetaMask/core/pull/6550))
+  - `isAccountGroupNameUnique` now checks for duplicates only within the same wallet, allowing different wallets to have groups with the same name.
+  - Function now throws an error for non-existent group IDs instead of returning `true`.
+  - Updated `setAccountGroupName` behavior to allow duplicate names across different wallets.
+
 ## [0.13.1]
 
 ### Fixed

packages/account-tree-controller/src/AccountTreeController.test.ts

@@ -24,6 +24,7 @@ import type { InternalAccount } from '@metamask/keyring-internal-api';
 import type { GetSnap as SnapControllerGetSnap } from '@metamask/snaps-controllers';
 
 import { AccountTreeController } from './AccountTreeController';
+import { isAccountGroupNameUnique } from './group';
 import { getAccountWalletNameFromKeyringType } from './rules/keyring';
 import {
   type AccountTreeControllerMessenger,
@@ -1973,7 +1974,7 @@ describe('AccountTreeController', () => {
       }).not.toThrow();
     });
 
-    it('prevents setting duplicate names across different groups', () => {
+    it('allows duplicate names across different wallets', () => {
       const { controller } = setup({
         accounts: [MOCK_HD_ACCOUNT_1, MOCK_HD_ACCOUNT_2],
         keyrings: [MOCK_HD_KEYRING_1, MOCK_HD_KEYRING_2],
@@ -2001,10 +2002,10 @@ describe('AccountTreeController', () => {
       // Set name for first group - should succeed
       controller.setAccountGroupName(groupId1, duplicateName);
 
-      // Try to set the same name for second group - should throw
+      // Set the same name for second group in different wallet - should succeed
       expect(() => {
         controller.setAccountGroupName(groupId2, duplicateName);
-      }).toThrow('Account group name already exists');
+      }).not.toThrow();
     });
 
     it('ensures unique names when generating default names', () => {
@@ -2026,7 +2027,7 @@ describe('AccountTreeController', () => {
       expect(names.every((name) => name.length > 0)).toBe(true);
     });
 
-    it('prevents duplicate names when comparing trimmed names', () => {
+    it('allows duplicate names with different spacing across different wallets', () => {
       const { controller } = setup({
         accounts: [MOCK_HD_ACCOUNT_1, MOCK_HD_ACCOUNT_2],
         keyrings: [MOCK_HD_KEYRING_1, MOCK_HD_KEYRING_2],
@@ -2052,12 +2053,90 @@ describe('AccountTreeController', () => {
       const nameWithSpaces = '  My Group Name  ';
       controller.setAccountGroupName(groupId1, nameWithSpaces);
 
-      // Try to set the same name for second group with different spacing - should throw
+      // Set the same name for second group with different spacing in different wallet - should succeed
       const nameWithDifferentSpacing = ' My Group Name ';
       expect(() => {
         controller.setAccountGroupName(groupId2, nameWithDifferentSpacing);
+      }).not.toThrow();
+    });
+
+    it('prevents duplicate names within the same wallet', () => {
+      // Create two accounts with the same entropy source to ensure they're in the same wallet
+      const mockAccount1: Bip44Account<InternalAccount> = {
+        ...MOCK_HD_ACCOUNT_1,
+        id: 'mock-id-1',
+        address: '0x123',
+        options: {
+          entropy: {
+            type: KeyringAccountEntropyTypeOption.Mnemonic,
+            id: 'mock-keyring-id-1',
+            groupIndex: 0,
+            derivationPath: '',
+          },
+        },
+      };
+
+      const mockAccount2: Bip44Account<InternalAccount> = {
+        ...MOCK_HD_ACCOUNT_2,
+        id: 'mock-id-2',
+        address: '0x456',
+        options: {
+          entropy: {
+            type: KeyringAccountEntropyTypeOption.Mnemonic,
+            id: 'mock-keyring-id-1', // Same entropy ID as account1
+            groupIndex: 1, // Different group index to create separate groups
+            derivationPath: '',
+          },
+        },
+      };
+
+      const { controller } = setup({
+        accounts: [mockAccount1, mockAccount2],
+        keyrings: [MOCK_HD_KEYRING_1],
+      });
+
+      controller.init();
+
+      const wallets = controller.getAccountWalletObjects();
+      expect(wallets).toHaveLength(1);
+
+      const wallet = wallets[0];
+      const groups = Object.values(wallet.groups);
+
+      expect(groups.length).toBeGreaterThanOrEqual(2);
+
+      const groupId1 = groups[0].id;
+      const groupId2 = groups[1].id;
+      const duplicateName = 'Duplicate Group Name';
+
+      // Set name for first group - should succeed
+      controller.setAccountGroupName(groupId1, duplicateName);
+
+      // Try to set the same name for second group in same wallet - should throw
+      expect(() => {
+        controller.setAccountGroupName(groupId2, duplicateName);
       }).toThrow('Account group name already exists');
     });
+
+    it('throws error for non-existent group ID', () => {
+      const { controller } = setup({
+        accounts: [MOCK_HD_ACCOUNT_1],
+        keyrings: [MOCK_HD_KEYRING_1],
+      });
+
+      controller.init();
+
+      // Test the isAccountGroupNameUnique function directly with a non-existent group ID
+      expect(() => {
+        isAccountGroupNameUnique(
+          controller.state,
+          'non-existent-group-id' as AccountGroupId,
+          'Some Name',
+        );
+      }).toThrow(
+        'Account group with ID "non-existent-group-id" not found in tree',
+      );
+    });
   });
 
   describe('Fallback Naming', () => {

packages/account-tree-controller/src/AccountTreeController.ts

@@ -670,11 +670,11 @@ export class AccountTreeController extends BaseController<
   }
 
   /**
-   * Asserts that an account group name is unique across all groups.
+   * Asserts that an account group name is unique within the same wallet.
    *
    * @param groupId - The account group ID to exclude from the check.
    * @param name - The name to validate for uniqueness.
-   * @throws Error if the name already exists in another group.
+   * @throws Error if the name already exists in another group within the same wallet.
    */
   #assertAccountGroupNameIsUnique(groupId: AccountGroupId, name: string): void {
     if (!isAccountGroupNameUnique(this.state, groupId, name)) {

packages/account-tree-controller/src/group.ts

@@ -87,12 +87,13 @@ export type AccountGroupObjectOf<GroupType extends AccountGroupType> = Extract<
 >['object'];
 
 /**
- * Checks if an account group name is unique across all groups.
+ * Checks if an account group name is unique within the same wallet.
  *
  * @param state - The account tree controller state.
  * @param groupId - The account group ID to exclude from the check.
  * @param name - The name to validate for uniqueness.
- * @returns True if the name is unique, false otherwise.
+ * @returns True if the name is unique within the same wallet, false otherwise.
+ * @throws Error if the group ID does not exist.
  */
 export function isAccountGroupNameUnique(
   state: AccountTreeControllerState,
@@ -101,13 +102,21 @@ export function isAccountGroupNameUnique(
 ): boolean {
   const trimmedName = name.trim();
 
+  // Find the wallet that contains the group being validated
   for (const wallet of Object.values(state.accountTree.wallets)) {
-    for (const group of Object.values(wallet.groups)) {
-      if (group.id !== groupId && group.metadata.name.trim() === trimmedName) {
-        return false;
+    if (wallet.groups[groupId]) {
+      // Check for duplicates only within this wallet
+      for (const group of Object.values(wallet.groups)) {
+        if (
+          group.id !== groupId &&
+          group.metadata.name.trim() === trimmedName
+        ) {
+          return false;
+        }
       }
+      return true;
     }
   }
 
-  return true;
+  throw new Error(`Account group with ID "${groupId}" not found in tree`);
 }