fix(accounts-controller): do not fire events during update blocks (#5555)

## Explanation

Events should be fired after making a state update. Otherwise we could
end up with race conditions like some other component react to
`AccountsController:accountAdded` and try to call
`getAccount(account.id)`, it might not the same value compared to the
`account` being passed during the event (since the state update has not
been persisted yet).

This PR now moves all events firing after the state update.

It also includes a small refactor of the `KeyringController:stateChange`
handler (which made the new events firing logic, a bit easier).

Test E2E PR:
- MetaMask/metamask-extension#31393 (CI is
passing)

## References

N/A

## Changelog

TODO

## 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 highlighted breaking changes using the "BREAKING" category
above as appropriate
- [ ] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

---------

Co-authored-by: Monte Lai <[email protected]>

Author: ccharly

packages/accounts-controller/src/AccountsController.test.ts

@@ -464,7 +464,7 @@ describe('AccountsController', () => {
       expect(listMultichainAccountsSpy).toHaveBeenCalled();
     });
 
-    it('not update state when only keyring is unlocked without any keyrings', async () => {
+    it('does not update state when only keyring is unlocked without any keyrings', async () => {
       const messenger = buildMessenger();
       const { accountsController } = setupAccountsController({
         initialState: {
@@ -652,7 +652,7 @@ describe('AccountsController', () => {
         ]);
       });
 
-      it('handle the event when a Snap deleted the account before the it was added', async () => {
+      it('handles the event when a Snap deleted the account before it was added', async () => {
         mockUUIDWithNormalAccounts([mockAccount]);
 
         const messenger = buildMessenger();
@@ -720,6 +720,66 @@ describe('AccountsController', () => {
         ]);
       });
 
+      it('handles the event when a Snap keyring has been deleted', async () => {
+        mockUUIDWithNormalAccounts([mockAccount]);
+
+        const messenger = buildMessenger();
+        messenger.registerActionHandler(
+          'KeyringController:getKeyringsByType',
+          // The Snap keyring will be treated as undefined
+          mockGetKeyringByType.mockReturnValue([]),
+        );
+
+        const mockNewKeyringState = {
+          isUnlocked: true,
+          keyrings: [
+            {
+              type: KeyringTypes.hd,
+              accounts: [mockAccount.address],
+            },
+            {
+              type: KeyringTypes.snap,
+              // Since the Snap keyring will be mocked as "unavailable", this account won't be added
+              // to the state (like if the Snap did remove it right before the keyring controller
+              // state change got triggered).
+              accounts: [mockAccount3.address],
+            },
+          ],
+          keyringsMetadata: [
+            {
+              id: 'mock-id',
+              name: 'mock-name',
+            },
+            {
+              id: 'mock-id2',
+              name: 'mock-name2',
+            },
+          ],
+        };
+
+        const { accountsController } = setupAccountsController({
+          initialState: {
+            internalAccounts: {
+              accounts: {
+                [mockAccount.id]: mockAccount,
+              },
+              selectedAccount: mockAccount.id,
+            },
+          },
+          messenger,
+        });
+
+        messenger.publish(
+          'KeyringController:stateChange',
+          mockNewKeyringState,
+          [],
+        );
+
+        const accounts = accountsController.listMultichainAccounts();
+
+        expect(accounts).toStrictEqual([setLastSelectedAsAny(mockAccount)]);
+      });
+
       it('increment the default account number when adding an account', async () => {
         const messenger = buildMessenger();
 
@@ -994,7 +1054,9 @@ describe('AccountsController', () => {
 
         // First call is 'KeyringController:stateChange'
         expect(messengerSpy).toHaveBeenNthCalledWith(
-          2,
+          // 1. KeyringController:stateChange
+          // 2. AccountsController:stateChange
+          3,
           'AccountsController:accountAdded',
           setLastSelectedAsAny(mockAccount2),
         );
@@ -1298,7 +1360,9 @@ describe('AccountsController', () => {
 
         // First call is 'KeyringController:stateChange'
         expect(messengerSpy).toHaveBeenNthCalledWith(
-          2,
+          // 1. KeyringController:stateChange
+          // 2. AccountsController:stateChange
+          3,
           'AccountsController:accountRemoved',
           mockAccount3.id,
         );
@@ -2165,6 +2229,10 @@ describe('AccountsController', () => {
         expect(selectedAccount.id).toStrictEqual(expectedSelectedId);
       },
     );
+
+    it.todo(
+      'does not re-fire a accountChanged event if the account is still the same',
+    );
   });
 
   describe('loadBackup', () => {
@@ -2603,7 +2671,7 @@ describe('AccountsController', () => {
       ).toStrictEqual(mockAccount2.id);
     });
 
-    it('not emit setSelectedEvmAccountChange if the account is non-EVM', () => {
+    it('does not emit setSelectedEvmAccountChange if the account is non-EVM', () => {
       const mockNonEvmAccount = createMockInternalAccount({
         id: 'mock-non-evm',
         name: 'non-evm',
@@ -2635,17 +2703,18 @@ describe('AccountsController', () => {
       expect(
         accountsController.state.internalAccounts.selectedAccount,
       ).toStrictEqual(mockNonEvmAccount.id);
+      console.log(accountsController.state.internalAccounts.selectedAccount);
 
       expect(messengerSpy.mock.calls).toHaveLength(2); // state change and then selectedAccountChange
 
-      expect(messengerSpy).not.toHaveBeenCalledWith(
+      expect(messengerSpy).not.toHaveBeenLastCalledWith(
         'AccountsController:selectedEvmAccountChange',
         mockNonEvmAccount,
       );
 
-      expect(messengerSpy).toHaveBeenCalledWith(
+      expect(messengerSpy).toHaveBeenLastCalledWith(
         'AccountsController:selectedAccountChange',
-        mockNonEvmAccount,
+        setLastSelectedAsAny(mockNonEvmAccount),
       );
     });
   });