From ea80db9e279e1730a3276bc36dc301fca850325c Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 25 Jul 2023 00:23:53 +0200 Subject: [PATCH 1/4] refactor: use addNewAccount from core KeyringController --- .../metamask-controller.actions.test.js | 12 ++----- app/scripts/metamask-controller.js | 34 +++---------------- app/scripts/metamask-controller.test.js | 4 +-- ui/store/actions.test.js | 2 +- ui/store/actions.ts | 10 ++---- 5 files changed, 13 insertions(+), 49 deletions(-) diff --git a/app/scripts/metamask-controller.actions.test.js b/app/scripts/metamask-controller.actions.test.js index 5e7c771a1a6b..397648c371da 100644 --- a/app/scripts/metamask-controller.actions.test.js +++ b/app/scripts/metamask-controller.actions.test.js @@ -145,27 +145,21 @@ describe('MetaMaskController', function () { metamaskController.addNewAccount(1), metamaskController.addNewAccount(1), ]); - assert.deepEqual( - Object.keys(addNewAccountResult1.identities), - Object.keys(addNewAccountResult2.identities), - ); + assert.equal(addNewAccountResult1, addNewAccountResult2); }); it('two successive calls with same accountCount give same result', async function () { await metamaskController.createNewVaultAndKeychain('test@123'); const addNewAccountResult1 = await metamaskController.addNewAccount(1); const addNewAccountResult2 = await metamaskController.addNewAccount(1); - assert.deepEqual( - Object.keys(addNewAccountResult1.identities), - Object.keys(addNewAccountResult2.identities), - ); + assert.equal(addNewAccountResult1, addNewAccountResult2); }); it('two successive calls with different accountCount give different results', async function () { await metamaskController.createNewVaultAndKeychain('test@123'); const addNewAccountResult1 = await metamaskController.addNewAccount(1); const addNewAccountResult2 = await metamaskController.addNewAccount(2); - assert.notDeepEqual(addNewAccountResult1, addNewAccountResult2); + assert.notEqual(addNewAccountResult1, addNewAccountResult2); }); }); diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index fa120200fc2a..5238af01ccb2 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -3404,38 +3404,12 @@ export default class MetamaskController extends EventEmitter { await new Promise((resolve) => setTimeout(resolve, 5_000)); } - const [primaryKeyring] = this.coreKeyringController.getKeyringsByType( - KeyringType.hdKeyTree, - ); - if (!primaryKeyring) { - throw new Error('MetamaskController - No HD Key Tree found'); - } - const { keyringController } = this; - const { identities: oldIdentities } = - this.preferencesController.store.getState(); - - if (Object.keys(oldIdentities).length === accountCount) { - const oldAccounts = await keyringController.getAccounts(); - const keyState = await keyringController.addNewAccount(primaryKeyring); - const newAccounts = await keyringController.getAccounts(); + const { addedAccountAddress } = + await this.coreKeyringController.addNewAccount(accountCount); - await this.verifySeedPhrase(); + this.preferencesController.setSelectedAddress(addedAccountAddress); - this.preferencesController.setAddresses(newAccounts); - newAccounts.forEach((address) => { - if (!oldAccounts.includes(address)) { - this.preferencesController.setSelectedAddress(address); - } - }); - - const { identities } = this.preferencesController.store.getState(); - return { ...keyState, identities }; - } - - return { - ...keyringController.memStore.getState(), - identities: oldIdentities, - }; + return addedAccountAddress; } /** diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index b11e34372ffb..f2c3fa6adeeb 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -737,7 +737,7 @@ describe('MetaMaskController', function () { metamaskController.keyringController, 'addNewAccount', ); - addNewAccountStub.returns({}); + addNewAccountStub.returns('0x123'); getAccountsStub = sinon.stub( metamaskController.keyringController, @@ -818,7 +818,7 @@ describe('MetaMaskController', function () { await addNewAccount; assert.fail('should throw'); } catch (e) { - assert.equal(e.message, 'MetamaskController - No HD Key Tree found'); + assert.equal(e.message, 'No HD keyring found'); } }); }); diff --git a/ui/store/actions.test.js b/ui/store/actions.test.js index 933ec6ce2c91..359e2a349b03 100644 --- a/ui/store/actions.test.js +++ b/ui/store/actions.test.js @@ -398,7 +398,7 @@ describe('Actions', () => { const addNewAccount = background.addNewAccount.callsFake((_, cb) => cb(null, { - identities: {}, + addedAccountAddress: '0x123', }), ); diff --git a/ui/store/actions.ts b/ui/store/actions.ts index 6d92e5b1b246..00e94aa71a27 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -432,12 +432,11 @@ export function addNewAccount(): ThunkAction< const oldIdentities = getState().metamask.identities; dispatch(showLoadingIndication()); - let newIdentities; + let addedAccountAddress; try { - const { identities } = await submitRequestToBackground('addNewAccount', [ + addedAccountAddress = await submitRequestToBackground('addNewAccount', [ Object.keys(oldIdentities).length, ]); - newIdentities = identities; } catch (error) { dispatch(displayWarning(error)); throw error; @@ -445,11 +444,8 @@ export function addNewAccount(): ThunkAction< dispatch(hideLoadingIndication()); } - const newAccountAddress = Object.keys(newIdentities).find( - (address) => !oldIdentities[address], - ); await forceUpdateMetamaskState(dispatch); - return newAccountAddress; + return addedAccountAddress; }; } From c0a35640f1897d7cd4712abe62bda54df410eebc Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 25 Jul 2023 14:25:01 +0200 Subject: [PATCH 2/4] refactor: replace missed interaction --- app/scripts/metamask-controller.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 5238af01ccb2..6c65dcd7027e 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -2943,12 +2943,10 @@ export default class MetamaskController extends EventEmitter { // seek out the first zero balance while (lastBalance !== '0x0') { - await this.coreKeyringController.addNewAccount(accounts.length); + const { addedAccountAddress } = + await this.coreKeyringController.addNewAccount(accounts.length); accounts = await this.coreKeyringController.getAccounts(); - lastBalance = await this.getBalance( - accounts[accounts.length - 1], - ethQuery, - ); + lastBalance = await this.getBalance(addedAccountAddress, ethQuery); } // remove extra zero balance account potentially created from seeking ahead From 6a11f2ee409e85b0fec8fbbcb63987fd10f26c39 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 31 Jul 2023 11:10:36 +0200 Subject: [PATCH 3/4] refactor: select account only when is new --- app/scripts/metamask-controller.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 6c65dcd7027e..bae56c2584a7 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -3392,7 +3392,7 @@ export default class MetamaskController extends EventEmitter { * Adds a new account to the default (first) HD seed phrase Keyring. * * @param accountCount - * @returns {} keyState + * @returns {Promise} The address of the newly-created account. */ async addNewAccount(accountCount) { const isActionMetricsQueueE2ETest = @@ -3402,10 +3402,15 @@ export default class MetamaskController extends EventEmitter { await new Promise((resolve) => setTimeout(resolve, 5_000)); } + const { identities: oldIdentities } = + this.preferencesController.store.getState(); + const { addedAccountAddress } = await this.coreKeyringController.addNewAccount(accountCount); - this.preferencesController.setSelectedAddress(addedAccountAddress); + if (Object.keys(oldIdentities).length === accountCount) { + this.preferencesController.setSelectedAddress(addedAccountAddress); + } return addedAccountAddress; } From 275037f584f9f1c8ac73d0bb358243a94d0df41b Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 31 Jul 2023 15:19:39 +0200 Subject: [PATCH 4/4] refactor: use getAccounts to check if account is new --- app/scripts/metamask-controller.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index bae56c2584a7..366556d359e8 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -3402,13 +3402,12 @@ export default class MetamaskController extends EventEmitter { await new Promise((resolve) => setTimeout(resolve, 5_000)); } - const { identities: oldIdentities } = - this.preferencesController.store.getState(); + const oldAccounts = await this.coreKeyringController.getAccounts(); const { addedAccountAddress } = await this.coreKeyringController.addNewAccount(accountCount); - if (Object.keys(oldIdentities).length === accountCount) { + if (!oldAccounts.includes(addedAccountAddress)) { this.preferencesController.setSelectedAddress(addedAccountAddress); }