Skip to content

test(KeyringController): mock encryptor upgrade #5943

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

test(KeyringController): mock encryptor upgrade #5943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
24c3bb8
(wip) new mock encryptor
matthiasgeihs Jun 9, 2025
0f191cf
cleanup
matthiasgeihs Jun 9, 2025
1f25ec5
adjust coverage threshold
matthiasgeihs Jun 10, 2025
709bd54
update changelog
matthiasgeihs Jun 10, 2025
79a9fe4
address review comments
matthiasgeihs Jun 11, 2025
867a525
cleanup comment
matthiasgeihs Jun 11, 2025
12f8275
Remove CHANGELOG entry
matthiasgeihs Jun 13, 2025
94ad779
remove obsolete comment
matthiasgeihs Jun 13, 2025
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
134 changes: 80 additions & 54 deletions packages/keyring-controller/src/KeyringController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ import {
keyringBuilderFactory,
} from './KeyringController';
import MockEncryptor, {
DECRYPTION_ERROR,
MOCK_ENCRYPTION_KEY,
SALT,
} from '../tests/mocks/mockEncryptor';
import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring';
import { MockKeyring } from '../tests/mocks/mockKeyring';
Expand Down Expand Up @@ -67,6 +69,8 @@ const uint8ArraySeed = new Uint8Array(
const privateKey =
'1e4e6a4c0c077f4ae8ddfbf372918e61dd0fb4a4cfa592cb16e7546d505e68fc';
const password = 'password123';
const freshVault =
'{"data":"{\\"tag\\":{\\"key\\":{\\"password\\":\\"password123\\",\\"salt\\":\\"salt\\"},\\"iv\\":\\"iv\\"},\\"value\\":[{\\"type\\":\\"HD Key Tree\\",\\"data\\":{\\"mnemonic\\":[119,97,114,114,105,111,114,32,108,97,110,103,117,97,103,101,32,106,111,107,101,32,98,111,110,117,115,32,117,110,102,97,105,114,32,97,114,116,105,115,116,32,107,97,110,103,97,114,111,111,32,99,105,114,99,108,101,32,101,120,112,97,110,100,32,104,111,112,101,32,109,105,100,100,108,101,32,103,97,117,103,101],\\"numberOfAccounts\\":1,\\"hdPath\\":\\"m/44\'/60\'/0\'/0\\"},\\"metadata\\":{\\"id\\":\\"01JXEFM7DAX2VJ0YFR4ESNY3GQ\\",\\"name\\":\\"\\"}}]}","iv":"iv","salt":"salt"}';

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

Expand Down Expand Up @@ -553,15 +557,13 @@ describe('KeyringController', () => {
await withController(
{ cacheEncryptionKey },
async ({ controller, initialState }) => {
const initialVault = controller.state.vault;
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.keyrings).toHaveLength(
Comment on lines 559 to 567
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this deletion still relevant after the mock encryptor refactor, or, do you think we can check in other ways if the new vault is the intended one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you add that, this is what you get
image
so the mnemonic and id are different. (as expected because mnenomic is randomnly generated in the first case and fixed in the second. not sure about the id, but probably similar case.)

feel free to make a suggestion how this check can be added back again (or at least the intention behind it)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I guess we could make it match if we made this test import the same SRP that was in-place before. But I would interpret the intent to be importing a new SRP, so them not matching seems appropriate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree that matching the new vault with the initial one doesn't make sense, though what I was proposing was to leverage the new mock encryptor capabilities to check if the seed in the new vault is the one imported

initialKeyrings.length,
);
Expand All @@ -577,7 +579,7 @@ describe('KeyringController', () => {
await withController(
{ cacheEncryptionKey },
async ({ controller, encryptor }) => {
const encryptSpy = jest.spyOn(encryptor, 'encrypt');
const encryptSpy = jest.spyOn(encryptor, 'encryptWithKey');
const serializedKeyring = await controller.withKeyring(
{ type: 'HD Key Tree' },
async ({ keyring }) => keyring.serialize(),
Expand All @@ -590,7 +592,8 @@ describe('KeyringController', () => {
currentSeedWord,
);

expect(encryptSpy).toHaveBeenCalledWith(password, [
const key = JSON.parse(MOCK_ENCRYPTION_KEY);
expect(encryptSpy).toHaveBeenCalledWith(key, [
{
data: serializedKeyring,
type: 'HD Key Tree',
Expand Down Expand Up @@ -1302,7 +1305,15 @@ describe('KeyringController', () => {
},
],
};
expect(controller.state).toStrictEqual(modifiedState);
const modifiedStateWithoutVault = {
...modifiedState,
vault: undefined,
};
const stateWithoutVault = {
...controller.state,
vault: undefined,
};
expect(stateWithoutVault).toStrictEqual(modifiedStateWithoutVault);
expect(importedAccountAddress).toBe(address);
});
});
Expand Down Expand Up @@ -1381,7 +1392,15 @@ describe('KeyringController', () => {
},
],
};
expect(controller.state).toStrictEqual(modifiedState);
const modifiedStateWithoutVault = {
...modifiedState,
vault: undefined,
};
const stateWithoutVault = {
...controller.state,
vault: undefined,
};
expect(stateWithoutVault).toStrictEqual(modifiedStateWithoutVault);
expect(importedAccountAddress).toBe(address);
});
});
Expand Down Expand Up @@ -2678,10 +2697,10 @@ describe('KeyringController', () => {
{
cacheEncryptionKey,
skipVaultCreation: true,
state: { vault: 'my vault' },
state: { vault: freshVault },
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
type: 'UnsupportedKeyring',
data: '0x1234',
Expand All @@ -2700,10 +2719,10 @@ describe('KeyringController', () => {
{
cacheEncryptionKey,
skipVaultCreation: true,
state: { vault: 'my vault' },
state: { vault: freshVault },
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
foo: 'bar',
},
Expand Down Expand Up @@ -2733,11 +2752,11 @@ describe('KeyringController', () => {
await withController(
{
cacheEncryptionKey,
state: { vault: 'my vault' },
state: { vault: freshVault },
skipVaultCreation: true,
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: {
Expand Down Expand Up @@ -2776,7 +2795,7 @@ describe('KeyringController', () => {
{
cacheEncryptionKey: true,
state: {
vault: 'my vault',
vault: freshVault,
},
skipVaultCreation: true,
},
Expand All @@ -2787,9 +2806,8 @@ describe('KeyringController', () => {
);
jest
.spyOn(encryptor, 'importKey')
// @ts-expect-error we are assigning a mock value
.mockResolvedValue('imported key');
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: {
Expand Down Expand Up @@ -2840,7 +2858,7 @@ describe('KeyringController', () => {
{
cacheEncryptionKey: false,
state: {
vault: 'my vault',
vault: freshVault,
},
skipVaultCreation: true,
},
Expand Down Expand Up @@ -2895,14 +2913,14 @@ describe('KeyringController', () => {
{
skipVaultCreation: true,
cacheEncryptionKey,
state: { vault: 'my vault' },
state: { vault: freshVault },
keyringBuilders: [keyringBuilderFactory(MockKeyring)],
},
async ({ controller, encryptor, messenger }) => {
const unlockListener = jest.fn();
messenger.subscribe('KeyringController:unlock', unlockListener);
jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false);
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: {},
Expand All @@ -2926,12 +2944,12 @@ describe('KeyringController', () => {
{
skipVaultCreation: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial vault creation was being skipped in submitPassword tests because the default withController behavior is to generate a vault and leave the controller unlocked. So when these tests get executed KeyringController will already be unlocked.

For some of these test cases, it shouldn't be a problem. Though for some of them, there's the risk of getting false positives

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initially i've changed it because the withController function didn't allow to setup with encryption key. i've changed that now, so will check if i can change it back in some cases. would be good to know in which cases it's problematic to have this skipped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's potentially problematic for any test case that aims to verify how unlocking works, since the controller is already unlocked before the tests.
e.g. if a test case asserts expect(controller.state.isUnlocked).toBe(true);, that would potentially be a false positive

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've changed skipVaultCreation back where i could.

tbh, i think it might be better to simply lock the controller in cases where you need a locked controller.
the issue i had with setting up the vault like this is that it depends on the implementation of the mock controller, so everytime this is changed, you need to change the fixed vault value that is used in tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh, I think it might be better to simply lock the controller in cases where you need a locked controller.

This may work in some cases, though you'd have to use another public function to test another one, which is ok, but better to avoid it if we can make the test simpler and more independent

the issue i had with setting up the vault like this is that it depends on the implementation of the mock controller

Perhaps you are referring to the encryptor? If so, that's the reason why we set up these mock responses:

jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
  {
    type: KeyringTypes.hd,
    data: '0x123',
  }
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. Yes. Mock encryptor

cacheEncryptionKey,
state: { vault: 'my vault' },
state: { vault: freshVault },
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false);
jest.spyOn(encryptor, 'encrypt').mockRejectedValue(new Error());
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: {
Expand All @@ -2955,13 +2973,13 @@ describe('KeyringController', () => {
{
skipVaultCreation: true,
cacheEncryptionKey,
state: { vault: 'my vault' },
state: { vault: freshVault },
keyringBuilders: [keyringBuilderFactory(MockKeyring)],
},
async ({ controller, encryptor, messenger }) => {
const unlockListener = jest.fn();
messenger.subscribe('KeyringController:unlock', unlockListener);
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: {},
Expand All @@ -2986,12 +3004,12 @@ describe('KeyringController', () => {
{
skipVaultCreation: true,
cacheEncryptionKey,
state: { vault: 'my vault' },
state: { vault: freshVault },
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false);
const encryptSpy = jest.spyOn(encryptor, 'encrypt');
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
const encryptSpy = jest.spyOn(encryptor, 'encryptWithKey');
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: {
Expand All @@ -3013,12 +3031,12 @@ describe('KeyringController', () => {
{
skipVaultCreation: true,
cacheEncryptionKey,
state: { vault: 'my vault' },
state: { vault: freshVault },
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true);
const encryptSpy = jest.spyOn(encryptor, 'encrypt');
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: {
Expand All @@ -3027,6 +3045,10 @@ describe('KeyringController', () => {
},
]);

// TODO actually this does trigger re-encryption. The catch is
// that this test is run with cacheEncryptionKey enabled, so
// `encryptWithKey` is being used instead of `encrypt`. Hence,
// the spy on `encrypt` doesn't trigger.
await controller.submitPassword(password);

expect(encryptSpy).not.toHaveBeenCalled();
Expand All @@ -3040,7 +3062,7 @@ describe('KeyringController', () => {
{
skipVaultCreation: true,
cacheEncryptionKey,
state: { vault: 'my vault' },
state: { vault: freshVault },
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false);
Expand All @@ -3066,7 +3088,7 @@ describe('KeyringController', () => {
{
skipVaultCreation: true,
cacheEncryptionKey,
state: { vault: 'my vault' },
state: { vault: freshVault },
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true);
Expand Down Expand Up @@ -3119,14 +3141,19 @@ describe('KeyringController', () => {
it('should throw error when using the wrong password', async () => {
await withController(
{
skipVaultCreation: true,
cacheEncryptionKey,
state: { vault: 'my vault' },
skipVaultCreation: true,
state: {
vault: freshVault,
// @ts-expect-error we want to force the controller to have an
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Hmm. I don't quite understand why we would need this here, as the encryptionSalt state is not used for anything other than detecting a mismatch between encryptionKey and vault (which is not relevant for this test, certainly because encryptionKey is not even present).

I see that it is a pre-existing widespread pattern in these tests though, so we can fix it later.

// encryption salt equal to the one in the vault
encryptionSalt: SALT,
},
},
async ({ controller }) => {
await expect(
controller.submitPassword('wrong password'),
).rejects.toThrow('Incorrect password.');
).rejects.toThrow(DECRYPTION_ERROR);
},
);
});
Expand Down Expand Up @@ -3154,14 +3181,14 @@ describe('KeyringController', () => {
cacheEncryptionKey: true,
skipVaultCreation: true,
state: {
vault: JSON.stringify({ data: '0x123', salt: 'my salt' }),
vault: freshVault,
// @ts-expect-error we want to force the controller to have an
// encryption salt equal to the one in the vault
encryptionSalt: 'my salt',
encryptionSalt: SALT,
},
},
async ({ controller, initialState, encryptor }) => {
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
type: 'UnsupportedKeyring',
data: '0x1234',
Expand All @@ -3188,19 +3215,15 @@ describe('KeyringController', () => {
cacheEncryptionKey: true,
skipVaultCreation: true,
state: {
vault: JSON.stringify({ data: '0x123', salt: 'my salt' }),
vault: freshVault,
// @ts-expect-error we want to force the controller to have an
// encryption salt equal to the one in the vault
encryptionSalt: 'my salt',
encryptionSalt: SALT,
},
},
async ({ controller, initialState, encryptor }) => {
const encryptWithKeySpy = jest.spyOn(encryptor, 'encryptWithKey');
jest
.spyOn(encryptor, 'importKey')
// @ts-expect-error we are assigning a mock value
.mockResolvedValue('imported key');
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: '0x123',
Expand All @@ -3213,18 +3236,21 @@ describe('KeyringController', () => {
);

expect(controller.state.isUnlocked).toBe(true);
expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [
{
type: KeyringTypes.hd,
data: {
accounts: ['0x123'],
},
metadata: {
id: expect.any(String),
name: '',
expect(encryptWithKeySpy).toHaveBeenCalledWith(
JSON.parse(MOCK_ENCRYPTION_KEY),
[
{
type: KeyringTypes.hd,
data: {
accounts: ['0x123'],
},
metadata: {
id: expect.any(String),
name: '',
},
},
},
]);
],
);
},
);
});
Expand All @@ -3233,7 +3259,7 @@ describe('KeyringController', () => {
await withController(
{ cacheEncryptionKey: true },
async ({ controller, initialState, encryptor }) => {
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([
{
foo: 'bar',
},
Expand Down
Loading
Loading