From cb11300459e08e7fe32d07bb757edd0f14837739 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 3 Jul 2024 19:10:35 +0800 Subject: [PATCH] fix: use `listMultichainAccounts` and set `lastSelected` for initial account (#4494) ## Explanation This PR fixes the following: 1. Emits `selectedAccountChange` for initial account 2. Updates `lastSelected` for initial account 3. Uses `listMultichainAccounts` instead of `listAccounts` for multichain methods ## References Fixes https://github.com/MetaMask/accounts-planning/issues/507 ## Changelog ### `@metamask/accounts-controller` - **FIXED**: Emits `selectedAccountChange` and updates last selected for initial account. - **FIXED**: Uses `listMultichainAccounts` instead of `listAccounts` for multichain methods ## Checklist - [x] 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 --------- Co-authored-by: Charly Chevalier --- .../src/AccountsController.test.ts | 108 ++++++++++++++---- .../src/AccountsController.ts | 85 +++++++++----- 2 files changed, 138 insertions(+), 55 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index cf8a1b1a4d..8a48aee9aa 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -445,6 +445,45 @@ describe('AccountsController', () => { }); describe('onKeyringStateChange', () => { + it('uses listMultichainAccounts', async () => { + const messenger = buildMessenger(); + mockUUID + .mockReturnValueOnce('mock-id') // call to check if its a new account + .mockReturnValueOnce('mock-id2'); // call to add account + + 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], + }, + ], + }; + + messenger.publish( + 'KeyringController:stateChange', + mockNewKeyringState, + [], + ); + + expect(listMultichainAccountsSpy).toHaveBeenCalled(); + }); it('not update state when only keyring is unlocked without any keyrings', async () => { const messenger = buildMessenger(); const { accountsController } = setupAccountsController({ @@ -463,7 +502,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([]); }); @@ -496,7 +535,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([]); }); @@ -537,7 +576,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([ mockAccount, @@ -595,7 +634,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([ mockAccount, @@ -661,7 +700,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([ mockAccount, @@ -709,7 +748,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([ mockAccount, @@ -774,7 +813,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts.map(setLastSelectedAsAny)).toStrictEqual([ mockAccount, @@ -872,7 +911,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([ mockAccount, @@ -915,7 +954,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([setLastSelectedAsAny(mockAccount2)]); expect(accountsController.getSelectedAccount()).toStrictEqual( @@ -968,7 +1007,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([ setLastSelectedAsAny(mockAccount), @@ -1031,7 +1070,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([ setLastSelectedAsAny(mockAccount), @@ -1043,6 +1082,7 @@ describe('AccountsController', () => { }); it('delete the account and select the account with the most recent lastSelected', async () => { + const currentTime = Date.now(); const messenger = buildMessenger(); mockUUID.mockReturnValueOnce('mock-id').mockReturnValueOnce('mock-id2'); @@ -1097,14 +1137,22 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([ - mockAccountWithoutLastSelected, + { + ...mockAccountWithoutLastSelected, + metadata: { + ...mockAccountWithoutLastSelected.metadata, + lastSelected: expect.any(Number), + }, + }, mockAccount2WithoutLastSelected, ]); - expect(accountsController.getSelectedAccount()).toStrictEqual( - mockAccountWithoutLastSelected, + + const selectedAccount = accountsController.getSelectedAccount(); + expect(selectedAccount.metadata.lastSelected).toBeGreaterThanOrEqual( + currentTime, ); }); }); @@ -1155,7 +1203,7 @@ describe('AccountsController', () => { ); const selectedAccount = accountsController.getSelectedAccount(); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); const expectedAccount = setLastSelectedAsAny(mockReinitialisedAccount); expect(selectedAccount).toStrictEqual(expectedAccount); @@ -1340,7 +1388,9 @@ describe('AccountsController', () => { await accountsController.updateAccounts(); - expect(accountsController.listAccounts()).toStrictEqual(expectedAccounts); + expect(accountsController.listMultichainAccounts()).toStrictEqual( + expectedAccounts, + ); }); it('update accounts with Snap accounts when snap keyring is defined and has accounts', async () => { @@ -1395,7 +1445,7 @@ describe('AccountsController', () => { await accountsController.updateAccounts(); expect( - accountsController.listAccounts().map(setLastSelectedAsAny), + accountsController.listMultichainAccounts().map(setLastSelectedAsAny), ).toStrictEqual(expectedAccounts); }); @@ -1425,7 +1475,9 @@ describe('AccountsController', () => { await accountsController.updateAccounts(); - expect(accountsController.listAccounts()).toStrictEqual(expectedAccounts); + expect(accountsController.listMultichainAccounts()).toStrictEqual( + expectedAccounts, + ); }); it('set the account with the correct index', async () => { @@ -1474,7 +1526,9 @@ describe('AccountsController', () => { await accountsController.updateAccounts(); - expect(accountsController.listAccounts()).toStrictEqual(expectedAccounts); + expect(accountsController.listMultichainAccounts()).toStrictEqual( + expectedAccounts, + ); }); it('filter Snap accounts from normalAccounts', async () => { @@ -1529,7 +1583,9 @@ describe('AccountsController', () => { await accountsController.updateAccounts(); - expect(accountsController.listAccounts()).toStrictEqual(expectedAccounts); + expect(accountsController.listMultichainAccounts()).toStrictEqual( + expectedAccounts, + ); }); it('filter Snap accounts from normalAccounts even if the snap account is listed before normal accounts', async () => { @@ -1585,7 +1641,9 @@ describe('AccountsController', () => { await accountsController.updateAccounts(); - expect(accountsController.listAccounts()).toStrictEqual(expectedAccounts); + expect(accountsController.listMultichainAccounts()).toStrictEqual( + expectedAccounts, + ); }); it.each([ @@ -1641,7 +1699,7 @@ describe('AccountsController', () => { await accountsController.updateAccounts(); expect( - accountsController.listAccounts().map(setLastSelectedAsAny), + accountsController.listMultichainAccounts().map(setLastSelectedAsAny), ).toStrictEqual(expectedAccounts); }); @@ -2253,7 +2311,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([ mockAccount, setLastSelectedAsAny(mockSimpleKeyring1), @@ -2310,7 +2368,7 @@ describe('AccountsController', () => { [], ); - const accounts = accountsController.listAccounts(); + const accounts = accountsController.listMultichainAccounts(); expect(accounts).toStrictEqual([ mockAccount, setLastSelectedAsAny(mockSimpleKeyring2), diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index ccc6c76f0f..34444da995 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -381,17 +381,7 @@ export class AccountsController extends BaseController< currentState.internalAccounts.selectedAccount = account.id; }); - if (isEvmAccountType(account.type)) { - this.messagingSystem.publish( - 'AccountsController:selectedEvmAccountChange', - account, - ); - } - - this.messagingSystem.publish( - 'AccountsController:selectedAccountChange', - account, - ); + this.#publishAccountChangeEvent(account); } /** @@ -405,7 +395,7 @@ export class AccountsController extends BaseController< const account = this.getAccountExpect(accountId); if ( - this.listAccounts().find( + this.listMultichainAccounts().find( (internalAccount) => internalAccount.metadata.name === accountName && internalAccount.id !== accountId, @@ -639,7 +629,7 @@ export class AccountsController extends BaseController< } const { previousNormalInternalAccounts, previousSnapInternalAccounts } = - this.listAccounts().reduce( + this.listMultichainAccounts().reduce( (accumulator, account) => { if (account.metadata.keyring.type === KeyringTypes.snap) { accumulator.previousSnapInternalAccounts.push(account); @@ -759,6 +749,11 @@ export class AccountsController extends BaseController< }, ); currentState.internalAccounts.selectedAccount = accountToSelect.id; + currentState.internalAccounts.accounts[ + accountToSelect.id + ].metadata.lastSelected = this.#getLastSelectedIndex(); + + this.#publishAccountChangeEvent(accountToSelect); } }); } @@ -772,7 +767,7 @@ export class AccountsController extends BaseController< #handleOnSnapStateChange(snapState: SnapControllerState) { // only check if snaps changed in status const { snaps } = snapState; - const accounts = this.listAccounts().filter( + const accounts = this.listMultichainAccounts().filter( (account) => account.metadata.snap, ); @@ -799,21 +794,23 @@ export class AccountsController extends BaseController< * @returns The list of accounts associcated with this keyring type. */ #getAccountsByKeyringType(keyringType: string, accounts?: InternalAccount[]) { - return (accounts ?? this.listAccounts()).filter((internalAccount) => { - // We do consider `hd` and `simple` keyrings to be of same type. So we check those 2 types - // to group those accounts together! - if ( - keyringType === KeyringTypes.hd || - keyringType === KeyringTypes.simple - ) { - return ( - internalAccount.metadata.keyring.type === KeyringTypes.hd || - internalAccount.metadata.keyring.type === KeyringTypes.simple - ); - } + return (accounts ?? this.listMultichainAccounts()).filter( + (internalAccount) => { + // We do consider `hd` and `simple` keyrings to be of same type. So we check those 2 types + // to group those accounts together! + if ( + keyringType === KeyringTypes.hd || + keyringType === KeyringTypes.simple + ) { + return ( + internalAccount.metadata.keyring.type === KeyringTypes.hd || + internalAccount.metadata.keyring.type === KeyringTypes.simple + ); + } - return internalAccount.metadata.keyring.type === keyringType; - }); + return internalAccount.metadata.keyring.type === keyringType; + }, + ); } /** @@ -901,6 +898,17 @@ export class AccountsController extends BaseController< return account.type.startsWith(parseCaipChainId(chainId).namespace); } + /** + * Retrieves the index value for `metadata.lastSelected`. + * + * @returns The index value. + */ + #getLastSelectedIndex() { + // NOTE: For now we use the current date, since we know this value + // will always be higher than any already selected account index. + return Date.now(); + } + /** * Handles the addition of a new account to the controller. * If the account is not a Snap Keyring account, generates an internal account for it and adds it to the controller. @@ -935,6 +943,8 @@ export class AccountsController extends BaseController< } } + const isFirstAccount = Object.keys(accountsState).length === 0; + // Get next account name available for this given keyring const accountName = this.getNextAvailableAccountName( newAccount.metadata.keyring.type, @@ -947,13 +957,26 @@ export class AccountsController extends BaseController< ...newAccount.metadata, name: accountName, importTime: Date.now(), - lastSelected: 0, + lastSelected: isFirstAccount ? this.#getLastSelectedIndex() : 0, }, }; return accountsState; } + #publishAccountChangeEvent(account: InternalAccount) { + if (isEvmAccountType(account.type)) { + this.messagingSystem.publish( + 'AccountsController:selectedEvmAccountChange', + account, + ); + } + this.messagingSystem.publish( + 'AccountsController:selectedAccountChange', + account, + ); + } + /** * Handles the removal of an account from the internal accounts list. * @param accountsState - AccountsController accounts state that is to be mutated. @@ -972,6 +995,7 @@ export class AccountsController extends BaseController< * Retrieves the value of a specific metadata key for an existing account. * @param accountId - The ID of the account. * @param metadataKey - The key of the metadata to retrieve. + * @param account - The account object to retrieve the metadata key from. * @returns The value of the specified metadata key, or undefined if the account or metadata key does not exist. */ // TODO: Either fix this lint violation or explain why it's necessary to ignore. @@ -979,8 +1003,9 @@ export class AccountsController extends BaseController< #populateExistingMetadata( accountId: string, metadataKey: T, + account?: InternalAccount, ): InternalAccount['metadata'][T] | undefined { - const internalAccount = this.getAccount(accountId); + const internalAccount = account ?? this.getAccount(accountId); return internalAccount ? internalAccount.metadata[metadataKey] : undefined; }