fix(26748): increment index label until get a label not duplicated

MetaMask · Sep 9, 2024 · 4b90b44 · 4b90b44
1 parent 416d024
commit 4b90b44
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 29 deletions.
diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js
@@ -4567,6 +4567,24 @@ export default class MetamaskController extends EventEmitter {
     } ${hdPathDescription || ''}`.trim();
   }
 
+  /**
+   * Set account name that checks for identical label
+   *
+   * @param account
+   * @param index
+   * @param hwDeviceName
+   * @param hdPathDescription
+   */
+  setAccountName(account, index, hwDeviceName, hdPathDescription) {
+    const label = this.getAccountLabel(hwDeviceName, index, hdPathDescription);
+    try {
+      this.accountsController.setAccountName(account.id, label);
+    } catch {
+      const newIndex = index + 1;
+      this.setAccountName(account, newIndex, hwDeviceName, hdPathDescription);
+    }
+  }
+
   /**
    * Imports an account from a Trezor or Ledger device.
    *
@@ -4583,26 +4601,16 @@ export default class MetamaskController extends EventEmitter {
     hdPathDescription,
   ) {
     const keyring = await this.getKeyringForDevice(deviceName, hdPath);
-
     keyring.setAccountToUnlock(index);
     const unlockedAccount =
       await this.keyringController.addNewAccountForKeyring(keyring);
-    const label = this.getAccountLabel(
-      deviceName === HardwareDeviceNames.qr ? keyring.getName() : deviceName,
-      index,
-      hdPathDescription,
-    );
-    // Set the account label to Trezor 1 / Ledger 1 / QR Hardware 1, etc
-    this.preferencesController.setAccountLabel(unlockedAccount, label);
-    // Select the account
-    this.preferencesController.setSelectedAddress(unlockedAccount);
-
-    // It is expected that the account also exist in the accounts-controller
-    // in other case, an error shall be thrown
+    const hwDeviceName =
+      deviceName === HardwareDeviceNames.qr ? keyring.getName() : deviceName;
     const account =
       this.accountsController.getAccountByAddress(unlockedAccount);
-    this.accountsController.setAccountName(account.id, label);
+    this.setAccountName(account, index, hwDeviceName, hdPathDescription);
 
+    this.preferencesController.setSelectedAddress(unlockedAccount);
     const accounts = this.accountsController.listAccounts();
 
     const { identities } = this.preferencesController.store.getState();

diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js
@@ -991,10 +991,10 @@ describe('MetaMaskController', () => {
                 ).toHaveBeenCalledTimes(1);
               });
 
-              it('should call preferencesController.setAccountLabel', async () => {
+              it('should call accountsController.getAccountByAddress', async () => {
                 jest.spyOn(
-                  metamaskController.preferencesController,
-                  'setAccountLabel',
+                  metamaskController.accountsController,
+                  'getAccountByAddress',
                 );
 
                 await metamaskController.unlockHardwareWalletAccount(
@@ -1003,14 +1003,14 @@ describe('MetaMaskController', () => {
                 );
 
                 expect(
-                  metamaskController.preferencesController.setAccountLabel,
+                  metamaskController.accountsController.getAccountByAddress,
                 ).toHaveBeenCalledTimes(1);
               });
 
-              it('should call accountsController.getAccountByAddress', async () => {
+              it('should call accountsController.setAccountName', async () => {
                 jest.spyOn(
                   metamaskController.accountsController,
-                  'getAccountByAddress',
+                  'setAccountName',
                 );
 
                 await metamaskController.unlockHardwareWalletAccount(
@@ -1019,24 +1019,38 @@ describe('MetaMaskController', () => {
                 );
 
                 expect(
-                  metamaskController.accountsController.getAccountByAddress,
+                  metamaskController.accountsController.setAccountName,
                 ).toHaveBeenCalledTimes(1);
               });
 
-              it('should call accountsController.setAccountName', async () => {
-                jest.spyOn(
-                  metamaskController.accountsController,
-                  'setAccountName',
-                );
-
+              it('should call twice getAccountLabel if accountsController.setAccountName throws an error', async () => {
+                jest
+                  .spyOn(
+                    metamaskController.accountsController,
+                    'setAccountName',
+                  )
+                  .mockImplementationOnce(() => {
+                    throw new Error();
+                  });
+                jest.spyOn(metamaskController, 'getAccountLabel');
                 await metamaskController.unlockHardwareWalletAccount(
                   accountToUnlock,
                   device,
                 );
 
                 expect(
-                  metamaskController.accountsController.setAccountName,
-                ).toHaveBeenCalledTimes(1);
+                  metamaskController.getAccountLabel,
+                ).toHaveBeenCalledTimes(2);
+                expect(metamaskController.getAccountLabel).toHaveBeenCalledWith(
+                  device,
+                  0,
+                  undefined,
+                );
+                expect(metamaskController.getAccountLabel).toHaveBeenCalledWith(
+                  device,
+                  1,
+                  undefined,
+                );
               });
             });
           },