fix(account-tree-controller): prevent :account{Added,Removed} if init has not been called yet

Author: ccharly

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

@@ -32,7 +32,7 @@ import {
 } from './AccountTreeController';
 import type { BackupAndSyncAnalyticsEventPayload } from './backup-and-sync/analytics';
 import { BackupAndSyncService } from './backup-and-sync/service';
-import { isAccountGroupNameUnique } from './group';
+import { AccountGroupMultichainAccountObject, isAccountGroupNameUnique } from './group';
 import { getAccountWalletNameFromKeyringType } from './rules/keyring';
 import {
   type AccountTreeControllerMessenger,
@@ -42,6 +42,7 @@ import {
   type AllowedActions,
   type AllowedEvents,
 } from './types';
+import { AccountWalletKeyringObject, AccountWalletObject } from './wallet';
 
 // Local mock of EMPTY_ACCOUNT to avoid circular dependency
 const EMPTY_ACCOUNT_MOCK: InternalAccount = {
@@ -1215,6 +1216,26 @@ describe('AccountTreeController', () => {
         },
       } as AccountTreeControllerState);
     });
+
+    it('does not remove account if init has not been called', () => {
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      const { controller, messenger } = setup({
+        accounts: [MOCK_HD_ACCOUNT_1],
+      });
+
+      const mockAccountTreeChange = jest.fn();
+      messenger.subscribe(
+        'AccountTreeController:accountTreeChange',
+        mockAccountTreeChange,
+      );
+
+      messenger.publish(
+        'AccountsController:accountRemoved',
+        MOCK_HD_ACCOUNT_1.id,
+      );
+
+      expect(mockAccountTreeChange).not.toHaveBeenCalled();
+    });
   });
 
   describe('account ordering by type', () => {
@@ -1487,6 +1508,14 @@ describe('AccountTreeController', () => {
         hasAccountTreeSyncingSyncedAtLeastOnce: false,
       } as AccountTreeControllerState);
     });
+
+    it('does not add any account if init has not been called', () => {
+      const { controller, messenger } = setup();
+
+      expect(controller.state.accountTree.wallets).toStrictEqual({});
+      messenger.publish('AccountsController:accountAdded', MOCK_HD_ACCOUNT_1);
+      expect(controller.state.accountTree.wallets).toStrictEqual({});
+    });
   });
 
   describe('on MultichainAccountService:walletStatusUpdate', () => {
@@ -4266,6 +4295,8 @@ describe('AccountTreeController', () => {
     it('names non-HD keyrings accounts properly', () => {
       const { controller, messenger } = setup();
 
+      controller.init();
+
       // Add all 3 accounts.
       [mockAccount1, mockAccount2, mockAccount3].forEach(
         (mockAccount, index) => {
@@ -4418,6 +4449,8 @@ describe('AccountTreeController', () => {
     it('fallbacks to natural indexing if group names are not using our default name pattern', () => {
       const { controller, messenger } = setup();
 
+      controller.init();
+
       [mockAccount1, mockAccount2, mockAccount3].forEach((mockAccount) =>
         messenger.publish('AccountsController:accountAdded', mockAccount),
       );

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

@@ -346,15 +346,6 @@ export class AccountTreeController extends BaseController<
     this.init();
   }
 
-  /**
-   * Force-init if the controller's state has not been initilized yet.
-   */
-  #initAtLeastOnce() {
-    if (!this.#initialized) {
-      this.init();
-    }
-  }
-
   /**
    * Rule for entropy-base wallets.
    *
@@ -664,9 +655,12 @@ export class AccountTreeController extends BaseController<
    * @param account - New account.
    */
   #handleAccountAdded(account: InternalAccount) {
-    // We force-init to make sure we have the proper account groups for the
-    // incoming account change.
-    this.#initAtLeastOnce();
+    // We wait for the first `init` to be called to actually build up the tree and
+    // mutate it. We expect the caller to first update the `AccountsController` state
+    // to force the migration of accounts, and then call `init`.
+    if (!this.#initialized) {
+      return;
+    }
 
     // Check if this account got already added by `#initAtLeastOnce`, if not, then we
     // can proceed.
@@ -700,9 +694,12 @@ export class AccountTreeController extends BaseController<
    * @param accountId - Removed account ID.
    */
   #handleAccountRemoved(accountId: AccountId) {
-    // We force-init to make sure we have the proper account groups for the
-    // incoming account change.
-    this.#initAtLeastOnce();
+    // We wait for the first `init` to be called to actually build up the tree and
+    // mutate it. We expect the caller to first update the `AccountsController` state
+    // to force the migration of accounts, and then call `init`.
+    if (!this.#initialized) {
+      return;
+    }
 
     const context = this.#accountIdToContext.get(accountId);