Skip to content

fix: Revert "refactor: use withKeyring method (#25435)" #26041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 23, 2024
Merged

fix: Revert "refactor: use withKeyring method (#25435)" #26041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
daeef09
Revert "refactor: use `withKeyring` method (#25435)"
seaona Jul 23, 2024
22035c4
revert PR
seaona Jul 23, 2024
ff84119
add filter back
seaona Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
256 changes: 103 additions & 153 deletions app/scripts/metamask-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
}
}

Expand Down Expand Up @@ -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();
}

/**
Expand All @@ -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;
}

/**
Expand All @@ -4407,12 +4373,8 @@ export default class MetamaskController extends EventEmitter {
* @returns {Promise<boolean>}
*/
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();
}

/**
Expand All @@ -4422,15 +4384,14 @@ export default class MetamaskController extends EventEmitter {
* @returns {Promise<boolean>}
*/
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;
}

/**
Expand Down Expand Up @@ -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;
}
}

/**
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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<void>}
*/
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

/**
Expand Down Expand Up @@ -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;
Expand Down
Loading