diff --git a/packages/accounts-controller/CHANGELOG.md b/packages/accounts-controller/CHANGELOG.md index 8ba444bbe2b..bbef70493cd 100644 --- a/packages/accounts-controller/CHANGELOG.md +++ b/packages/accounts-controller/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** Use new `KeyringController:account{Added,Removed}` events instead of the `KeyringController:stateChange` ([#7328](https://github.com/MetaMask/core/pull/7328)) + - This simplify the re-synchronization between keyrings and this controller. + - This should also improve performance slightly. - Move peer dependencies for controller and service packages to direct dependencies ([#7209](https://github.com/MetaMask/core/pull/7209), [#7258](https://github.com/MetaMask/core/pull/7258)) - The dependencies moved are: - `@metamask/keyring-controller` (^25.0.0) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 202a4613061..c99e168c6c4 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -13,10 +13,7 @@ import { EthScope, KeyringAccountEntropyTypeOption, } from '@metamask/keyring-api'; -import { - KeyringControllerState, - KeyringTypes, -} from '@metamask/keyring-controller'; +import { KeyringTypes } from '@metamask/keyring-controller'; import type { InternalAccount } from '@metamask/keyring-internal-api'; import { MOCK_ANY_NAMESPACE, Messenger } from '@metamask/messenger'; import type { @@ -273,7 +270,8 @@ function buildAccountsControllerMessenger( ], events: [ 'SnapController:stateChange', - 'KeyringController:stateChange', + 'KeyringController:accountAdded', + 'KeyringController:accountRemoved', 'SnapKeyring:accountAssetListUpdated', 'SnapKeyring:accountBalancesUpdated', 'SnapKeyring:accountTransactionsUpdated', @@ -577,136 +575,18 @@ describe('AccountsController', () => { }); }); - describe('onKeyringStateChange', () => { - it('uses listMultichainAccounts', async () => { - const messenger = buildMessenger(); - - mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); - - const { accountsController } = setupAccountsController({ - initialState: { - internalAccounts: { - accounts: {}, - selectedAccount: '', - }, - }, - messenger, - }); - - const listMultichainAccountsSpy = jest.spyOn( - accountsController, - 'listMultichainAccounts', - ); - - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; - - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); - - expect(listMultichainAccountsSpy).toHaveBeenCalled(); - }); - - it('does not update state when only keyring is unlocked without any keyrings', async () => { - const messenger = buildMessenger(); - const { accountsController } = setupAccountsController({ - initialState: { - internalAccounts: { - accounts: {}, - selectedAccount: '', - }, - }, - messenger, - }); - - messenger.publish( - 'KeyringController:stateChange', - { isUnlocked: true, keyrings: [] }, - [], - ); - - const accounts = accountsController.listMultichainAccounts(); - - expect(accounts).toStrictEqual([]); - }); - - it('only update if the keyring is unlocked and when there are keyrings', async () => { - const messenger = buildMessenger(); - - const mockNewKeyringState = { - isUnlocked: false, - keyrings: [ - { - accounts: [mockAccount.address, mockAccount2.address], - type: KeyringTypes.hd, - id: '123', - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; - const { accountsController } = setupAccountsController({ - initialState: { - internalAccounts: { - accounts: {}, - selectedAccount: '', - }, - }, - messenger, - }); - - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); - - const accounts = accountsController.listMultichainAccounts(); - - expect(accounts).toStrictEqual([]); - }); - + describe('on KeyringController:accountAdded', () => { describe('adding accounts', () => { it('add new accounts', async () => { const messenger = buildMessenger(); - mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { accounts: { [mockAccount.id]: mockAccount, - [mockAccount3.id]: mockAccount3, }, selectedAccount: mockAccount.id, }, @@ -714,10 +594,18 @@ describe('AccountsController', () => { messenger, }); + const keyring = { + type: KeyringTypes.hd, + accounts: [mockAccount.address, mockAccount2.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount2.address, + keyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -748,28 +636,6 @@ describe('AccountsController', () => { ]), ); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - { - type: KeyringTypes.snap, - accounts: [mockAccount3.address, mockAccount4.address], - metadata: { - id: 'mock-id2', - name: 'mock-name2', - }, - }, - ], - }; - const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -783,10 +649,18 @@ describe('AccountsController', () => { messenger, }); + const snapKeyring = { + type: KeyringTypes.snap, + accounts: [mockAccount3.address, mockAccount4.address], + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + snapKeyring.accounts[0], + snapKeyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -823,28 +697,6 @@ describe('AccountsController', () => { ]), ); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - { - type: KeyringTypes.snap, - accounts: [mockAccount3.address, mockAccount4.address], - metadata: { - id: 'mock-id2', - name: 'mock-name2', - }, - }, - ], - }; - const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -858,10 +710,18 @@ describe('AccountsController', () => { messenger, }); + const snapKeyring = { + type: KeyringTypes.snap, + accounts: [mockAccount3.address, mockAccount4.address], + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount3.address, + snapKeyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -882,31 +742,6 @@ describe('AccountsController', () => { mockGetKeyringByType.mockReturnValue([]), ); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - { - 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], - metadata: { - id: 'mock-id2', - name: 'mock-name2', - }, - }, - ], - }; - const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -919,10 +754,21 @@ describe('AccountsController', () => { messenger, }); + // 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). + const snapKeyring = { + type: KeyringTypes.snap, + accounts: [mockAccount3.address], + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount3.address, + snapKeyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -937,23 +783,6 @@ describe('AccountsController', () => { mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [ - mockAccount.address, - mockAccount2.address, - mockAccount3.address, - ], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -967,10 +796,22 @@ describe('AccountsController', () => { messenger, }); + const keyring = { + type: KeyringTypes.hd, + accounts: [ + mockAccount.address, + mockAccount2.address, + mockAccount3.address, + ], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount3.address, + keyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -1006,23 +847,6 @@ describe('AccountsController', () => { lastSelected: 1955565967656, }); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [ - mockAccount.address, - mockAccount2.address, - mockAccount3.address, - ], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1036,10 +860,22 @@ describe('AccountsController', () => { messenger, }); + const keyring = { + type: KeyringTypes.hd, + accounts: [ + mockAccount.address, + mockAccount2WithCustomName.address, + mockAccount3.address, + ], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount3.address, + keyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -1070,33 +906,11 @@ describe('AccountsController', () => { mockGetKeyringByType.mockReturnValue([ { type: KeyringTypes.snap, - getAccountByAddress: jest.fn().mockReturnValueOnce(null), + getAccountByAddress: jest.fn().mockReturnValueOnce(mockAccount3), }, ]), ); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - { - type: KeyringTypes.snap, - accounts: [mockAccount3.address], - metadata: { - id: 'mock-id2', - name: 'mock-name2', - }, - }, - ], - }; - const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1107,15 +921,22 @@ describe('AccountsController', () => { messenger, }); + const snapKeyring = { + type: KeyringTypes.snap, + accounts: [mockAccount3.address], + metadata: { id: 'mock-id2', name: 'mock-name2' }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount3.address, + snapKeyring, ); const { selectedAccount } = accountsController.state.internalAccounts; - expect(selectedAccount).toBe(''); + // The 'missing' account was not found, so the selectedAccount has been set to the + // newly added account automatically. + expect(selectedAccount).toBe(mockAccount3.id); }); it('selectedAccount remains the same after adding a new account', async () => { @@ -1123,25 +944,11 @@ describe('AccountsController', () => { mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { accounts: { [mockAccount.id]: mockAccount, - [mockAccount3.id]: mockAccount3, }, selectedAccount: mockAccount.id, }, @@ -1149,10 +956,18 @@ describe('AccountsController', () => { messenger, }); + const keyring = { + type: KeyringTypes.hd, + accounts: [mockAccount.address, mockAccount2.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount2.address, + keyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -1167,7 +982,7 @@ describe('AccountsController', () => { expect(accountsController.getSelectedAccount().id).toBe(mockAccount.id); }); - it('publishes accountAdded event', async () => { + it('publishes :accountAdded event', async () => { const messenger = buildMessenger(); mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); @@ -1186,24 +1001,18 @@ describe('AccountsController', () => { const messengerSpy = jest.spyOn(accountsControllerMessenger, 'publish'); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], + const keyring = { + type: KeyringTypes.hd, + accounts: [mockAccount.address, mockAccount2.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, }; - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount2.address, + keyring, ); // First call is 'AccountsController:stateChange' @@ -1225,19 +1034,6 @@ describe('AccountsController', () => { mockUUIDWithNormalAccounts([mockAccount2]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1252,9 +1048,8 @@ describe('AccountsController', () => { }); messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountRemoved', + mockAccount.address, ); const accounts = accountsController.listMultichainAccounts(); @@ -1272,19 +1067,6 @@ describe('AccountsController', () => { mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1307,11 +1089,7 @@ describe('AccountsController', () => { messenger, }); - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); + messenger.publish('KeyringController:accountRemoved', '0x999'); const accounts = accountsController.listMultichainAccounts(); @@ -1336,19 +1114,7 @@ describe('AccountsController', () => { lastSelected: undefined, }, }; - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; + const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1371,11 +1137,7 @@ describe('AccountsController', () => { messenger, }); - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); + messenger.publish('KeyringController:accountRemoved', '0x999'); const accounts = accountsController.listMultichainAccounts(); @@ -1410,22 +1172,6 @@ describe('AccountsController', () => { }, }; - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [ - mockAccountWithoutLastSelected.address, - mockAccount2.address, - ], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1443,11 +1189,7 @@ describe('AccountsController', () => { messenger, }); - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); + messenger.publish('KeyringController:accountRemoved', '0x999'); const accounts = accountsController.listMultichainAccounts(); @@ -1468,7 +1210,7 @@ describe('AccountsController', () => { ); }); - it('publishes accountRemoved event', async () => { + it('publishes :accountRemoved event', async () => { const messenger = buildMessenger(); mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); @@ -1488,23 +1230,9 @@ describe('AccountsController', () => { const messengerSpy = jest.spyOn(accountsControllerMessenger, 'publish'); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address, mockAccount2.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountRemoved', + mockAccount3.address, ); // First call is 'AccountsController:stateChange' @@ -1538,19 +1266,6 @@ describe('AccountsController', () => { mockReinitialisedAccount, ]); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockReinitialisedAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -1563,10 +1278,22 @@ describe('AccountsController', () => { messenger, }); + const keyring = { + type: KeyringTypes.hd, + accounts: [mockReinitialisedAccount.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; + messenger.publish( + 'KeyringController:accountRemoved', + mockInitialAccount.address, + ); messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockReinitialisedAccount.address, + keyring, ); const selectedAccount = accountsController.getSelectedAccount(); @@ -1582,92 +1309,6 @@ describe('AccountsController', () => { expect(accounts).toStrictEqual([expectedAccount]); }); - it.each([ - { - lastSelectedForAccount1: 1111, - lastSelectedForAccount2: 9999, - expectedSelectedId: 'mock-id2', - }, - { - lastSelectedForAccount1: undefined, - lastSelectedForAccount2: 9999, - expectedSelectedId: 'mock-id2', - }, - { - lastSelectedForAccount1: 1111, - lastSelectedForAccount2: undefined, - expectedSelectedId: 'mock-id', - }, - { - lastSelectedForAccount1: 1111, - lastSelectedForAccount2: 0, - expectedSelectedId: 'mock-id', - }, - ])( - 'handle keyring reinitialization with multiple accounts. Account 1 lastSelected $lastSelectedForAccount1, Account 2 lastSelected $lastSelectedForAccount2. Expected selected account: $expectedSelectedId', - async ({ - lastSelectedForAccount1, - lastSelectedForAccount2, - expectedSelectedId, - }) => { - const messenger = buildMessenger(); - const mockExistingAccount1 = createMockInternalAccount({ - id: 'mock-id', - name: 'Account 1', - address: '0x123', - keyringType: KeyringTypes.hd, - }); - mockExistingAccount1.metadata.lastSelected = lastSelectedForAccount1; - const mockExistingAccount2 = createMockInternalAccount({ - id: 'mock-id2', - name: 'Account 2', - address: '0x456', - keyringType: KeyringTypes.hd, - }); - mockExistingAccount2.metadata.lastSelected = lastSelectedForAccount2; - - mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); - - const { accountsController } = setupAccountsController({ - initialState: { - internalAccounts: { - accounts: { - [mockExistingAccount1.id]: mockExistingAccount1, - [mockExistingAccount2.id]: mockExistingAccount2, - }, - selectedAccount: 'unknown', - }, - }, - messenger, - }); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [ - mockExistingAccount1.address, - mockExistingAccount2.address, - ], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; - messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], - ); - - const selectedAccount = accountsController.getSelectedAccount(); - - expect(selectedAccount.id).toStrictEqual(expectedSelectedId); - }, - ); - it('fires :accountAdded before :selectedAccountChange', async () => { const messenger = buildMessenger(); @@ -1683,20 +1324,6 @@ describe('AccountsController', () => { messenger, }); - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - ], - }; - const mockEventsOrder = jest.fn(); messenger.subscribe('AccountsController:accountAdded', () => { @@ -1708,10 +1335,18 @@ describe('AccountsController', () => { expect(accountsController.getSelectedAccount()).toBe(EMPTY_ACCOUNT); + const keyring = { + type: KeyringTypes.hd, + accounts: [mockAccount.address], + metadata: { + id: 'mock-id', + name: 'mock-name', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringState, - [], + 'KeyringController:accountAdded', + mockAccount.address, + keyring, ); expect(mockEventsOrder).toHaveBeenNthCalledWith( @@ -3469,32 +3104,6 @@ describe('AccountsController', () => { keyringType: KeyringTypes.simple, }); - const mockNewKeyringStateWith = ( - simpleAddressess: string[], - ): KeyringControllerState => { - return { - isUnlocked: true, - keyrings: [ - { - type: 'HD Key Tree', - accounts: [mockAccount.address], - metadata: { - id: 'mock-id', - name: 'mock-name', - }, - }, - { - type: 'Simple Key Pair', - accounts: simpleAddressess, - metadata: { - id: 'mock-id2', - name: 'mock-name2', - }, - }, - ], - }; - }; - it('return the next account number', async () => { const messenger = buildMessenger(); @@ -3516,13 +3125,23 @@ describe('AccountsController', () => { messenger, }); + const simpleKeyring = { + type: 'Simple Key Pair', + accounts: [mockSimpleKeyring1.address, mockSimpleKeyring2.address], + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringStateWith([ - mockSimpleKeyring1.address, - mockSimpleKeyring2.address, - ]), - [], + 'KeyringController:accountAdded', + mockSimpleKeyring1.address, + simpleKeyring, + ); + messenger.publish( + 'KeyringController:accountAdded', + mockSimpleKeyring2.address, + simpleKeyring, ); const accounts = accountsController.listMultichainAccounts(); @@ -3555,31 +3174,37 @@ describe('AccountsController', () => { messenger, }); + const simpleKeyring = { + type: 'Simple Key Pair', + accounts: [mockSimpleKeyring1.address, mockSimpleKeyring2.address], + metadata: { + id: 'mock-id2', + name: 'mock-name2', + }, + }; messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringStateWith([ - mockSimpleKeyring1.address, - mockSimpleKeyring2.address, - ]), - [], + 'KeyringController:accountAdded', + mockSimpleKeyring1.address, + simpleKeyring, + ); + messenger.publish( + 'KeyringController:accountAdded', + mockSimpleKeyring2.address, + simpleKeyring, ); // We then remove "Acccount 2" to create a gap messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringStateWith([mockSimpleKeyring2.address]), - [], + 'KeyringController:accountRemoved', + mockSimpleKeyring1.address, ); // Finally we add a 3rd account, and it should be named "Account 4" (despite having a gap // since "Account 2" no longer exists) messenger.publish( - 'KeyringController:stateChange', - mockNewKeyringStateWith([ - mockSimpleKeyring2.address, - mockSimpleKeyring3.address, - ]), - [], + 'KeyringController:accountAdded', + mockSimpleKeyring3.address, + simpleKeyring, ); const accounts = accountsController.listMultichainAccounts(); diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 05aa4b867e7..a3ebe8d4e74 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -19,9 +19,10 @@ import { } from '@metamask/keyring-api'; import { KeyringTypes } from '@metamask/keyring-controller'; import type { - KeyringControllerState, KeyringControllerGetKeyringsByTypeAction, KeyringControllerStateChangeEvent, + KeyringControllerAccountAddedEvent, + KeyringControllerAccountRemovedEvent, KeyringControllerGetStateAction, KeyringObject, } from '@metamask/keyring-controller'; @@ -202,6 +203,8 @@ export type AccountsControllerAccountAssetListUpdatedEvent = { export type AllowedEvents = | SnapStateChange | KeyringControllerStateChangeEvent + | KeyringControllerAccountRemovedEvent + | KeyringControllerAccountAddedEvent | SnapKeyringAccountAssetListUpdatedEvent | SnapKeyringAccountBalancesUpdatedEvent | SnapKeyringAccountTransactionsUpdatedEvent @@ -256,19 +259,6 @@ export const EMPTY_ACCOUNT = { }, }; -/** - * A patch representing a keyring state change. - */ -type StatePatch = { - previous: Record; - added: { - address: string; - keyring: KeyringObject; - }[]; - updated: InternalAccount[]; - removed: InternalAccount[]; -}; - /** * Controller that manages internal accounts. * The accounts controller is responsible for creating and managing internal accounts. @@ -765,164 +755,73 @@ export class AccountsController extends BaseController< } /** - * Handles changes in the keyring state, specifically when new accounts are added or removed. + * Handle the addition of a new account in a keyring. * - * @param keyringState - The new state of the keyring controller. - * @param keyringState.isUnlocked - True if the keyrings are unlocked, false otherwise. - * @param keyringState.keyrings - List of all keyrings. + * @param address - The address of the new account. + * @param keyring - The keyring object of that new account. */ - #handleOnKeyringStateChange({ - isUnlocked, - keyrings, - }: KeyringControllerState): void { - // TODO: Change when accountAdded event is added to the keyring controller. - - // We check for keyrings length to be greater than 0 because the extension client may try execute - // submit password twice and clear the keyring state. - // https://github.com/MetaMask/KeyringController/blob/2d73a4deed8d013913f6ef0c9f5c0bb7c614f7d3/src/KeyringController.ts#L910 - if (!isUnlocked || keyrings.length === 0) { - return; - } + #handleOnKeyringAccountAdded(address: string, keyring: KeyringObject): void { + let account = this.#getInternalAccountFromAddressAndType(address, keyring); + if (account) { + // Re-compute the list of accounts everytime, so we can make sure new names + // are also considered. + const accounts = Object.values(this.state.internalAccounts.accounts); + + // Get next account name available for this given keyring. + const name = this.getNextAvailableAccountName( + account.metadata.keyring.type, + accounts, + ); - // State patches. - const generatePatch = (): StatePatch => { - return { - previous: {}, - added: [], - updated: [], - removed: [], + // If it's the first account, we need to select it. + const lastSelected = + accounts.length === 0 ? this.#getLastSelectedIndex() : 0; + + // Update account metadata. + account = { + ...account, + metadata: { + ...account.metadata, + name, + importTime: Date.now(), + lastSelected, + }, }; - }; - const patches = { - snap: generatePatch(), - normal: generatePatch(), - }; - - // Gets the patch object based on the keyring type (since Snap accounts and other accounts - // are handled differently). - const patchOf = (type: string): StatePatch => { - if (isSnapKeyringType(type)) { - return patches.snap; - } - return patches.normal; - }; - // Create a map (with lower-cased addresses) of all existing accounts. - for (const account of this.listMultichainAccounts()) { - const address = account.address.toLowerCase(); - const patch = patchOf(account.metadata.keyring.type); - - patch.previous[address] = account; + // Not sure why, but the compiler still infers `account` as possibly undefined. + const internalAccount: InternalAccount = account; + this.#update( + (state) => { + state.internalAccounts.accounts[internalAccount.id] = internalAccount; + }, + () => { + // Will be published before `:selectedAccountChange` event is published. + this.messenger.publish( + 'AccountsController:accountAdded', + internalAccount, + ); + }, + ); } + } - // Go over all keyring changes and create patches out of it. - const addresses = new Set(); - for (const keyring of keyrings) { - const patch = patchOf(keyring.type); - - for (const accountAddress of keyring.accounts) { - // Lower-case address to use it in the `previous` map. - const address = accountAddress.toLowerCase(); - const account = patch.previous[address]; - - if (account) { - // If the account exists before, this might be an update. - patch.updated.push(account); - } else { - // Otherwise, that's a new account. - patch.added.push({ - address, - keyring, - }); - } + /** + * Handle the removal of an existing account from a keyring. + * + * @param address - The address of the new account. + */ + #handleOnKeyringAccountRemoved(address: string): void { + const account = this.listMultichainAccounts().find( + ({ address: accountAddress }) => accountAddress === address, + ); - // Keep track of those address to check for removed accounts later. - addresses.add(address); - } - } + if (account) { + this.#update((state) => { + delete state.internalAccounts.accounts[account.id]; + }); - // We might have accounts associated with removed keyrings, so we iterate - // over all previous known accounts and check against the keyring addresses. - for (const patch of [patches.snap, patches.normal]) { - for (const [address, account] of Object.entries(patch.previous)) { - // If a previous address is not part of the new addesses, then it got removed. - if (!addresses.has(address)) { - patch.removed.push(account); - } - } + this.messenger.publish('AccountsController:accountRemoved', account.id); } - - // Diff that we will use to publish events afterward. - const diff: { removed: string[]; added: InternalAccount[] } = { - removed: [], - added: [], - }; - - this.#update( - (state) => { - const { internalAccounts } = state; - - for (const patch of [patches.snap, patches.normal]) { - for (const account of patch.removed) { - delete internalAccounts.accounts[account.id]; - - diff.removed.push(account.id); - } - - for (const added of patch.added) { - const account = this.#getInternalAccountFromAddressAndType( - added.address, - added.keyring, - ); - - if (account) { - // Re-compute the list of accounts everytime, so we can make sure new names - // are also considered. - const accounts = Object.values( - internalAccounts.accounts, - ) as InternalAccount[]; - - // Get next account name available for this given keyring. - const name = this.getNextAvailableAccountName( - account.metadata.keyring.type, - accounts, - ); - - // If it's the first account, we need to select it. - const lastSelected = - accounts.length === 0 ? this.#getLastSelectedIndex() : 0; - - internalAccounts.accounts[account.id] = { - ...account, - metadata: { - ...account.metadata, - name, - importTime: Date.now(), - lastSelected, - }, - }; - - diff.added.push(internalAccounts.accounts[account.id]); - } - } - } - }, - // Will get executed after the update, but before re-selecting an account in case - // the current one is not valid anymore. - () => { - // Now publish events - for (const id of diff.removed) { - this.messenger.publish('AccountsController:accountRemoved', id); - } - - for (const account of diff.added) { - this.messenger.publish('AccountsController:accountAdded', account); - } - }, - ); - - // NOTE: Since we also track "updated" accounts with our patches, we could fire a new event - // like `accountUpdated` (we would still need to check if anything really changed on the account). } /** @@ -1231,8 +1130,13 @@ export class AccountsController extends BaseController< this.#handleOnSnapStateChange(snapStateState), ); - this.messenger.subscribe('KeyringController:stateChange', (keyringState) => - this.#handleOnKeyringStateChange(keyringState), + this.messenger.subscribe('KeyringController:accountRemoved', (address) => + this.#handleOnKeyringAccountRemoved(address), + ); + + this.messenger.subscribe( + 'KeyringController:accountAdded', + (address, keyring) => this.#handleOnKeyringAccountAdded(address, keyring), ); this.messenger.subscribe( diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 4c695436dbf..10e785745c5 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Added new `KeyringController:account{Added,Removed}` events ([#7328](https://github.com/MetaMask/core/pull/7328)) - Added new `KeyringBuilder` type ([#7334](https://github.com/MetaMask/core/pull/7334)) - Added an action to call `removeAccount` ([#7241](https://github.com/MetaMask/core/pull/7241)) - This action is meant to be consumed by the `MultichainAccountService` to encapsulate the act of removing a wallet when seed phrase backup fails in the clients. diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 174837a44e1..50e8d5bb5fa 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -1246,16 +1246,109 @@ describe('KeyringController', () => { }); describe('persistAllKeyrings', () => { + const getPrimaryKeyringFrom = ( + controller: KeyringController, + ): EthKeyring => { + return controller.getKeyringsByType(KeyringTypes.hd)[0] as EthKeyring; + }; + it('should reflect changes made directly to a keyring into the KeyringController state', async () => { await withController(async ({ controller }) => { - const primaryKeyring = controller.getKeyringsByType( - KeyringTypes.hd, - )[0] as EthKeyring; + const primaryKeyring = getPrimaryKeyringFrom(controller); + const [addedAccount] = await primaryKeyring.addAccounts(1); + + await controller.persistAllKeyrings(); + + expect(controller.state.keyrings[0].accounts[1]).toBe(addedAccount); + }); + }); + + it('should fire `:accountAdded` if a keyring got updated outside of a withKeyring call', async () => { + await withController(async ({ controller, messenger }) => { + const primaryKeyring = getPrimaryKeyringFrom(controller); const [addedAccount] = await primaryKeyring.addAccounts(1); + const mockAccountAdded = jest.fn(); + messenger.subscribe('KeyringController:accountAdded', mockAccountAdded); + await controller.persistAllKeyrings(); + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountAdded).toHaveBeenCalledWith( + addedAccount, + keyringObject, + ); + }); + }); + + it('should fire `:accountRemoved` if a keyring got updated outside of a withKeyring call', async () => { + await withController(async ({ controller, messenger }) => { + const primaryKeyring = getPrimaryKeyringFrom(controller); + + const [addedAccount] = await primaryKeyring.addAccounts(1); + await controller.persistAllKeyrings(); expect(controller.state.keyrings[0].accounts[1]).toBe(addedAccount); + + const mockAccountRemoved = jest.fn(); + messenger.subscribe( + 'KeyringController:accountRemoved', + mockAccountRemoved, + ); + + primaryKeyring.removeAccount?.(addedAccount); + await controller.persistAllKeyrings(); + + const removedAccount = addedAccount; + expect(mockAccountRemoved).toHaveBeenCalledWith(removedAccount); + }); + }); + + it('should fire `:accountAdded` and `:accountRemoved` if multiple operations get executed', async () => { + await withController(async ({ controller, messenger }) => { + const primaryKeyring = getPrimaryKeyringFrom(controller); + + const [addedAccount] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => await keyring.addAccounts(1), + ); + expect(addedAccount).toBeDefined(); + + const mockAccountAdded = jest.fn(); + const mockAccountRemoved = jest.fn(); + messenger.subscribe('KeyringController:accountAdded', mockAccountAdded); + messenger.subscribe( + 'KeyringController:accountRemoved', + mockAccountRemoved, + ); + + const removedAccount = addedAccount; + + // We create more than 1 account, otherwise the serialized state would be the same between + // the old state and new state, thus, preventing the vault update. + const [addedAccount2, addedAccount3] = + await primaryKeyring.addAccounts(2); + + // We shouldn't be allowed to remove accounts out-of-order with HD keyrings, but that's ok + // for test purposes. + primaryKeyring.removeAccount?.(removedAccount); + + // Now trigger persistence and vault update. + await controller.persistAllKeyrings(); + + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountRemoved).toHaveBeenCalledWith(removedAccount); + expect(mockAccountAdded).toHaveBeenNthCalledWith( + 1, + addedAccount2, + keyringObject, + ); + expect(mockAccountAdded).toHaveBeenNthCalledWith( + 2, + addedAccount3, + keyringObject, + ); }); }); @@ -3426,6 +3519,125 @@ describe('KeyringController', () => { }); }); + it('should fire `:accountAdded` if an account gets added', async () => { + await withController(async ({ controller, messenger }) => { + const mockAccountAdded = jest.fn(); + messenger.subscribe('KeyringController:accountAdded', mockAccountAdded); + + const [addedAccount] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => await keyring.addAccounts(1), + ); + expect(addedAccount).toBeDefined(); + + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountAdded).toHaveBeenCalledWith( + addedAccount, + keyringObject, + ); + }); + }); + + it('should fire multiple `:accountAdded` if multiple accounts get added', async () => { + await withController(async ({ controller, messenger }) => { + const mockAccountAdded = jest.fn(); + messenger.subscribe('KeyringController:accountAdded', mockAccountAdded); + + const [addedAccount1, addedAccount2] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => await keyring.addAccounts(2), + ); + expect(addedAccount1).toBeDefined(); + expect(addedAccount2).toBeDefined(); + + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountAdded).toHaveBeenCalledWith( + addedAccount1, + keyringObject, + ); + expect(mockAccountAdded).toHaveBeenCalledWith( + addedAccount2, + keyringObject, + ); + }); + }); + + it('should fire `:accountRemoved` if an account gets removed', async () => { + await withController(async ({ controller, messenger }) => { + const mockAccountRemoved = jest.fn(); + messenger.subscribe( + 'KeyringController:accountRemoved', + mockAccountRemoved, + ); + + const [addedAccount] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => await keyring.addAccounts(1), + ); + expect(addedAccount).toBeDefined(); + + await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => keyring.removeAccount?.(addedAccount), + ); + + const removedAccount = addedAccount; + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountRemoved).toHaveBeenCalledWith(removedAccount); + }); + }); + + it('should fire `:accountAdded` and `:accountRemoved` if multiple operations get executed', async () => { + await withController(async ({ controller, messenger }) => { + const [addedAccount] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => await keyring.addAccounts(1), + ); + expect(addedAccount).toBeDefined(); + + const mockAccountAdded = jest.fn(); + const mockAccountRemoved = jest.fn(); + messenger.subscribe('KeyringController:accountAdded', mockAccountAdded); + messenger.subscribe( + 'KeyringController:accountRemoved', + mockAccountRemoved, + ); + + const removedAccount = addedAccount; + const [addedAccount2, addedAccount3] = await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => { + // We create more than 1 account, otherwise the serialized state would be the same between + // the old state and new state, thus, preventing the vault update. + const addedAccounts = await keyring.addAccounts(2); + + // We shouldn't be allowed to remove accounts out-of-order with HD keyrings, but that's ok + // for test purposes. + keyring.removeAccount?.(removedAccount); + + return addedAccounts; + }, + ); + + const keyringObject = controller.state.keyrings[0]; + expect(keyringObject).toBeDefined(); + expect(mockAccountRemoved).toHaveBeenCalledWith(removedAccount); + expect(mockAccountAdded).toHaveBeenNthCalledWith( + 1, + addedAccount2, + keyringObject, + ); + expect(mockAccountAdded).toHaveBeenNthCalledWith( + 2, + addedAccount3, + keyringObject, + ); + }); + }); + describe('when the keyring is selected by type', () => { it('should call the given function with the selected keyring', async () => { await withController(async ({ controller }) => { diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 2b148798f6c..800ffad8dc8 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -213,6 +213,11 @@ export type KeyringControllerStateChangeEvent = { payload: [KeyringControllerState, Patch[]]; }; +export type KeyringControllerAccountAddedEvent = { + type: `${typeof name}:accountAdded`; + payload: [string, KeyringObject]; +}; + export type KeyringControllerAccountRemovedEvent = { type: `${typeof name}:accountRemoved`; payload: [string]; @@ -254,6 +259,7 @@ export type KeyringControllerEvents = | KeyringControllerStateChangeEvent | KeyringControllerLockEvent | KeyringControllerUnlockEvent + | KeyringControllerAccountAddedEvent | KeyringControllerAccountRemovedEvent; export type KeyringControllerMessenger = Messenger< @@ -301,11 +307,11 @@ export type KeyringObject = { */ export type KeyringMetadata = { /** - * Keyring ID + * Keyring ID. */ id: string; /** - * Keyring name + * Keyring name. */ name: string; }; @@ -691,6 +697,15 @@ function normalize(address: string): string | undefined { return isEthAddress(address) ? ethNormalize(address) : address; } +/** + * Local type used when override `BaseController` methods. + */ +type KeyringBaseController = BaseController< + typeof name, + KeyringControllerState, + KeyringControllerMessenger +>; + /** * Controller responsible for establishing and managing user identity. * @@ -1245,7 +1260,7 @@ export class KeyringController< } }); - this.messenger.publish(`${name}:accountRemoved`, address); + // `:accountRemoved` event is automatically emitted during `update` calls. } /** @@ -2241,8 +2256,8 @@ export class KeyringController< * * @returns A promise resolving to `true` if the operation is successful. */ - #updateVault(): Promise { - return this.#withVaultLock(async () => { + async #updateVault(): Promise { + return this.#withVaultLock(async (): Promise => { // Ensure no duplicate accounts are persisted. await this.#assertNoDuplicateAccounts(); @@ -2290,6 +2305,63 @@ export class KeyringController< }); } + /** + * Overriden `BaseController.update` method to automatically emit events + * when accounts are added or removed from the keyrings. + * + * @param callback - The state update callback. + * @returns The return value of the `super.update` call. + */ + override update( + callback: Parameters[0], + ): ReturnType { + const toAccountsMap = ( + keyrings: KeyringObject[], + ): Map => { + const accounts = new Map(); + for (const keyring of keyrings) { + for (const account of keyring.accounts) { + accounts.set(account, keyring); + } + } + + return accounts; + }; + + // Keep track of the old keyring states so we can make a diff of added/removed accounts. + const oldKeyrings = this.state.keyrings; + const result = super.update(callback); // This is the real update call. + const newKeyrings = this.state.keyrings; + + const oldAccounts = toAccountsMap(oldKeyrings); + const newAccounts = toAccountsMap(newKeyrings); + + for (const newAccount of newAccounts.keys()) { + if (oldAccounts.has(newAccount)) { + // We remove accounts that intersects, this way we'll now what are the removed + // accounts (from `oldAccounts`) and newly added accounts (from `newAccounts`). + oldAccounts.delete(newAccount); + newAccounts.delete(newAccount); + } + } + + // Those accounts got removed, since they are not part of the new set of accounts. + oldAccounts.forEach((_, oldAccount) => + this.messenger.publish('KeyringController:accountRemoved', oldAccount), + ); + + // Those accounts got added, since they were not part of the old set of accounts. + newAccounts.forEach((keyringObject, newAccount) => + this.messenger.publish( + 'KeyringController:accountAdded', + newAccount, + keyringObject, + ), + ); + + return result; + } + /** * Check if there are new encryption parameters available. *