From f29013cf7d71441a6bf9822dc7637d59888a0fcc Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 4 Jun 2024 12:03:31 +0800 Subject: [PATCH 01/10] fix: set account when deleting selected account --- .../src/AccountsController.ts | 73 ++++++++++++++----- 1 file changed, 54 insertions(+), 19 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 820ab6447f7..d7995092c1f 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -436,7 +436,7 @@ export class AccountsController extends BaseController< const keyringTypeName = keyringTypeToName( internalAccount.metadata.keyring.type, ); - const keyringAccountIndex = keyringTypes.get(keyringTypeName) ?? 0; + const keyringAccountIndex = keyringTypes.get(keyringTypeName) || 0; if (keyringAccountIndex) { keyringTypes.set(keyringTypeName, keyringAccountIndex + 1); } else { @@ -451,16 +451,16 @@ export class AccountsController extends BaseController< metadata: { ...internalAccount.metadata, name: - this.#populateExistingMetadata(existingAccount?.id, 'name') ?? + this.#populateExistingMetadata(existingAccount?.id, 'name') || `${keyringTypeName} ${keyringAccountIndex + 1}`, importTime: - this.#populateExistingMetadata(existingAccount?.id, 'importTime') ?? + this.#populateExistingMetadata(existingAccount?.id, 'importTime') || Date.now(), lastSelected: this.#populateExistingMetadata( existingAccount?.id, 'lastSelected', - ) ?? 0, + ) || 0, }, }; @@ -588,10 +588,10 @@ export class AccountsController extends BaseController< ], type: EthAccountType.Eoa, metadata: { - name: this.#populateExistingMetadata(id, 'name') ?? '', + name: this.#populateExistingMetadata(id, 'name') || '', importTime: - this.#populateExistingMetadata(id, 'importTime') ?? Date.now(), - lastSelected: this.#populateExistingMetadata(id, 'lastSelected') ?? 0, + this.#populateExistingMetadata(id, 'importTime') || Date.now(), + lastSelected: this.#populateExistingMetadata(id, 'lastSelected') || 0, keyring: { type: (keyring as Keyring).type, }, @@ -725,25 +725,25 @@ export class AccountsController extends BaseController< // handle if the selected account was deleted if (!this.getAccount(this.state.internalAccounts.selectedAccount)) { - const [accountToSelect] = this.listAccounts().sort( - (accountA, accountB) => { - // sort by lastSelected descending - return ( - (accountB.metadata.lastSelected ?? 0) - - (accountA.metadata.lastSelected ?? 0) - ); - }, - ); - // if the accountToSelect is undefined, then there are no accounts // it mean the keyring was reinitialized. - if (!accountToSelect) { + if (this.listAccounts().length === 0) { this.update((currentState: Draft) => { currentState.internalAccounts.selectedAccount = ''; }); return; } + const [accountToSelect] = this.listAccounts().sort( + (accountA, accountB) => { + // sort by lastSelected descending + return ( + (accountB.metadata.lastSelected || 0) - + (accountA.metadata.lastSelected || 0) + ); + }, + ); + this.setSelectedAccount(accountToSelect.id); } } @@ -937,8 +937,43 @@ export class AccountsController extends BaseController< * @param accountId - The ID of the account to be removed. */ #handleAccountRemoved(accountId: string) { + const accountToBeRemovedIsSelected = + accountId === this.state.internalAccounts.selectedAccount; + let newAccountToBeSelected: InternalAccount | undefined; + + // handle the case where the account to be removed is the selected account + if (accountToBeRemovedIsSelected) { + [newAccountToBeSelected] = this.listAccounts() + .filter((account) => account.id !== accountId) + .sort((accountA, accountB) => { + // sort by lastSelected descending + return ( + (accountB.metadata.lastSelected || 0) - + (accountA.metadata.lastSelected || 0) + ); + }); + } + + // we perform the update in the same step instead of using setSelectedAccount this.update((currentState: Draft) => { - delete currentState.internalAccounts.accounts[accountId]; + const updatedState = deepCloneDraft(currentState); + + delete updatedState.internalAccounts.accounts[accountId]; + + if (accountToBeRemovedIsSelected) { + if (!newAccountToBeSelected) { + // if there are no accounts left, set the selected account to an empty string which is the default state + updatedState.internalAccounts.selectedAccount = ''; + } else { + updatedState.internalAccounts.selectedAccount = + newAccountToBeSelected.id; + updatedState.internalAccounts.accounts[ + newAccountToBeSelected.id + ].metadata.lastSelected = Date.now(); + } + } + + return updatedState; }); } From 119613091628c45285bfca88f94c13c2ddb796db Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 4 Jun 2024 15:31:20 +0800 Subject: [PATCH 02/10] fix: set new selected account and selectedAccount being deleted in the same update --- .../src/AccountsController.test.ts | 16 +++++----------- .../src/AccountsController.ts | 5 +++-- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 930a569cd4a..66866da039a 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -1037,16 +1037,10 @@ describe('AccountsController', () => { initialState: { internalAccounts: { accounts: { - 'missing-account': { - id: 'missing-account', + 'missing-account': createMockInternalAccount({ address: '0x999', - metadata: { - keyring: { - type: KeyringTypes.hd, - }, - }, - [mockAccount2.id]: mockAccount2WithoutLastSelected, - } as unknown as InternalAccount, + id: 'missing-account', + }), [mockAccount.id]: mockAccountWithoutLastSelected, [mockAccount2.id]: mockAccount2WithoutLastSelected, }, @@ -1066,10 +1060,10 @@ describe('AccountsController', () => { expect(accounts).toStrictEqual([ mockAccountWithoutLastSelected, - mockAccount2WithoutLastSelected, + setLastSelectedAsAny(mockAccount2WithoutLastSelected), ]); expect(accountsController.getSelectedAccount()).toStrictEqual( - mockAccountWithoutLastSelected, + setLastSelectedAsAny(mockAccount2WithoutLastSelected), ); }); }); diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index d7995092c1f..50dd911ab0f 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -943,7 +943,7 @@ export class AccountsController extends BaseController< // handle the case where the account to be removed is the selected account if (accountToBeRemovedIsSelected) { - [newAccountToBeSelected] = this.listAccounts() + newAccountToBeSelected = this.listAccounts() .filter((account) => account.id !== accountId) .sort((accountA, accountB) => { // sort by lastSelected descending @@ -951,7 +951,8 @@ export class AccountsController extends BaseController< (accountB.metadata.lastSelected || 0) - (accountA.metadata.lastSelected || 0) ); - }); + }) + .pop(); } // we perform the update in the same step instead of using setSelectedAccount From c439acc544902173fbf3b592d005f51f51a54fb5 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 4 Jun 2024 15:33:37 +0800 Subject: [PATCH 03/10] refactor: default behaviour of getSelectedAccount --- .../src/AccountsController.test.ts | 53 +++++++++---------- .../src/AccountsController.ts | 28 +++++----- 2 files changed, 40 insertions(+), 41 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 66866da039a..bdd60e99377 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -1807,6 +1807,32 @@ describe('AccountsController', () => { 'No EVM accounts', ); }); + + it('handle the edge case of undefined accountId during onboarding', async () => { + const { accountsController } = setupAccountsController({ + initialState: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + }, + }); + + expect(accountsController.getSelectedAccount()).toStrictEqual({ + id: '', + address: '', + options: {}, + methods: [], + type: EthAccountType.Eoa, + metadata: { + name: '', + keyring: { + type: '', + }, + importTime: 0, + }, + }); + }); }); describe('getSelectedMultichainAccount', () => { @@ -2017,33 +2043,6 @@ describe('AccountsController', () => { `Account Id "${accountId}" not found`, ); }); - - it('handle the edge case of undefined accountId during onboarding', async () => { - const { accountsController } = setupAccountsController({ - initialState: { - internalAccounts: { - accounts: { [mockAccount.id]: mockAccount }, - selectedAccount: mockAccount.id, - }, - }, - }); - - // @ts-expect-error forcing undefined accountId - expect(accountsController.getAccountExpect(undefined)).toStrictEqual({ - id: '', - address: '', - options: {}, - methods: [], - type: EthAccountType.Eoa, - metadata: { - name: '', - keyring: { - type: '', - }, - importTime: 0, - }, - }); - }); }); describe('setSelectedAccount', () => { diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 50dd911ab0f..3af67820969 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -265,9 +265,22 @@ export class AccountsController extends BaseController< * @throws An error if the account ID is not found. */ getAccountExpect(accountId: string): InternalAccount { + const account = this.getAccount(accountId); + if (account === undefined) { + throw new Error(`Account Id "${accountId}" not found`); + } + return account; + } + + /** + * Returns the last selected evm account. + * + * @returns The selected internal account. + */ + getSelectedAccount(): InternalAccount { // Edge case where the extension is setup but the srp is not yet created // certain ui elements will query the selected address before any accounts are created. - if (!accountId) { + if (this.state.internalAccounts.selectedAccount === '') { return { id: '', address: '', @@ -284,19 +297,6 @@ export class AccountsController extends BaseController< }; } - const account = this.getAccount(accountId); - if (account === undefined) { - throw new Error(`Account Id "${accountId}" not found`); - } - return account; - } - - /** - * Returns the last selected evm account. - * - * @returns The selected internal account. - */ - getSelectedAccount(): InternalAccount { const selectedAccount = this.getAccountExpect( this.state.internalAccounts.selectedAccount, ); From 3f27d6476f26b52f60cd53cfdbd17ea6012cefee Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 19 Jun 2024 01:09:17 +0800 Subject: [PATCH 04/10] refactor: onKeyringState to use one update --- .../src/AccountsController.test.ts | 4 +- .../src/AccountsController.ts | 161 +++++++++--------- 2 files changed, 78 insertions(+), 87 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 47bc407e0b7..c2b30de278b 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -1108,10 +1108,10 @@ describe('AccountsController', () => { expect(accounts).toStrictEqual([ mockAccountWithoutLastSelected, - setLastSelectedAsAny(mockAccount2WithoutLastSelected), + mockAccount2WithoutLastSelected, ]); expect(accountsController.getSelectedAccount()).toStrictEqual( - setLastSelectedAsAny(mockAccount2WithoutLastSelected), + mockAccountWithoutLastSelected, ); }); }); diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 9b6d3c3c783..deee1b556a5 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -24,7 +24,7 @@ import type { SnapStateChange, } from '@metamask/snaps-controllers'; import type { SnapId } from '@metamask/snaps-sdk'; -import type { Snap } from '@metamask/snaps-utils'; +import { deepClone, type Snap } from '@metamask/snaps-utils'; import type { CaipChainId } from '@metamask/utils'; import { type Keyring, @@ -717,41 +717,55 @@ export class AccountsController extends BaseController< } } + let accountsState = deepClone(this.state.internalAccounts.accounts); + if (deletedAccounts.length > 0) { for (const account of deletedAccounts) { - this.#handleAccountRemoved(account.id); + accountsState = this.#handleAccountRemoved(accountsState, account.id); } } if (addedAccounts.length > 0) { for (const account of addedAccounts) { - this.#handleNewAccountAdded(account); + accountsState = this.#handleNewAccountAdded(accountsState, account); } } - // handle if the selected account was deleted - if (!this.getAccount(this.state.internalAccounts.selectedAccount)) { - // if the accountToSelect is undefined, then there are no accounts - // it mean the keyring was reinitialized. - if (this.listAccounts().length === 0) { - this.update((currentState: Draft) => { - currentState.internalAccounts.selectedAccount = ''; - }); - return; - } - - const [accountToSelect] = this.listAccounts().sort( - (accountA, accountB) => { - // sort by lastSelected descending - return ( - (accountB.metadata.lastSelected || 0) - - (accountA.metadata.lastSelected || 0) - ); + this.update((currentState: Draft) => { + const newState: AccountsControllerState = { + internalAccounts: { + accounts: accountsState, + selectedAccount: currentState.internalAccounts.selectedAccount, }, - ); + }; + + const existingAccounts = Object.values(accountsState); + + // handle if the selected account was deleted + if (!accountsState[this.state.internalAccounts.selectedAccount]) { + newState.internalAccounts.accounts = accountsState; + // if the accountToSelect is undefined, then there are no accounts + // it mean the keyring was reinitialized. + if (existingAccounts.length === 0) { + newState.internalAccounts.selectedAccount = ''; + return newState; + } - this.setSelectedAccount(accountToSelect.id); - } + const [accountToSelect] = existingAccounts.sort( + (accountA, accountB) => { + // sort by lastSelected descending + return ( + (accountB.metadata.lastSelected || 0) - + (accountA.metadata.lastSelected || 0) + ); + }, + ); + + newState.internalAccounts.selectedAccount = accountToSelect.id; + } + + return newState; + }); } } @@ -786,10 +800,11 @@ export class AccountsController extends BaseController< /** * Returns the list of accounts for a given keyring type. * @param keyringType - The type of keyring. + * @param accounts - Accounts to filter by keyring type. * @returns The list of accounts associcated with this keyring type. */ - #getAccountsByKeyringType(keyringType: string) { - return this.listAccounts().filter((internalAccount) => { + #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 ( @@ -833,11 +848,18 @@ export class AccountsController extends BaseController< /** * Returns the next account number for a given keyring type. * @param keyringType - The type of keyring. + * @param accounts - Existing accounts to check for the next available account number. * @returns An object containing the account prefix and index to use. */ - getNextAvailableAccountName(keyringType: string = KeyringTypes.hd): string { + getNextAvailableAccountName( + keyringType: string = KeyringTypes.hd, + accounts?: InternalAccount[], + ): string { const keyringName = keyringTypeToName(keyringType); - const keyringAccounts = this.#getAccountsByKeyringType(keyringType); + const keyringAccounts = this.#getAccountsByKeyringType( + keyringType, + accounts, + ); const lastDefaultIndexUsedForKeyringType = keyringAccounts.reduce( (maxInternalAccountIndex, internalAccount) => { // We **DO NOT USE** `\d+` here to only consider valid "human" @@ -888,9 +910,14 @@ export class AccountsController extends BaseController< * 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. * If the account is a Snap Keyring account, retrieves the account from the keyring and adds it to the controller. + * @param accountsState - Accounts Controller accounts state that is to be mutated. * @param account - The address and keyring type object of the new account. + * @returns The updated Accounts Controller accounts state. */ - #handleNewAccountAdded(account: AddressAndKeyringTypeObject) { + #handleNewAccountAdded( + accountsState: AccountsControllerState['internalAccounts']['accounts'], + account: AddressAndKeyringTypeObject, + ): AccountsControllerState['internalAccounts']['accounts'] { let newAccount: InternalAccount; if (account.type !== KeyringTypes.snap) { newAccount = this.#generateInternalAccountForNonSnapAccount( @@ -909,77 +936,41 @@ export class AccountsController extends BaseController< // The snap deleted the account before the keyring controller could add it if (!newAccount) { - return; + return accountsState; } } // Get next account name available for this given keyring const accountName = this.getNextAvailableAccountName( newAccount.metadata.keyring.type, + Object.values(accountsState), ); - this.update((currentState: Draft) => { - // FIXME: deep clone of old state to get around Type instantiation is excessively deep and possibly infinite. - const newState = deepCloneDraft(currentState); - - newState.internalAccounts.accounts[newAccount.id] = { - ...newAccount, - metadata: { - ...newAccount.metadata, - name: accountName, - importTime: Date.now(), - lastSelected: 0, - }, - }; + accountsState[newAccount.id] = { + ...newAccount, + metadata: { + ...newAccount.metadata, + name: accountName, + importTime: Date.now(), + lastSelected: 0, + }, + }; - return newState; - }); + return accountsState; } /** * Handles the removal of an account from the internal accounts list. + * @param accountsState - Accounts Controller accounts state that is to be mutated. * @param accountId - The ID of the account to be removed. + * @returns The updated Accounts Controller state. */ - #handleAccountRemoved(accountId: string) { - const accountToBeRemovedIsSelected = - accountId === this.state.internalAccounts.selectedAccount; - let newAccountToBeSelected: InternalAccount | undefined; - - // handle the case where the account to be removed is the selected account - if (accountToBeRemovedIsSelected) { - newAccountToBeSelected = this.listAccounts() - .filter((account) => account.id !== accountId) - .sort((accountA, accountB) => { - // sort by lastSelected descending - return ( - (accountB.metadata.lastSelected || 0) - - (accountA.metadata.lastSelected || 0) - ); - }) - .pop(); - } - - // we perform the update in the same step instead of using setSelectedAccount - this.update((currentState: Draft) => { - const updatedState = deepCloneDraft(currentState); - - delete updatedState.internalAccounts.accounts[accountId]; - - if (accountToBeRemovedIsSelected) { - if (!newAccountToBeSelected) { - // if there are no accounts left, set the selected account to an empty string which is the default state - updatedState.internalAccounts.selectedAccount = ''; - } else { - updatedState.internalAccounts.selectedAccount = - newAccountToBeSelected.id; - updatedState.internalAccounts.accounts[ - newAccountToBeSelected.id - ].metadata.lastSelected = Date.now(); - } - } - - return updatedState; - }); + #handleAccountRemoved( + accountsState: AccountsControllerState['internalAccounts']['accounts'], + accountId: string, + ): AccountsControllerState['internalAccounts']['accounts'] { + delete accountsState[accountId]; + return accountsState; } /** From f485b6eb798bd0707eb9798eda6042dae66c26b1 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 19 Jun 2024 01:11:20 +0800 Subject: [PATCH 05/10] fix: revert to old ?? --- .../src/AccountsController.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index deee1b556a5..4971a16f0f1 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -442,7 +442,7 @@ export class AccountsController extends BaseController< const keyringTypeName = keyringTypeToName( internalAccount.metadata.keyring.type, ); - const keyringAccountIndex = keyringTypes.get(keyringTypeName) || 0; + const keyringAccountIndex = keyringTypes.get(keyringTypeName) ?? 0; if (keyringAccountIndex) { keyringTypes.set(keyringTypeName, keyringAccountIndex + 1); } else { @@ -457,16 +457,16 @@ export class AccountsController extends BaseController< metadata: { ...internalAccount.metadata, name: - this.#populateExistingMetadata(existingAccount?.id, 'name') || + this.#populateExistingMetadata(existingAccount?.id, 'name') ?? `${keyringTypeName} ${keyringAccountIndex + 1}`, importTime: - this.#populateExistingMetadata(existingAccount?.id, 'importTime') || + this.#populateExistingMetadata(existingAccount?.id, 'importTime') ?? Date.now(), lastSelected: this.#populateExistingMetadata( existingAccount?.id, 'lastSelected', - ) || 0, + ) ?? 0, }, }; @@ -594,10 +594,10 @@ export class AccountsController extends BaseController< ], type: EthAccountType.Eoa, metadata: { - name: this.#populateExistingMetadata(id, 'name') || '', + name: this.#populateExistingMetadata(id, 'name') ?? '', importTime: - this.#populateExistingMetadata(id, 'importTime') || Date.now(), - lastSelected: this.#populateExistingMetadata(id, 'lastSelected') || 0, + this.#populateExistingMetadata(id, 'importTime') ?? Date.now(), + lastSelected: this.#populateExistingMetadata(id, 'lastSelected') ?? 0, keyring: { type: (keyring as Keyring).type, }, @@ -755,8 +755,8 @@ export class AccountsController extends BaseController< (accountA, accountB) => { // sort by lastSelected descending return ( - (accountB.metadata.lastSelected || 0) - - (accountA.metadata.lastSelected || 0) + (accountB.metadata.lastSelected ?? 0) - + (accountA.metadata.lastSelected ?? 0) ); }, ); From bc4ac8fa9aab61a18955f1d472b60602c9e61b1d Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Wed, 19 Jun 2024 15:31:44 +0800 Subject: [PATCH 06/10] refactor: move addition and deletion into update --- .../src/AccountsController.ts | 47 ++++++++++--------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index 4971a16f0f1..bf1d7a91185 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -24,7 +24,7 @@ import type { SnapStateChange, } from '@metamask/snaps-controllers'; import type { SnapId } from '@metamask/snaps-sdk'; -import { deepClone, type Snap } from '@metamask/snaps-utils'; +import type { Snap } from '@metamask/snaps-utils'; import type { CaipChainId } from '@metamask/utils'; import { type Keyring, @@ -717,33 +717,38 @@ export class AccountsController extends BaseController< } } - let accountsState = deepClone(this.state.internalAccounts.accounts); + this.update((currentState: Draft) => { + const newState = deepCloneDraft(currentState); - if (deletedAccounts.length > 0) { - for (const account of deletedAccounts) { - accountsState = this.#handleAccountRemoved(accountsState, account.id); + if (deletedAccounts.length > 0) { + for (const account of deletedAccounts) { + newState.internalAccounts.accounts = this.#handleAccountRemoved( + newState.internalAccounts.accounts, + account.id, + ); + } } - } - if (addedAccounts.length > 0) { - for (const account of addedAccounts) { - accountsState = this.#handleNewAccountAdded(accountsState, account); + if (addedAccounts.length > 0) { + for (const account of addedAccounts) { + newState.internalAccounts.accounts = this.#handleNewAccountAdded( + newState.internalAccounts.accounts, + account, + ); + } } - } - this.update((currentState: Draft) => { - const newState: AccountsControllerState = { - internalAccounts: { - accounts: accountsState, - selectedAccount: currentState.internalAccounts.selectedAccount, - }, - }; - - const existingAccounts = Object.values(accountsState); + // We don't use list accounts because it is not the updated state yet. + const existingAccounts = Object.values( + newState.internalAccounts.accounts, + ); // handle if the selected account was deleted - if (!accountsState[this.state.internalAccounts.selectedAccount]) { - newState.internalAccounts.accounts = accountsState; + if ( + !newState.internalAccounts.accounts[ + this.state.internalAccounts.selectedAccount + ] + ) { // if the accountToSelect is undefined, then there are no accounts // it mean the keyring was reinitialized. if (existingAccounts.length === 0) { From 2ff7fce60a7bd2deae84a34073f77d7d6889e999 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Fri, 21 Jun 2024 22:25:22 +0800 Subject: [PATCH 07/10] fix: remove deepClone --- .../src/AccountsController.ts | 50 ++++++------------- packages/accounts-controller/src/utils.ts | 17 ------- 2 files changed, 16 insertions(+), 51 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index bf1d7a91185..a8295acb693 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -35,7 +35,6 @@ import { import type { Draft } from 'immer'; import { - deepCloneDraft, getUUIDFromAddressOfNormalAccount, isNormalKeyringType, keyringTypeToName, @@ -412,12 +411,8 @@ export class AccountsController extends BaseController< ...account, metadata: { ...account.metadata, name: accountName }, }; - // FIXME: deep clone of old state to get around Type instantiation is excessively deep and possibly infinite. - const newState = deepCloneDraft(currentState); - - newState.internalAccounts.accounts[accountId] = internalAccount; - - return newState; + // @ts-expect-error Type instantiation is excessively deep and possibly infinite. See: https://github.com/MetaMask/utils/issues/168 + currentState.internalAccounts.accounts[accountId] = internalAccount; }); } @@ -474,12 +469,7 @@ export class AccountsController extends BaseController< }, {} as Record); this.update((currentState: Draft) => { - // FIXME: deep clone of old state to get around Type instantiation is excessively deep and possibly infinite. - const newState = deepCloneDraft(currentState); - - newState.internalAccounts.accounts = accounts; - - return newState; + currentState.internalAccounts.accounts = accounts; }); } @@ -491,12 +481,7 @@ export class AccountsController extends BaseController< loadBackup(backup: AccountsControllerState): void { if (backup.internalAccounts) { this.update((currentState: Draft) => { - // FIXME: deep clone of old state to get around Type instantiation is excessively deep and possibly infinite. - const newState = deepCloneDraft(currentState); - - newState.internalAccounts = backup.internalAccounts; - - return newState; + currentState.internalAccounts = backup.internalAccounts; }); } } @@ -718,12 +703,10 @@ export class AccountsController extends BaseController< } this.update((currentState: Draft) => { - const newState = deepCloneDraft(currentState); - if (deletedAccounts.length > 0) { for (const account of deletedAccounts) { - newState.internalAccounts.accounts = this.#handleAccountRemoved( - newState.internalAccounts.accounts, + currentState.internalAccounts.accounts = this.#handleAccountRemoved( + currentState.internalAccounts.accounts, account.id, ); } @@ -731,29 +714,30 @@ export class AccountsController extends BaseController< if (addedAccounts.length > 0) { for (const account of addedAccounts) { - newState.internalAccounts.accounts = this.#handleNewAccountAdded( - newState.internalAccounts.accounts, - account, - ); + currentState.internalAccounts.accounts = + this.#handleNewAccountAdded( + currentState.internalAccounts.accounts, + account, + ); } } // We don't use list accounts because it is not the updated state yet. const existingAccounts = Object.values( - newState.internalAccounts.accounts, + currentState.internalAccounts.accounts, ); // handle if the selected account was deleted if ( - !newState.internalAccounts.accounts[ + !currentState.internalAccounts.accounts[ this.state.internalAccounts.selectedAccount ] ) { // if the accountToSelect is undefined, then there are no accounts // it mean the keyring was reinitialized. if (existingAccounts.length === 0) { - newState.internalAccounts.selectedAccount = ''; - return newState; + currentState.internalAccounts.selectedAccount = ''; + return; } const [accountToSelect] = existingAccounts.sort( @@ -766,10 +750,8 @@ export class AccountsController extends BaseController< }, ); - newState.internalAccounts.selectedAccount = accountToSelect.id; + currentState.internalAccounts.selectedAccount = accountToSelect.id; } - - return newState; }); } } diff --git a/packages/accounts-controller/src/utils.ts b/packages/accounts-controller/src/utils.ts index 458523c1f55..52d5bb8af38 100644 --- a/packages/accounts-controller/src/utils.ts +++ b/packages/accounts-controller/src/utils.ts @@ -1,6 +1,5 @@ import { toBuffer } from '@ethereumjs/util'; import { isCustodyKeyring, KeyringTypes } from '@metamask/keyring-controller'; -import { deepClone } from '@metamask/snaps-utils'; import { sha256 } from 'ethereum-cryptography/sha256'; import type { Draft } from 'immer'; import type { V4Options } from 'uuid'; @@ -83,19 +82,3 @@ export function isNormalKeyringType(keyringType: KeyringTypes): boolean { // adapted later on if we have new kind of keyrings! return keyringType !== KeyringTypes.snap; } - -/** - * WARNING: To be removed once type issue is fixed. https://github.com/MetaMask/utils/issues/168 - * - * Creates a deep clone of the given object. - * This is to get around error `Type instantiation is excessively deep and possibly infinite.` - * - * @param obj - The object to be cloned. - * @returns The deep clone of the object. - */ -export function deepCloneDraft( - obj: Draft, -): AccountsControllerState { - // We use unknown here because the type inference when using structured clone leads to the same type error. - return deepClone(obj) as unknown as AccountsControllerState; -} From a8bedbda5d70b95d969fc41a7f16814ad360b35d Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Mon, 24 Jun 2024 20:35:56 +0800 Subject: [PATCH 08/10] fix: lint --- packages/accounts-controller/src/utils.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/accounts-controller/src/utils.ts b/packages/accounts-controller/src/utils.ts index 52d5bb8af38..d3cb5aede23 100644 --- a/packages/accounts-controller/src/utils.ts +++ b/packages/accounts-controller/src/utils.ts @@ -1,12 +1,9 @@ import { toBuffer } from '@ethereumjs/util'; import { isCustodyKeyring, KeyringTypes } from '@metamask/keyring-controller'; import { sha256 } from 'ethereum-cryptography/sha256'; -import type { Draft } from 'immer'; import type { V4Options } from 'uuid'; import { v4 as uuid } from 'uuid'; -import type { AccountsControllerState } from './AccountsController'; - /** * Returns the name of the keyring type. * From ed5d02232be86979808561dfaa00e709f9d97b17 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Mon, 24 Jun 2024 20:44:57 +0800 Subject: [PATCH 09/10] fix: unused error directive --- 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 a8295acb693..aa55cfa9e03 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -411,7 +411,6 @@ export class AccountsController extends BaseController< ...account, metadata: { ...account.metadata, name: accountName }, }; - // @ts-expect-error Type instantiation is excessively deep and possibly infinite. See: https://github.com/MetaMask/utils/issues/168 currentState.internalAccounts.accounts[accountId] = internalAccount; }); } From 1f998a4991ef3a2b128645ed27721fdfcbb1cf1c Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Mon, 24 Jun 2024 22:13:23 +0800 Subject: [PATCH 10/10] fix: comments --- .../accounts-controller/src/AccountsController.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index aa55cfa9e03..9e97a927b6a 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -278,7 +278,7 @@ export class AccountsController extends BaseController< } /** - * Returns the last selected evm account. + * Returns the last selected EVM account. * * @returns The selected internal account. */ @@ -732,13 +732,15 @@ export class AccountsController extends BaseController< this.state.internalAccounts.selectedAccount ] ) { - // if the accountToSelect is undefined, then there are no accounts + // if currently selected account is undefined and there are no accounts // it mean the keyring was reinitialized. if (existingAccounts.length === 0) { currentState.internalAccounts.selectedAccount = ''; return; } + // at this point, we know that `existingAccounts.length` is > 0, so + // `accountToSelect` won't be `undefined`! const [accountToSelect] = existingAccounts.sort( (accountA, accountB) => { // sort by lastSelected descending @@ -748,7 +750,6 @@ export class AccountsController extends BaseController< ); }, ); - currentState.internalAccounts.selectedAccount = accountToSelect.id; } }); @@ -896,9 +897,9 @@ export class AccountsController extends BaseController< * 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. * If the account is a Snap Keyring account, retrieves the account from the keyring and adds it to the controller. - * @param accountsState - Accounts Controller accounts state that is to be mutated. + * @param accountsState - AccountsController accounts state that is to be mutated. * @param account - The address and keyring type object of the new account. - * @returns The updated Accounts Controller accounts state. + * @returns The updated AccountsController accounts state. */ #handleNewAccountAdded( accountsState: AccountsControllerState['internalAccounts']['accounts'], @@ -947,9 +948,9 @@ export class AccountsController extends BaseController< /** * Handles the removal of an account from the internal accounts list. - * @param accountsState - Accounts Controller accounts state that is to be mutated. + * @param accountsState - AccountsController accounts state that is to be mutated. * @param accountId - The ID of the account to be removed. - * @returns The updated Accounts Controller state. + * @returns The updated AccountsController state. */ #handleAccountRemoved( accountsState: AccountsControllerState['internalAccounts']['accounts'],