From daeef0935e74fbddf9b358beb7f81cfa59349f5d Mon Sep 17 00:00:00 2001 From: seaona Date: Tue, 23 Jul 2024 12:12:24 +0200 Subject: [PATCH 1/3] Revert "refactor: use `withKeyring` method (#25435)" This reverts commit def6b1545e020c14e94463e0480765f1027e74b2. --- app/scripts/metamask-controller.js | 256 ++++++++++-------------- app/scripts/metamask-controller.test.js | 40 +++- 2 files changed, 140 insertions(+), 156 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 715cb3bf71cd..408d3f70d9ca 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -142,7 +142,6 @@ import { import { Interface } from '@ethersproject/abi'; import { abiERC1155, abiERC721 } from '@metamask/metamask-eth-abis'; import { isEvmAccountType } from '@metamask/keyring-api'; -import { normalize } from '@metamask/eth-sig-util'; import { methodsRequiringNetworkSwitch, methodsWithConfirmation, @@ -4046,10 +4045,7 @@ export default class MetamaskController extends EventEmitter { // keyring's iframe and have the setting initialized properly // Optimistically called to not block MetaMask login due to // Ledger Keyring GitHub downtime - this.withKeyringForDevice( - { name: HardwareDeviceNames.ledger }, - async (keyring) => this.setLedgerTransportPreference(keyring), - ); + this.setLedgerTransportPreference(); } } finally { releaseLock(); @@ -4179,10 +4175,7 @@ export default class MetamaskController extends EventEmitter { // Optimistically called to not block MetaMask login due to // Ledger Keyring GitHub downtime if (completedOnboarding) { - this.withKeyringForDevice( - { name: HardwareDeviceNames.ledger }, - async (keyring) => this.setLedgerTransportPreference(keyring), - ); + this.setLedgerTransportPreference(); } } @@ -4286,74 +4279,50 @@ export default class MetamaskController extends EventEmitter { // Hardware // - /** - * Select a hardware wallet device and execute a - * callback with the keyring for that device. - * - * Note that KeyringController state is not updated before - * the end of the callback execution, and calls to KeyringController - * methods within the callback can lead to deadlocks. - * - * @param {object} options - The options for the device - * @param {string} options.name - The device name to select - * @param {string} options.hdPath - An optional hd path to be set on the device - * keyring - * @param {*} callback - The callback to execute with the keyring - * @returns {*} The result of the callback - */ - async withKeyringForDevice(options, callback) { + async getKeyringForDevice(deviceName, hdPath = null) { const keyringOverrides = this.opts.overrides?.keyrings; - let keyringType = null; - switch (options.name) { + let keyringName = null; + switch (deviceName) { case HardwareDeviceNames.trezor: - keyringType = keyringOverrides?.trezor?.type || TrezorKeyring.type; + keyringName = keyringOverrides?.trezor?.type || TrezorKeyring.type; break; case HardwareDeviceNames.ledger: - keyringType = keyringOverrides?.ledger?.type || LedgerKeyring.type; + keyringName = keyringOverrides?.ledger?.type || LedgerKeyring.type; break; case HardwareDeviceNames.qr: - keyringType = QRHardwareKeyring.type; + keyringName = QRHardwareKeyring.type; break; case HardwareDeviceNames.lattice: - keyringType = keyringOverrides?.lattice?.type || LatticeKeyring.type; + keyringName = keyringOverrides?.lattice?.type || LatticeKeyring.type; break; default: throw new Error( - 'MetamaskController:withKeyringForDevice - Unknown device', + 'MetamaskController:getKeyringForDevice - Unknown device', ); } + let [keyring] = await this.keyringController.getKeyringsByType(keyringName); + if (!keyring) { + keyring = await this.keyringController.addNewKeyring(keyringName); + } + if (hdPath && keyring.setHdPath) { + keyring.setHdPath(hdPath); + } + if (deviceName === HardwareDeviceNames.lattice) { + keyring.appName = 'MetaMask'; + } + if (deviceName === HardwareDeviceNames.trezor) { + const model = keyring.getModel(); + this.appStateController.setTrezorModel(model); + } - return this.keyringController.withKeyring( - { type: keyringType }, - async (keyring) => { - if (options.hdPath && keyring.setHdPath) { - keyring.setHdPath(options.hdPath); - } - - if (options.name === HardwareDeviceNames.lattice) { - keyring.appName = 'MetaMask'; - } - - if (options.name === HardwareDeviceNames.trezor) { - const model = keyring.getModel(); - this.appStateController.setTrezorModel(model); - } - - keyring.network = this.networkController.state.providerConfig.type; + keyring.network = this.networkController.state.providerConfig.type; - return await callback(keyring); - }, - { - createIfMissing: true, - }, - ); + return keyring; } async attemptLedgerTransportCreation() { - return await this.withKeyringForDevice( - HardwareDeviceNames.ledger, - async (keyring) => keyring.attemptMakeApp(), - ); + const keyring = await this.getKeyringForDevice(HardwareDeviceNames.ledger); + return await keyring.attemptMakeApp(); } /** @@ -4365,38 +4334,35 @@ export default class MetamaskController extends EventEmitter { * @returns [] accounts */ async connectHardware(deviceName, page, hdPath) { - return this.withKeyringForDevice( - { name: deviceName, hdPath }, - async (keyring) => { - if (deviceName === HardwareDeviceNames.ledger) { - await this.setLedgerTransportPreference(keyring); - } + const keyring = await this.getKeyringForDevice(deviceName, hdPath); - let accounts = []; - switch (page) { - case -1: - accounts = await keyring.getPreviousPage(); - break; - case 1: - accounts = await keyring.getNextPage(); - break; - default: - accounts = await keyring.getFirstPage(); - } + if (deviceName === HardwareDeviceNames.ledger) { + await this.setLedgerTransportPreference(keyring); + } - // Merge with existing accounts - // and make sure addresses are not repeated - const oldAccounts = await this.keyringController.getAccounts(); + let accounts = []; + switch (page) { + case -1: + accounts = await keyring.getPreviousPage(); + break; + case 1: + accounts = await keyring.getNextPage(); + break; + default: + accounts = await keyring.getFirstPage(); + } - const accountsToTrack = [ - ...new Set( - oldAccounts.concat(accounts.map((a) => a.address.toLowerCase())), - ), - ]; - this.accountTracker.syncWithAddresses(accountsToTrack); - return accounts; - }, - ); + // Merge with existing accounts + // and make sure addresses are not repeated + const oldAccounts = await this.keyringController.getAccounts(); + + const accountsToTrack = [ + ...new Set( + oldAccounts.concat(accounts.map((a) => a.address.toLowerCase())), + ), + ]; + this.accountTracker.syncWithAddresses(accountsToTrack); + return accounts; } /** @@ -4407,12 +4373,8 @@ export default class MetamaskController extends EventEmitter { * @returns {Promise} */ async checkHardwareStatus(deviceName, hdPath) { - return this.withKeyringForDevice( - { name: deviceName, hdPath }, - async (keyring) => { - return keyring.isUnlocked(); - }, - ); + const keyring = await this.getKeyringForDevice(deviceName, hdPath); + return keyring.isUnlocked(); } /** @@ -4422,15 +4384,14 @@ export default class MetamaskController extends EventEmitter { * @returns {Promise} */ async forgetDevice(deviceName) { - return this.withKeyringForDevice({ name: deviceName }, async (keyring) => { - for (const address of keyring.accounts) { - await this._onAccountRemoved(address); - } + const keyring = await this.getKeyringForDevice(deviceName); - keyring.forgetDevice(); + for (const address of keyring.accounts) { + await this.removeAccount(address); + } - return true; - }); + keyring.forgetDevice(); + return true; } /** @@ -4468,22 +4429,21 @@ export default class MetamaskController extends EventEmitter { * @returns {'ledger' | 'lattice' | string | undefined} */ async getDeviceModel(address) { - return this.keyringController.withKeyring({ address }, async (keyring) => { - switch (keyring.type) { - case KeyringType.trezor: - return keyring.getModel(); - case KeyringType.qr: - return keyring.getName(); - case KeyringType.ledger: - // TODO: get model after ledger keyring exposes method - return HardwareDeviceNames.ledger; - case KeyringType.lattice: - // TODO: get model after lattice keyring exposes method - return HardwareDeviceNames.lattice; - default: - return undefined; - } - }); + const keyring = await this.keyringController.getKeyringForAccount(address); + switch (keyring.type) { + case KeyringType.trezor: + return keyring.getModel(); + case KeyringType.qr: + return keyring.getName(); + case KeyringType.ledger: + // TODO: get model after ledger keyring exposes method + return HardwareDeviceNames.ledger; + case KeyringType.lattice: + // TODO: get model after lattice keyring exposes method + return HardwareDeviceNames.lattice; + default: + return undefined; + } } /** @@ -4513,24 +4473,16 @@ export default class MetamaskController extends EventEmitter { hdPath, hdPathDescription, ) { - const { address: unlockedAccount, label } = await this.withKeyringForDevice( - { name: deviceName, hdPath }, - async (keyring) => { - keyring.setAccountToUnlock(index); - const [address] = await keyring.addAccounts(1); - return { - address: normalize(address), - label: this.getAccountLabel( - deviceName === HardwareDeviceNames.qr - ? keyring.getName() - : deviceName, - index, - hdPathDescription, - ), - }; - }, + const keyring = await this.getKeyringForDevice(deviceName, hdPath); + + keyring.setAccountToUnlock(index); + const unlockedAccount = + await this.keyringController.addNewAccountForKeyring(keyring); + const label = this.getAccountLabel( + deviceName === HardwareDeviceNames.qr ? keyring.getName() : deviceName, + index, + hdPathDescription, ); - // Set the account label to Trezor 1 / Ledger 1 / QR Hardware 1, etc this.preferencesController.setAccountLabel(unlockedAccount, label); // Select the account @@ -4706,8 +4658,20 @@ export default class MetamaskController extends EventEmitter { * @param {string[]} address - A hex address */ async removeAccount(address) { - await this._onAccountRemoved(address); + // Remove all associated permissions + this.removeAllAccountPermissions(address); + + ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) + this.custodyController.removeAccount(address); + ///: END:ONLY_INCLUDE_IF(build-mmi) + + const keyring = await this.keyringController.getKeyringForAccount(address); + // Remove account from the keyring await this.keyringController.removeAccount(address); + const updatedKeyringAccounts = keyring ? await keyring.getAccounts() : {}; + if (updatedKeyringAccounts?.length === 0) { + keyring.destroy?.(); + } return address; } @@ -5920,21 +5884,6 @@ export default class MetamaskController extends EventEmitter { this._notifyChainChange(); } - /** - * Execute side effects of a removed account. - * - * @param {string} address - The address of the account to remove. - * @returns {Promise} - */ - async _onAccountRemoved(address) { - // Remove all associated permissions - this.removeAllAccountPermissions(address); - - ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) - this.custodyController.removeAccount(address); - ///: END:ONLY_INCLUDE_IF(build-mmi) - } - // misc /** @@ -6209,15 +6158,16 @@ export default class MetamaskController extends EventEmitter { /** * Sets the Ledger Live preference to use for Ledger hardware wallet support * - * @param keyring + * @param _keyring * @deprecated This method is deprecated and will be removed in the future. * Only webhid connections are supported in chrome and u2f in firefox. */ - async setLedgerTransportPreference(keyring) { + async setLedgerTransportPreference(_keyring) { const transportType = window.navigator.hid ? LedgerTransportTypes.webhid : LedgerTransportTypes.u2f; - + const keyring = + _keyring || (await this.getKeyringForDevice(HardwareDeviceNames.ledger)); if (keyring?.updateTransportMethod) { return keyring.updateTransportMethod(transportType).catch((e) => { throw e; diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index 73a1f6b61886..8f5e37123a9e 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -818,16 +818,21 @@ describe('MetaMaskController', () => { ); await expect(result).rejects.toThrow( - 'MetamaskController:withKeyringForDevice - Unknown device', + 'MetamaskController:getKeyringForDevice - Unknown device', ); }); it('should add the Trezor Hardware keyring and return the first page of accounts', async () => { + jest.spyOn(metamaskController.keyringController, 'addNewKeyring'); + const firstPage = await metamaskController.connectHardware( HardwareDeviceNames.trezor, 0, ); + expect( + metamaskController.keyringController.addNewKeyring, + ).toHaveBeenCalledWith(KeyringType.trezor); expect( metamaskController.keyringController.state.keyrings[1].type, ).toBe(TrezorKeyring.type); @@ -835,11 +840,16 @@ describe('MetaMaskController', () => { }); it('should add the Ledger Hardware keyring and return the first page of accounts', async () => { + jest.spyOn(metamaskController.keyringController, 'addNewKeyring'); + const firstPage = await metamaskController.connectHardware( HardwareDeviceNames.ledger, 0, ); + expect( + metamaskController.keyringController.addNewKeyring, + ).toHaveBeenCalledWith(KeyringType.ledger); expect( metamaskController.keyringController.state.keyrings[1].type, ).toBe(LedgerKeyring.type); @@ -854,7 +864,7 @@ describe('MetaMaskController', () => { `m/44/0'/0'`, ); await expect(result).rejects.toThrow( - 'MetamaskController:withKeyringForDevice - Unknown device', + 'MetamaskController:getKeyringForDevice - Unknown device', ); }); @@ -881,7 +891,7 @@ describe('MetaMaskController', () => { 'Some random device name', ); await expect(result).rejects.toThrow( - 'MetamaskController:withKeyringForDevice - Unknown device', + 'MetamaskController:getKeyringForDevice - Unknown device', ); }); @@ -970,6 +980,22 @@ describe('MetaMaskController', () => { ]); }); + it('should call keyringController.addNewAccountForKeyring', async () => { + jest.spyOn( + metamaskController.keyringController, + 'addNewAccountForKeyring', + ); + + await metamaskController.unlockHardwareWalletAccount( + accountToUnlock, + device, + ); + + expect( + metamaskController.keyringController.addNewAccountForKeyring, + ).toHaveBeenCalledTimes(1); + }); + it('should call preferencesController.setSelectedAddress', async () => { jest.spyOn( metamaskController.preferencesController, @@ -1161,6 +1187,14 @@ describe('MetaMaskController', () => { it('should return address', async () => { expect(ret).toStrictEqual('0x1'); }); + it('should call keyringController.getKeyringForAccount', async () => { + expect( + metamaskController.keyringController.getKeyringForAccount, + ).toHaveBeenCalledWith(addressToRemove); + }); + it('should call keyring.destroy', async () => { + expect(mockKeyring.destroy).toHaveBeenCalledTimes(1); + }); }); describe('#setupUntrustedCommunicationEip1193', () => { From 22035c4dd69f599c0e936d9c63d0b2bca68d81eb Mon Sep 17 00:00:00 2001 From: seaona Date: Tue, 23 Jul 2024 12:13:42 +0200 Subject: [PATCH 2/3] revert PR --- .circleci/config.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index ba9283f218ca..35b39f2742dc 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -167,7 +167,6 @@ workflows: requires: - prep-deps - prep-build-test-flask-mv2: - <<: *develop_master_rc_only requires: - prep-deps - prep-build-test-mmi: @@ -217,7 +216,6 @@ workflows: - prep-build-test-flask - get-changed-files-with-git-diff - test-e2e-firefox-flask: - <<: *develop_master_rc_only requires: - prep-build-test-flask-mv2 - test-e2e-chrome-mmi: From ff841192deaf56c05219c5b9981a0a8712acf930 Mon Sep 17 00:00:00 2001 From: seaona Date: Tue, 23 Jul 2024 12:44:37 +0200 Subject: [PATCH 3/3] add filter back --- .circleci/config.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index 35b39f2742dc..ba9283f218ca 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -167,6 +167,7 @@ workflows: requires: - prep-deps - prep-build-test-flask-mv2: + <<: *develop_master_rc_only requires: - prep-deps - prep-build-test-mmi: @@ -216,6 +217,7 @@ workflows: - prep-build-test-flask - get-changed-files-with-git-diff - test-e2e-firefox-flask: + <<: *develop_master_rc_only requires: - prep-build-test-flask-mv2 - test-e2e-chrome-mmi: