From 2ed2ddec6aeb400f762dd74b100daefd5c6cfe1f Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 2 Jul 2024 22:03:33 +0800 Subject: [PATCH 1/9] fix: publish account change event at end of keyring state change --- .../src/AccountsController.ts | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index ccc6c76f0f4..f6ac485fc44 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); } /** @@ -759,6 +749,8 @@ export class AccountsController extends BaseController< }, ); currentState.internalAccounts.selectedAccount = accountToSelect.id; + + this.#publishAccountChangeEvent(accountToSelect); } }); } @@ -954,6 +946,19 @@ export class AccountsController extends BaseController< 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. From e1038507a8c56e3e4e0d8acca22fbba6bb1fd747 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 2 Jul 2024 22:04:37 +0800 Subject: [PATCH 2/9] fix: update last selected for inital account --- packages/accounts-controller/src/AccountsController.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index f6ac485fc44..2049c77a3cb 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -749,6 +749,9 @@ export class AccountsController extends BaseController< }, ); currentState.internalAccounts.selectedAccount = accountToSelect.id; + currentState.internalAccounts.accounts[ + accountToSelect.id + ].metadata.lastSelected = Date.now(); this.#publishAccountChangeEvent(accountToSelect); } @@ -927,6 +930,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, From 7e8050f5935a5b6956063c3e7018fe06d4c52e0a Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 2 Jul 2024 22:05:57 +0800 Subject: [PATCH 3/9] fix: use listMultichain accounts --- .../src/AccountsController.ts | 49 ++++++++++++------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 2049c77a3cb..d5f1ff19da2 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -395,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, @@ -409,6 +409,7 @@ export class AccountsController extends BaseController< ...account, metadata: { ...account.metadata, name: accountName }, }; + // @ts-ignore currentState.internalAccounts.accounts[accountId] = internalAccount; }); } @@ -629,7 +630,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); @@ -767,7 +768,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, ); @@ -794,21 +795,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; + }, + ); } /** @@ -850,6 +853,9 @@ export class AccountsController extends BaseController< keyringType, accounts, ); + console.log('accounts', accounts); + console.log('keyringType', keyringType); + console.log('keyringAccounts', keyringAccounts); const lastDefaultIndexUsedForKeyringType = keyringAccounts.reduce( (maxInternalAccountIndex, internalAccount) => { // We **DO NOT USE** `\d+` here to only consider valid "human" @@ -944,7 +950,7 @@ export class AccountsController extends BaseController< ...newAccount.metadata, name: accountName, importTime: Date.now(), - lastSelected: 0, + lastSelected: isFirstAccount ? Date.now() : 0, }, }; @@ -952,6 +958,9 @@ export class AccountsController extends BaseController< } #publishAccountChangeEvent(account: InternalAccount) { + console.log('publishing new account'); + console.log('previosu', this.state.internalAccounts.selectedAccount); + console.log('new', account); if (isEvmAccountType(account.type)) { this.messagingSystem.publish( 'AccountsController:selectedEvmAccountChange', @@ -982,6 +991,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. @@ -989,8 +999,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; } From 1bd042ef6fadc3fe5aa10ea39a9d565368f08e23 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 2 Jul 2024 22:24:00 +0800 Subject: [PATCH 4/9] fix: update tests --- .../src/AccountsController.test.ts | 108 ++++++++++++++---- 1 file changed, 83 insertions(+), 25 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index cf8a1b1a4d9..d06cdb83e41 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 listMultichainAccount', async () => { + const messenger = buildMessenger(); + mockUUID + .mockReturnValueOnce('mock-id') // call to check if its a new account + .mockReturnValueOnce('mock-id2'); // call to add account + + const mockNewKeyringState = { + isUnlocked: true, + keyrings: [ + { + type: KeyringTypes.hd, + accounts: [mockAccount.address], + }, + ], + }; + + const { accountsController } = setupAccountsController({ + initialState: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + }, + messenger, + }); + + const listMultichainAccountsSpy = jest.spyOn( + accountsController, + 'listMultichainAccounts', + ); + + 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), From 8ab6834be0e35243262ceb15993430ebd8257d05 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 2 Jul 2024 23:28:12 +0800 Subject: [PATCH 5/9] fix: remove log --- packages/accounts-controller/src/AccountsController.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index d5f1ff19da2..e74af2af727 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -853,9 +853,6 @@ export class AccountsController extends BaseController< keyringType, accounts, ); - console.log('accounts', accounts); - console.log('keyringType', keyringType); - console.log('keyringAccounts', keyringAccounts); const lastDefaultIndexUsedForKeyringType = keyringAccounts.reduce( (maxInternalAccountIndex, internalAccount) => { // We **DO NOT USE** `\d+` here to only consider valid "human" @@ -958,9 +955,6 @@ export class AccountsController extends BaseController< } #publishAccountChangeEvent(account: InternalAccount) { - console.log('publishing new account'); - console.log('previosu', this.state.internalAccounts.selectedAccount); - console.log('new', account); if (isEvmAccountType(account.type)) { this.messagingSystem.publish( 'AccountsController:selectedEvmAccountChange', From ca4ed28f56b11b8cd9fe4dc1d7a376cbcaae7ed6 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 2 Jul 2024 23:44:37 +0800 Subject: [PATCH 6/9] fix: remove ignore --- packages/accounts-controller/src/AccountsController.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index e74af2af727..a426c82d43e 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -409,7 +409,6 @@ export class AccountsController extends BaseController< ...account, metadata: { ...account.metadata, name: accountName }, }; - // @ts-ignore currentState.internalAccounts.accounts[accountId] = internalAccount; }); } From 24874f526a0e14184c1c9a533f3dbf77571ed305 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 3 Jul 2024 16:56:55 +0800 Subject: [PATCH 7/9] Update packages/accounts-controller/src/AccountsController.test.ts Co-authored-by: Charly Chevalier --- packages/accounts-controller/src/AccountsController.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index d06cdb83e41..f54a8975853 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -445,7 +445,7 @@ describe('AccountsController', () => { }); describe('onKeyringStateChange', () => { - it('uses listMultichainAccount', async () => { + it('uses listMultichainAccounts', async () => { const messenger = buildMessenger(); mockUUID .mockReturnValueOnce('mock-id') // call to check if its a new account From a3fbf300ae8d788917de2fcacd4c0f3e1850c3b3 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 3 Jul 2024 17:42:33 +0800 Subject: [PATCH 8/9] fix: move mockState closer to publish --- .../src/AccountsController.test.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index f54a8975853..8a48aee9aa9 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -451,16 +451,6 @@ describe('AccountsController', () => { .mockReturnValueOnce('mock-id') // call to check if its a new account .mockReturnValueOnce('mock-id2'); // call to add account - const mockNewKeyringState = { - isUnlocked: true, - keyrings: [ - { - type: KeyringTypes.hd, - accounts: [mockAccount.address], - }, - ], - }; - const { accountsController } = setupAccountsController({ initialState: { internalAccounts: { @@ -476,6 +466,16 @@ describe('AccountsController', () => { 'listMultichainAccounts', ); + const mockNewKeyringState = { + isUnlocked: true, + keyrings: [ + { + type: KeyringTypes.hd, + accounts: [mockAccount.address], + }, + ], + }; + messenger.publish( 'KeyringController:stateChange', mockNewKeyringState, From 48d56062794d26a2019aa66371f616e1916af33d Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 3 Jul 2024 17:52:02 +0800 Subject: [PATCH 9/9] refactor: Date.now() in lastSelected to a helper to add context --- .../accounts-controller/src/AccountsController.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index a426c82d43e..34444da995c 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -751,7 +751,7 @@ export class AccountsController extends BaseController< currentState.internalAccounts.selectedAccount = accountToSelect.id; currentState.internalAccounts.accounts[ accountToSelect.id - ].metadata.lastSelected = Date.now(); + ].metadata.lastSelected = this.#getLastSelectedIndex(); this.#publishAccountChangeEvent(accountToSelect); } @@ -898,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. @@ -946,7 +957,7 @@ export class AccountsController extends BaseController< ...newAccount.metadata, name: accountName, importTime: Date.now(), - lastSelected: isFirstAccount ? Date.now() : 0, + lastSelected: isFirstAccount ? this.#getLastSelectedIndex() : 0, }, };