From 6a165c82a531b5bb1342f624071f8e3032fb4162 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 10 Sep 2024 15:40:40 +0200 Subject: [PATCH 1/2] refactor: use `withKeyring` method (#25435) --- app/scripts/metamask-controller.js | 260 ++++++++++++++---------- app/scripts/metamask-controller.test.js | 40 +--- 2 files changed, 158 insertions(+), 142 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 479fe748c609..e20d1741064d 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -143,6 +143,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, @@ -4145,7 +4146,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(); @@ -4277,7 +4281,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), + ); } } @@ -4381,52 +4388,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(), + ); } /** @@ -4438,35 +4469,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; + }, + ); } /** @@ -4477,8 +4511,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(); + }, + ); } /** @@ -4488,14 +4526,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; + }); } /** @@ -4533,21 +4572,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; + } + }); } /** @@ -4577,16 +4617,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 @@ -4763,20 +4811,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; } @@ -6025,6 +6061,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 /** @@ -6306,16 +6357,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 bae372187f14..ec2f6339cfa0 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -797,21 +797,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); @@ -819,16 +814,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); @@ -843,7 +833,7 @@ describe('MetaMaskController', () => { `m/44/0'/0'`, ); await expect(result).rejects.toThrow( - 'MetamaskController:getKeyringForDevice - Unknown device', + 'MetamaskController:withKeyringForDevice - Unknown device', ); }); @@ -870,7 +860,7 @@ describe('MetaMaskController', () => { 'Some random device name', ); await expect(result).rejects.toThrow( - 'MetamaskController:getKeyringForDevice - Unknown device', + 'MetamaskController:withKeyringForDevice - Unknown device', ); }); @@ -959,22 +949,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, @@ -1166,14 +1140,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', () => { From 181a31f7e0e5c6c8fb45377c42e95e074e7e6eb4 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 24 Sep 2024 12:07:03 +0200 Subject: [PATCH 2/2] test: update snapshots --- .../info/__snapshots__/info.test.tsx.snap | 401 ++++++++++-------- .../approve-details.test.tsx.snap | 13 +- .../set-approval-for-all-info.test.tsx.snap | 154 ++++--- ...al-for-all-static-simulation.test.tsx.snap | 50 ++- 4 files changed, 345 insertions(+), 273 deletions(-) diff --git a/ui/pages/confirmations/components/confirm/info/__snapshots__/info.test.tsx.snap b/ui/pages/confirmations/components/confirm/info/__snapshots__/info.test.tsx.snap index 93ff5170e172..60bb488888b3 100644 --- a/ui/pages/confirmations/components/confirm/info/__snapshots__/info.test.tsx.snap +++ b/ui/pages/confirmations/components/confirm/info/__snapshots__/info.test.tsx.snap @@ -12,13 +12,16 @@ exports[`Info renders info section for approve request 1`] = ` >
-

- Data -

+

+ Data +

+
-

- Data -

+

+ Data +

+
-

- Request from -

-
-
- + Request from +

+
+
+ +
@@ -185,27 +194,30 @@ exports[`Info renders info section for approve request 1`] = ` >
-

- Network fee -

-
-
- + Network fee +

+
+
+ +
@@ -246,13 +258,16 @@ exports[`Info renders info section for approve request 1`] = ` >
-

- Speed -

+

+ Speed +

+
-

- Request from -

-
-
- + Request from +

+
+
+ +
@@ -416,27 +434,30 @@ exports[`Info renders info section for contract interaction request 1`] = ` >
-

- Interacting with -

-
-
- + Interacting with +

+
+
+ +
@@ -510,27 +531,30 @@ exports[`Info renders info section for contract interaction request 1`] = ` >
-

- Network fee -

-
-
- + Network fee +

+
+
+ +
@@ -571,13 +595,16 @@ exports[`Info renders info section for contract interaction request 1`] = ` >
-

- Speed -

+

+ Speed +

+
-

- Estimated changes -

-
-
- + Estimated changes +

+
+
+ +
@@ -742,13 +772,16 @@ exports[`Info renders info section for setApprovalForAll request 1`] = ` >
-

- Withdrawing -

+

+ Withdrawing +

+
-

- Data -

+

+ Data +

+
-

- Request from -

-
-
- + Request from +

+
+
+ +
@@ -906,27 +945,30 @@ exports[`Info renders info section for setApprovalForAll request 1`] = ` >
-

- Network fee -

-
-
- + Network fee +

+
+
+ +
@@ -967,13 +1009,16 @@ exports[`Info renders info section for setApprovalForAll request 1`] = ` >
-

- Speed -

+

+ Speed +

+
renders component for approve details for setApprova >
-

- Data -

+

+ Data +

+
renders component for approve request 1`] = ` >
-

- Estimated changes -

-
-
- + Estimated changes +

+
+
+ +
@@ -54,13 +57,16 @@ exports[` renders component for approve request 1`] = ` >
-

- Withdrawing -

+

+ Withdrawing +

+
renders component for approve request 1`] = ` >
-

- Data -

+

+ Data +

+
renders component for approve request 1`] = ` >
-

- Request from -

-
-
- + Request from +

+
+
+ +
@@ -218,27 +230,30 @@ exports[` renders component for approve request 1`] = ` >
-

- Network fee -

-
-
- + Network fee +

+
+
+ +
@@ -279,13 +294,16 @@ exports[` renders component for approve request 1`] = ` >
-

- Speed -

+

+ Speed +

+
renders component for approve req >
-

- Estimated changes -

-
-
- + Estimated changes +

+
+
+ +
@@ -52,13 +55,16 @@ exports[` renders component for approve req >
-

- Withdrawing -

+

+ Withdrawing +

+