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

## Explanation

I initially thought calling `init` implicitly would solve some issues,
but it actually adds one when it comes to automatically migrating
accounts from v1 state.

In the clients we usually use the tree like so:
```ts
this.accountsController.updateAccounts();
this.accountTreeController.init();
```

The `updateAccounts` part re-creates the list of internal accounts and
potentially migrate them if needed (in the case of BIP-44, this is
needed to inject the `options.entropy` for those accounts).

If a `KeyingController:stateChange` happens before `updateAccounts`,
this might trigger some `:accountAdded` event, but some accounts might
not have been automatically migrated yet (since `updateAccounts` has not
been called at this point). Thus, this can lead the
`AccountTreeController` to misuse those accounts and put them in wrong
wallet (like older HD accounts had not `options.entropy` objects, thus
they are not considered BIP-44 accounts yet).

To avoid such issues (which seems to mainly happen for E2E), we wait for
`init` to have been called explicitly.

In anyway, calling `updateAccounts` right before `init` would make sure
that the list of accounts is up-to-date AND migrated.

## References

<!--
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these
changes better?
Are there client or consumer pull requests to adopt any breaking
changes?

For example:

* Fixes #12345
* Related to #67890
-->

## 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/contributing.md#updating-changelogs),
highlighting breaking changes as necessary
- [ ] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

Author: ccharly

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);