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

packages/account-tree-controller/CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- Prevent `:account{Added,Removed}` to be used if `init` has not been called yet ([#6717](https://github.com/MetaMask/core/pull/6717))
+  - We now wait for `init` to have been called at least once. Clients will need to ensure internal accounts are fully ready before calling `init`.
+  - This should also enforce account group ordering, since all accounts will be ready to consume right away.
+
 ## [1.2.0]
 
 ### Added

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

@@ -1215,6 +1215,28 @@ describe('AccountTreeController', () => {
         },
       } as AccountTreeControllerState);
     });
+
+    it('does not remove account if init has not been called', () => {
+      const { controller, messenger } = setup({
+        accounts: [MOCK_HD_ACCOUNT_1],
+      });
+
+      // Force ref to the controller, even if we don't use it in this test.
+      expect(controller).toBeDefined();
+
+      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 +1509,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 +4296,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 +4450,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,12 +655,14 @@ 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.
+    // Check if this account is already known by the tree to avoid double-insertion.
     if (!this.#accountIdToContext.has(account.id)) {
       this.update((state) => {
         this.#insert(state.accountTree.wallets, account);
@@ -700,9 +693,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);