Skip to content

fix: remove metadata for unsupported keyrings #5725

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.

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 21 commits into from
May 13, 2025
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
335 changes: 156 additions & 179 deletions packages/accounts-controller/src/AccountsController.test.ts

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions packages/accounts-controller/src/AccountsController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -684,11 +684,11 @@ export class AccountsController extends BaseController<
*/
async #listNormalAccounts(): Promise<InternalAccount[]> {
const internalAccounts: InternalAccount[] = [];
const { keyrings, keyringsMetadata } = this.messagingSystem.call(
const { keyrings } = this.messagingSystem.call(
'KeyringController:getState',
);

for (const [keyringIndex, keyring] of keyrings.entries()) {
for (const keyring of keyrings) {
const keyringType = keyring.type;
if (!isNormalKeyringType(keyringType as KeyringTypes)) {
// We only consider "normal accounts" here, so keep looping
Expand All @@ -702,7 +702,7 @@ export class AccountsController extends BaseController<

if (isHdKeyringType(keyring.type as KeyringTypes)) {
options = {
entropySource: keyringsMetadata[keyringIndex].id,
entropySource: keyring.metadata.id,
// NOTE: We are not using the `hdPath` from the associated keyring here and
// getting the keyring instance here feels a bit overkill.
// This will be naturally fixed once every keyring start using `KeyringAccount` and implement the keyring API.
Expand Down
4 changes: 4 additions & 0 deletions packages/keyring-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- Metadata for unsupported keyring is removed on unlock ([#5725](https://github.com/MetaMask/core/pull/5725))

## [21.0.6]

### Changed
Expand Down
6 changes: 3 additions & 3 deletions packages/keyring-controller/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, {
// An object that configures minimum threshold enforcement for coverage results
coverageThreshold: {
global: {
branches: 93.64,
branches: 94.11,
functions: 100,
lines: 98.92,
statements: 98.93,
lines: 99.06,
statements: 99.07,
},
},

Expand Down
189 changes: 153 additions & 36 deletions packages/keyring-controller/src/KeyringController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,95 @@ describe('KeyringController', () => {
},
);
});

it('allows removing a keyring builder without bricking the wallet when metadata was already generated', async () => {
await withController(
{
skipVaultCreation: true,
state: {
vault: 'my vault',
},
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: '',
metadata: { id: 'hd', name: '' },
},
{
type: 'Unsupported',
data: '',
metadata: { id: 'unsupported', name: '' },
},
{
type: KeyringTypes.hd,
data: '',
metadata: { id: 'hd2', name: '' },
},
]);

await controller.submitPassword(password);

expect(controller.state.keyrings).toHaveLength(2);
expect(controller.state.keyrings[0].type).toBe(KeyringTypes.hd);
expect(controller.state.keyrings[0].metadata).toStrictEqual({
id: 'hd',
name: '',
});
expect(controller.state.keyrings[1].type).toBe(KeyringTypes.hd);
expect(controller.state.keyrings[1].metadata).toStrictEqual({
id: 'hd2',
name: '',
});
},
);
});

it('allows removing a keyring builder without bricking the wallet when metadata was not yet generated', async () => {
await withController(
{
skipVaultCreation: true,
state: {
vault: 'my vault',
},
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
{
type: 'HD Key Tree',
data: '',
metadata: { id: 'hd', name: '' },
},
{
type: 'HD Key Tree',
data: '',
metadata: { id: 'hd2', name: '' },
},
// This keyring was already unsupported
// (no metadata, and is at the end of the array)
{
type: MockKeyring.type,
data: 'unsupported',
},
]);

await controller.submitPassword(password);

expect(controller.state.keyrings).toHaveLength(2);
expect(controller.state.keyrings[0].type).toBe(KeyringTypes.hd);
expect(controller.state.keyrings[0].metadata).toStrictEqual({
id: 'hd',
name: '',
});
expect(controller.state.keyrings[1].type).toBe(KeyringTypes.hd);
expect(controller.state.keyrings[1].metadata).toStrictEqual({
id: 'hd2',
name: '',
});
},
);
});
});

describe('addNewAccount', () => {
Expand Down Expand Up @@ -467,20 +556,20 @@ describe('KeyringController', () => {
{ cacheEncryptionKey },
async ({ controller, initialState }) => {
const initialVault = controller.state.vault;
const initialKeyringsMetadata = controller.state.keyringsMetadata;
const initialKeyrings = controller.state.keyrings;
await controller.createNewVaultAndRestore(
password,
uint8ArraySeed,
);
expect(controller.state).not.toBe(initialState);
expect(controller.state.vault).toBeDefined();
expect(controller.state.vault).toStrictEqual(initialVault);
expect(controller.state.keyringsMetadata).toHaveLength(
initialKeyringsMetadata.length,
expect(controller.state.keyrings).toHaveLength(
initialKeyrings.length,
);
// new keyring metadata should be generated
expect(controller.state.keyringsMetadata).not.toStrictEqual(
initialKeyringsMetadata,
expect(controller.state.keyrings).not.toStrictEqual(
initialKeyrings,
);
},
);
Expand All @@ -507,6 +596,10 @@ describe('KeyringController', () => {
{
data: serializedKeyring,
type: 'HD Key Tree',
metadata: {
id: expect.any(String),
name: '',
},
},
]);
},
Expand Down Expand Up @@ -769,7 +862,7 @@ describe('KeyringController', () => {

it('should export seed phrase with valid keyringId', async () => {
await withController(async ({ controller, initialState }) => {
const keyringId = initialState.keyringsMetadata[0].id;
const keyringId = initialState.keyrings[0].metadata.id;
const seed = await controller.exportSeedPhrase(password, keyringId);
expect(seed).not.toBe('');
});
Expand Down Expand Up @@ -799,7 +892,7 @@ describe('KeyringController', () => {
it('should throw invalid password error with valid keyringId', async () => {
await withController(
async ({ controller, encryptor, initialState }) => {
const keyringId = initialState.keyringsMetadata[0].id;
const keyringId = initialState.keyrings[0].metadata.id;
jest
.spyOn(encryptor, 'decrypt')
.mockRejectedValueOnce(new Error('Invalid password'));
Expand Down Expand Up @@ -1203,10 +1296,12 @@ describe('KeyringController', () => {
);
const modifiedState = {
...initialState,
keyrings: [initialState.keyrings[0], newKeyring],
keyringsMetadata: [
initialState.keyringsMetadata[0],
controller.state.keyringsMetadata[1],
keyrings: [
initialState.keyrings[0],
{
...newKeyring,
metadata: controller.state.keyrings[1].metadata,
},
],
};
expect(controller.state).toStrictEqual(modifiedState);
Expand Down Expand Up @@ -1280,10 +1375,12 @@ describe('KeyringController', () => {
};
const modifiedState = {
...initialState,
keyrings: [initialState.keyrings[0], newKeyring],
keyringsMetadata: [
initialState.keyringsMetadata[0],
controller.state.keyringsMetadata[1],
keyrings: [
initialState.keyrings[0],
{
...newKeyring,
metadata: controller.state.keyrings[1].metadata,
},
],
};
expect(controller.state).toStrictEqual(modifiedState);
Expand Down Expand Up @@ -1480,12 +1577,10 @@ describe('KeyringController', () => {
await withController(async ({ controller }) => {
await controller.addNewKeyring(KeyringTypes.hd);
expect(controller.state.keyrings).toHaveLength(2);
expect(controller.state.keyringsMetadata).toHaveLength(2);
await controller.removeAccount(
controller.state.keyrings[1].accounts[0],
);
expect(controller.state.keyrings).toHaveLength(1);
expect(controller.state.keyringsMetadata).toHaveLength(1);
});
});
});
Expand Down Expand Up @@ -2635,10 +2730,12 @@ describe('KeyringController', () => {
});

it('should unlock succesfully when the controller is instantiated with an existing `keyringsMetadata`', async () => {
// @ts-expect-error HdKeyring is not yet compatible with Keyring type.
stubKeyringClassWithAccount(HdKeyring, '0x123');
await withController(
{
cacheEncryptionKey,
state: { keyringsMetadata: [], vault: 'my vault' },
state: { vault: 'my vault' },
skipVaultCreation: true,
},
async ({ controller, encryptor }) => {
Expand All @@ -2648,25 +2745,36 @@ describe('KeyringController', () => {
data: {
accounts: ['0x123'],
},
metadata: {
id: '123',
name: '',
},
},
]);

await controller.submitPassword(password);

expect(controller.state.keyringsMetadata).toHaveLength(1);
expect(controller.state.keyrings).toStrictEqual([
{
type: KeyringTypes.hd,
accounts: ['0x123'],
metadata: {
id: '123',
name: '',
},
},
]);
},
);
});

it('should throw an error when the controller is instantiated with an existing `keyringsMetadata` with too many objects', async () => {
it('should generate new metadata when there is no metadata in the vault', async () => {
// @ts-expect-error HdKeyring is not yet compatible with Keyring type.
stubKeyringClassWithAccount(HdKeyring, '0x123');
await withController(
{
cacheEncryptionKey,
state: {
keyringsMetadata: [
{ id: '123', name: '' },
{ id: '456', name: '' },
],
vault: 'my vault',
},
skipVaultCreation: true,
Expand All @@ -2681,9 +2789,18 @@ describe('KeyringController', () => {
},
]);

await expect(controller.submitPassword(password)).rejects.toThrow(
KeyringControllerError.KeyringMetadataLengthMismatch,
);
await controller.submitPassword(password);

expect(controller.state.keyrings).toStrictEqual([
{
type: KeyringTypes.hd,
accounts: ['0x123'],
metadata: {
id: expect.any(String),
name: '',
},
},
]);
},
);
});
Expand Down Expand Up @@ -2930,7 +3047,7 @@ describe('KeyringController', () => {
it('should return seedphrase for a specific keyring', async () => {
await withController(async ({ controller }) => {
const seedPhrase = await controller.verifySeedPhrase(
controller.state.keyringsMetadata[0].id,
controller.state.keyrings[0].metadata.id,
);
expect(seedPhrase).toBeDefined();
});
Expand Down Expand Up @@ -2965,7 +3082,7 @@ describe('KeyringController', () => {
await withController(async ({ controller }) => {
await controller.addNewKeyring(KeyringTypes.simple, [privateKey]);

const keyringId = controller.state.keyringsMetadata[1].id;
const keyringId = controller.state.keyrings[1].metadata.id;
await expect(controller.verifySeedPhrase(keyringId)).rejects.toThrow(
KeyringControllerError.UnsupportedVerifySeedPhrase,
);
Expand Down Expand Up @@ -3073,7 +3190,7 @@ describe('KeyringController', () => {
const fn = jest.fn();
const selector = { type: KeyringTypes.hd };
const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0];
const metadata = controller.state.keyringsMetadata[0];
const { metadata } = controller.state.keyrings[0];

await controller.withKeyring(selector, fn);

Expand Down Expand Up @@ -3211,7 +3328,7 @@ describe('KeyringController', () => {
address: initialState.keyrings[0].accounts[0] as Hex,
};
const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0];
const metadata = controller.state.keyringsMetadata[0];
const { metadata } = controller.state.keyrings[0];

await controller.withKeyring(selector, fn);

Expand Down Expand Up @@ -3253,11 +3370,11 @@ describe('KeyringController', () => {

describe('when the keyring is selected by id', () => {
it('should call the given function with the selected keyring', async () => {
await withController(async ({ controller, initialState }) => {
await withController(async ({ controller }) => {
const fn = jest.fn();
const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0];
const selector = { id: initialState.keyringsMetadata[0].id };
const metadata = controller.state.keyringsMetadata[0];
const { metadata } = controller.state.keyrings[0];
const selector = { id: metadata.id };

await controller.withKeyring(selector, fn);

Expand All @@ -3268,15 +3385,15 @@ describe('KeyringController', () => {
it('should return the result of the function', async () => {
await withController(async ({ controller, initialState }) => {
const fn = async () => Promise.resolve('hello');
const selector = { id: initialState.keyringsMetadata[0].id };
const selector = { id: initialState.keyrings[0].metadata.id };

expect(await controller.withKeyring(selector, fn)).toBe('hello');
});
});

it('should throw an error if the callback returns the selected keyring', async () => {
await withController(async ({ controller, initialState }) => {
const selector = { id: initialState.keyringsMetadata[0].id };
const selector = { id: initialState.keyrings[0].metadata.id };

await expect(
controller.withKeyring(selector, async ({ keyring }) => {
Expand Down
Loading
Loading