diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 1827c6d57b48..219e66445723 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -147,6 +147,7 @@ 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 { AuthenticationController, UserStorageController, @@ -4238,7 +4239,10 @@ 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.setLedgerTransportPreference(); + this.withKeyringForDevice( + { name: HardwareDeviceNames.ledger }, + async (keyring) => this.setLedgerTransportPreference(keyring), + ); } } finally { releaseLock(); @@ -4370,7 +4374,10 @@ export default class MetamaskController extends EventEmitter { // Optimistically called to not block MetaMask login due to // Ledger Keyring GitHub downtime if (completedOnboarding) { - this.setLedgerTransportPreference(); + this.withKeyringForDevice( + { name: HardwareDeviceNames.ledger }, + async (keyring) => this.setLedgerTransportPreference(keyring), + ); } } @@ -4474,52 +4481,76 @@ export default class MetamaskController extends EventEmitter { // Hardware // - async getKeyringForDevice(deviceName, hdPath = null) { + /** + * 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) { const keyringOverrides = this.opts.overrides?.keyrings; - let keyringName = null; - switch (deviceName) { + let keyringType = null; + switch (options.name) { case HardwareDeviceNames.trezor: - keyringName = keyringOverrides?.trezor?.type || TrezorKeyring.type; + keyringType = keyringOverrides?.trezor?.type || TrezorKeyring.type; break; case HardwareDeviceNames.ledger: - keyringName = keyringOverrides?.ledger?.type || LedgerKeyring.type; + keyringType = keyringOverrides?.ledger?.type || LedgerKeyring.type; break; case HardwareDeviceNames.qr: - keyringName = QRHardwareKeyring.type; + keyringType = QRHardwareKeyring.type; break; case HardwareDeviceNames.lattice: - keyringName = keyringOverrides?.lattice?.type || LatticeKeyring.type; + keyringType = keyringOverrides?.lattice?.type || LatticeKeyring.type; break; default: throw new Error( - 'MetamaskController:getKeyringForDevice - Unknown device', + 'MetamaskController:withKeyringForDevice - 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); - } - keyring.network = getProviderConfig({ - metamask: this.networkController.state, - }).type; + 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 = getProviderConfig({ + metamask: this.networkController.state, + }).type; - return keyring; + return await callback(keyring); + }, + { + createIfMissing: true, + }, + ); } async attemptLedgerTransportCreation() { - const keyring = await this.getKeyringForDevice(HardwareDeviceNames.ledger); - return await keyring.attemptMakeApp(); + return await this.withKeyringForDevice( + HardwareDeviceNames.ledger, + async (keyring) => keyring.attemptMakeApp(), + ); } /** @@ -4531,35 +4562,38 @@ export default class MetamaskController extends EventEmitter { * @returns [] accounts */ async connectHardware(deviceName, page, hdPath) { - const keyring = await this.getKeyringForDevice(deviceName, hdPath); - - if (deviceName === HardwareDeviceNames.ledger) { - await this.setLedgerTransportPreference(keyring); - } + return this.withKeyringForDevice( + { name: deviceName, hdPath }, + async (keyring) => { + if (deviceName === HardwareDeviceNames.ledger) { + await this.setLedgerTransportPreference(keyring); + } - let accounts = []; - switch (page) { - case -1: - accounts = await keyring.getPreviousPage(); - break; - case 1: - accounts = await keyring.getNextPage(); - break; - default: - accounts = await keyring.getFirstPage(); - } + let accounts = []; + switch (page) { + case -1: + accounts = await keyring.getPreviousPage(); + break; + case 1: + accounts = await keyring.getNextPage(); + break; + default: + accounts = await keyring.getFirstPage(); + } - // Merge with existing accounts - // and make sure addresses are not repeated - const oldAccounts = await this.keyringController.getAccounts(); + // 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; + const accountsToTrack = [ + ...new Set( + oldAccounts.concat(accounts.map((a) => a.address.toLowerCase())), + ), + ]; + this.accountTracker.syncWithAddresses(accountsToTrack); + return accounts; + }, + ); } /** @@ -4570,8 +4604,12 @@ export default class MetamaskController extends EventEmitter { * @returns {Promise} */ async checkHardwareStatus(deviceName, hdPath) { - const keyring = await this.getKeyringForDevice(deviceName, hdPath); - return keyring.isUnlocked(); + return this.withKeyringForDevice( + { name: deviceName, hdPath }, + async (keyring) => { + return keyring.isUnlocked(); + }, + ); } /** @@ -4581,14 +4619,15 @@ export default class MetamaskController extends EventEmitter { * @returns {Promise} */ async forgetDevice(deviceName) { - const keyring = await this.getKeyringForDevice(deviceName); + return this.withKeyringForDevice({ name: deviceName }, async (keyring) => { + for (const address of keyring.accounts) { + await this._onAccountRemoved(address); + } - for (const address of keyring.accounts) { - await this.removeAccount(address); - } + keyring.forgetDevice(); - keyring.forgetDevice(); - return true; + return true; + }); } /** @@ -4626,21 +4665,22 @@ export default class MetamaskController extends EventEmitter { * @returns {'ledger' | 'lattice' | string | undefined} */ async getDeviceModel(address) { - 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; - } + 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; + } + }); } /** @@ -4670,16 +4710,24 @@ export default class MetamaskController extends EventEmitter { hdPath, 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, + 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, + ), + }; + }, ); + // Set the account label to Trezor 1 / Ledger 1 / QR Hardware 1, etc this.preferencesController.setAccountLabel(unlockedAccount, label); // Select the account @@ -4834,20 +4882,8 @@ export default class MetamaskController extends EventEmitter { * @param {string[]} address - A hex address */ async removeAccount(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._onAccountRemoved(address); await this.keyringController.removeAccount(address); - const updatedKeyringAccounts = keyring ? await keyring.getAccounts() : {}; - if (updatedKeyringAccounts?.length === 0) { - keyring.destroy?.(); - } return address; } @@ -6139,6 +6175,21 @@ 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 /** @@ -6400,16 +6451,15 @@ 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 4121160a45af..2c35fea7f0e4 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -791,21 +791,16 @@ describe('MetaMaskController', () => { ); await expect(result).rejects.toThrow( - 'MetamaskController:getKeyringForDevice - Unknown device', + 'MetamaskController:withKeyringForDevice - 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); @@ -813,16 +808,11 @@ 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); @@ -837,7 +827,7 @@ describe('MetaMaskController', () => { `m/44/0'/0'`, ); await expect(result).rejects.toThrow( - 'MetamaskController:getKeyringForDevice - Unknown device', + 'MetamaskController:withKeyringForDevice - Unknown device', ); }); @@ -864,7 +854,7 @@ describe('MetaMaskController', () => { 'Some random device name', ); await expect(result).rejects.toThrow( - 'MetamaskController:getKeyringForDevice - Unknown device', + 'MetamaskController:withKeyringForDevice - Unknown device', ); }); @@ -953,22 +943,6 @@ 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, @@ -1160,14 +1134,6 @@ 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', () => {