Skip to content

Commit

Permalink
Update @metamask/eth-keyring-controller (#1441)
Browse files Browse the repository at this point in the history 
* chore: update eth-keyring-controller

* fix: update airgapped keyring and fix deserialize issue

* refactor: leave better comment

* chore: use eth-keyring-controller v12

* refactor: use latest @metamask/utils

* chore: fix lint

* chore: yarn dedupe

* chore: add obs-store module declaration

* refactor: remove cacheEncryptionKey from default test parameters

* refactor: apply @mcmire suggestions

* refactor remove unnecessary typecast

* refactor: simplify keyring builders type

* refactor: remove qr assertion

* refactor: use @metamask/eth-keyring-controller v13

* refactor: remove obs-store module declaration

* refactor: remove unnecessary code

* refactor: addQRKeyring as hash function

* refactor: #addQRKeyring reorder with other prv methods

* refactor: apply @Gudahtt suggestions
  • Loading branch information
@mikesposito @MajorLift
mikesposito authored and MajorLift committed Oct 11, 2023
1 parent c6736df commit 57989b9
Show file tree
Hide file tree
Showing 5 changed files with 1,677 additions and 1,142 deletions.
6 changes: 3 additions & 3 deletions packages/keyring-controller/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,10 @@
"dependencies": {
"@keystonehq/metamask-airgapped-keyring": "^0.13.1",
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/eth-keyring-controller": "^10.0.1",
"@metamask/eth-sig-util": "^6.0.0",
"@metamask/eth-keyring-controller": "^13.0.0",
"@metamask/message-manager": "workspace:^",
"@metamask/preferences-controller": "workspace:^",
"@metamask/utils": "^6.2.0",
"async-mutex": "^0.2.6",
"ethereumjs-util": "^7.0.10",
"ethereumjs-wallet": "^1.0.1",
Expand All @@ -46,6 +45,7 @@
"@ethereumjs/tx": "^4.2.0",
"@keystonehq/bc-ur-registry-eth": "^0.9.0",
"@metamask/auto-changelog": "^3.1.0",
"@metamask/eth-sig-util": "^6.0.0",
"@metamask/scure-bip39": "^2.1.0",
"@types/jest": "^27.4.1",
"deepmerge": "^4.2.2",
Expand Down
120 changes: 86 additions & 34 deletions packages/keyring-controller/src/KeyringController.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { Common } from '@ethereumjs/common';
import { Chain, Common, Hardfork } from '@ethereumjs/common';
import { TransactionFactory } from '@ethereumjs/tx';
import { CryptoHDKey, ETHSignature } from '@keystonehq/bc-ur-registry-eth';
import { MetaMaskKeyring as QRKeyring } from '@keystonehq/metamask-airgapped-keyring';
import { ControllerMessenger } from '@metamask/base-controller';
import { isValidHexAddress } from '@metamask/controller-utils';
import { keyringBuilderFactory } from '@metamask/eth-keyring-controller';
import {
normalize,
Expand All @@ -12,12 +11,17 @@ import {
SignTypedDataVersion,
} from '@metamask/eth-sig-util';
import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english';
import {
isValidHexAddress,
type Hex,
type Keyring,
type Json,
} from '@metamask/utils';
import { bufferToHex } from 'ethereumjs-util';
import * as sinon from 'sinon';
import * as uuid from 'uuid';

import type {
KeyringObject,
KeyringControllerEvents,
KeyringControllerMessenger,
KeyringControllerState,
Expand Down Expand Up @@ -55,7 +59,7 @@ const privateKey =
'1e4e6a4c0c077f4ae8ddfbf372918e61dd0fb4a4cfa592cb16e7546d505e68fc';
const password = 'password123';

const commonConfig = { chain: 'goerli', hardfork: 'berlin' };
const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin };

describe('KeyringController', () => {
afterEach(() => {
Expand Down Expand Up @@ -272,7 +276,7 @@ describe('KeyringController', () => {
expect(initialSeedWord).not.toBe(currentSeedWord);
expect(
isValidHexAddress(
cleanKeyringController.state.keyrings[0].accounts[0],
cleanKeyringController.state.keyrings[0].accounts[0] as Hex,
),
).toBe(true);
expect(controller.state.vault).toBeDefined();
Expand Down Expand Up @@ -347,24 +351,42 @@ describe('KeyringController', () => {
});

describe('exportSeedPhrase', () => {
describe('when correct password is provided', () => {
it('should export seed phrase', async () => {
describe('when mnemonic is not exportable', () => {
it('should throw error', async () => {
await withController(async ({ controller }) => {
const seed = await controller.exportSeedPhrase(password);
expect(seed).not.toBe('');
const primaryKeyring = controller.getKeyringsByType(
KeyringTypes.hd,
)[0] as Keyring<Json> & { mnemonic: string };

primaryKeyring.mnemonic = '';

await expect(controller.exportSeedPhrase(password)).rejects.toThrow(
"Can't get mnemonic bytes from keyring",
);
});
});
});

describe('when wrong password is provided', () => {
it('should export seed phrase', async () => {
await withController(async ({ controller, encryptor }) => {
sinon
.stub(encryptor, 'decrypt')
.throws(new Error('Invalid password'));
await expect(controller.exportSeedPhrase('')).rejects.toThrow(
'Invalid password',
);
describe('when mnemonic is exportable', () => {
describe('when correct password is provided', () => {
it('should export seed phrase', async () => {
await withController(async ({ controller }) => {
const seed = await controller.exportSeedPhrase(password);
expect(seed).not.toBe('');
});
});
});

describe('when wrong password is provided', () => {
it('should export seed phrase', async () => {
await withController(async ({ controller, encryptor }) => {
sinon
.stub(encryptor, 'decrypt')
.throws(new Error('Invalid password'));
await expect(controller.exportSeedPhrase('')).rejects.toThrow(
'Invalid password',
);
});
});
});
});
Expand All @@ -390,7 +412,9 @@ describe('KeyringController', () => {
await withController(async ({ controller }) => {
await expect(
controller.exportAccount(password, ''),
).rejects.toThrow(/^No keyring found for the requested account./u);
).rejects.toThrow(
'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
);
});
});
});
Expand Down Expand Up @@ -433,10 +457,11 @@ describe('KeyringController', () => {
it('should get correct keyring', async () => {
await withController(async ({ controller }) => {
const normalizedInitialAccounts =
controller.state.keyrings[0].accounts.map(normalize) as string[];
controller.state.keyrings[0].accounts.map(normalize);
const keyring = (await controller.getKeyringForAccount(
normalizedInitialAccounts[0],
)) as KeyringObject;
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
normalizedInitialAccounts[0]!,
)) as Keyring<Json>;
expect(keyring.type).toBe('HD Key Tree');
expect(keyring.getAccounts()).toStrictEqual(
normalizedInitialAccounts,
Expand All @@ -448,8 +473,12 @@ describe('KeyringController', () => {
describe('when non-existing account is provided', () => {
it('should throw error', async () => {
await withController(async ({ controller }) => {
await expect(controller.getKeyringForAccount('0x0')).rejects.toThrow(
'No keyring found for the requested account. Error info: There are keyrings, but none match the address',
await expect(
controller.getKeyringForAccount(
'0x0000000000000000000000000000000000000000',
),
).rejects.toThrow(
'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
);
});
});
Expand All @@ -462,7 +491,7 @@ describe('KeyringController', () => {
await withController(async ({ controller }) => {
const keyrings = controller.getKeyringsByType(
KeyringTypes.hd,
) as KeyringObject[];
) as Keyring<Json>[];
expect(keyrings).toHaveLength(1);
expect(keyrings[0].type).toBe(KeyringTypes.hd);
expect(keyrings[0].getAccounts()).toStrictEqual(
Expand Down Expand Up @@ -658,7 +687,7 @@ describe('KeyringController', () => {
*/
it('should remove HD Key Tree keyring from state when single account associated with it is deleted', async () => {
await withController(async ({ controller, initialState }) => {
const account = initialState.keyrings[0].accounts[0];
const account = initialState.keyrings[0].accounts[0] as Hex;
await controller.removeAccount(account);
expect(controller.state.keyrings).toHaveLength(0);
});
Expand Down Expand Up @@ -700,12 +729,16 @@ describe('KeyringController', () => {
[privateKey],
);

await expect(controller.removeAccount('')).rejects.toThrow(
/^No keyring found for the requested account/u,
await expect(
controller.removeAccount(
'0x0000000000000000000000000000000000000000',
),
).rejects.toThrow(
'KeyringController - No keyring found. Error info: There are keyrings, but none match the address',
);

await expect(controller.removeAccount('DUMMY_INPUT')).rejects.toThrow(
/^No keyring found for the requested account/u,
await expect(controller.removeAccount('0xDUMMY_INPUT')).rejects.toThrow(
'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
);
});
});
Expand Down Expand Up @@ -744,7 +777,7 @@ describe('KeyringController', () => {
from: '',
}),
).rejects.toThrow(
'No keyring found for the requested account. Error info: There are keyrings, but none match the address',
'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
);
});
});
Expand Down Expand Up @@ -788,7 +821,7 @@ describe('KeyringController', () => {
from: '',
}),
).rejects.toThrow(
'No keyring found for the requested account. Error info: There are keyrings, but none match the address',
'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
);
});
});
Expand All @@ -797,6 +830,7 @@ describe('KeyringController', () => {
describe('signTypedMessage', () => {
it('should throw when given invalid version', async () => {
await withController(
// @ts-expect-error QRKeyring is not yet compatible with Keyring type.
{ keyringBuilders: [keyringBuilderFactory(QRKeyring)] },
async ({ controller, initialState }) => {
const typedMsgParams = [
Expand Down Expand Up @@ -826,6 +860,7 @@ describe('KeyringController', () => {

it('should sign typed message V1', async () => {
await withController(
// @ts-expect-error QRKeyring is not yet compatible with Keyring type.
{ keyringBuilders: [keyringBuilderFactory(QRKeyring)] },
async ({ controller, initialState }) => {
const typedMsgParams = [
Expand Down Expand Up @@ -857,6 +892,7 @@ describe('KeyringController', () => {

it('should sign typed message V3', async () => {
await withController(
// @ts-expect-error QRKeyring is not yet compatible with Keyring type.
{ keyringBuilders: [keyringBuilderFactory(QRKeyring)] },
async ({ controller, initialState }) => {
const msgParams = {
Expand Down Expand Up @@ -913,6 +949,7 @@ describe('KeyringController', () => {

it('should sign typed message V4', async () => {
await withController(
// @ts-expect-error QRKeyring is not yet compatible with Keyring type.
{ keyringBuilders: [keyringBuilderFactory(QRKeyring)] },
async ({ controller, initialState }) => {
const msgParams = {
Expand Down Expand Up @@ -1073,7 +1110,7 @@ describe('KeyringController', () => {
expect(unsignedEthTx.v).toBeUndefined();
await controller.signTransaction(unsignedEthTx, '');
}).rejects.toThrow(
'No keyring found for the requested account. Error info: There are keyrings, but none match the address',
'KeyringController - No keyring found. Error info: The address passed in is invalid/empty',
);
});
});
Expand All @@ -1086,6 +1123,7 @@ describe('KeyringController', () => {
await withController(async ({ controller, initialState }) => {
await expect(async () => {
const account = initialState.keyrings[0].accounts[0];
// @ts-expect-error invalid transaction
await controller.signTransaction({}, account);
}).rejects.toThrow('tx.sign is not a function');
});
Expand Down Expand Up @@ -1158,6 +1196,20 @@ describe('KeyringController', () => {
expect(seedPhrase).toBeInstanceOf(Uint8Array);
});
});

it('should throw if mnemonic is not defined', async () => {
await withController(async ({ controller }) => {
const primaryKeyring = controller.getKeyringsByType(
KeyringTypes.hd,
)[0] as Keyring<Json> & { mnemonic: string };

primaryKeyring.mnemonic = '';

await expect(controller.verifySeedPhrase()).rejects.toThrow(
"Can't get mnemonic bytes from keyring",
);
});
});
});

describe('verifyPassword', () => {
Expand Down Expand Up @@ -1221,6 +1273,7 @@ describe('KeyringController', () => {

beforeEach(async () => {
signProcessKeyringController = await withController(
// @ts-expect-error QRKeyring is not yet compatible with Keyring type.
{ keyringBuilders: [keyringBuilderFactory(QRKeyring)] },
({ controller }) => controller,
);
Expand Down Expand Up @@ -1730,7 +1783,6 @@ async function withController<ReturnValue>(
const messenger = buildKeyringControllerMessenger();
const controller = new KeyringController({
encryptor,
cacheEncryptionKey: false,
messenger,
...preferences,
...rest,
Expand Down
Loading

0 comments on commit 57989b9

Please sign in to comment.