Skip to content
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

refactor!: addNewKeyring returns metadata instead of keyring #5372

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

refactor!: addNewKeyring returns metadata instead of keyring #5372

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1b33d3f
refactor!: `addNewKeyring` returns metadata instead of keyring
mikesposito Feb 21, 2025
d99a1be
update changelog
mikesposito Feb 21, 2025
1cb2d0a
add changelog entry
mikesposito Feb 21, 2025
774ed85
fix changelog entry
mikesposito Feb 21, 2025
57e869d
fix: refactor profile-sync-controller
mikesposito Feb 21, 2025
f4bee6b
fix coverage
mikesposito Feb 21, 2025
80e0e50
test: add case to check that metadata is readonly
mikesposito Feb 21, 2025
f67e921
Merge branch 'main' into mikesposito/add-new-keyring-return-type
mikesposito Feb 21, 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
3 changes: 3 additions & 0 deletions packages/keyring-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- **BREAKING:** `addNewKeyring` method now returns `Promise<KeyringMetadata>` instead of `Promise<unknown>` ([#5372](https://github.com/MetaMask/core/pull/5372))
- Consumers can use the returned `KeyringMetadata.id` to access the created keyring instance via `withKeyring`.
- **BREAKING:** `withKeyring` method now requires a callback argument of type `({ keyring: SelectedKeyring; metadata: KeyringMetadata }) => Promise<CallbackResult>` ([#5372](https://github.com/MetaMask/core/pull/5372))
- Bump `@metamask/keyring-api"` from `^17.0.0` to `^17.2.0` ([#5366](https://github.com/MetaMask/core/pull/5366))
- Bump `@metamask/keyring-internal-api` from `^4.0.1` to `^4.0.3` ([#5356](https://github.com/MetaMask/core/pull/5356)), ([#5366](https://github.com/MetaMask/core/pull/5366))

Expand Down
6 changes: 3 additions & 3 deletions packages/keyring-controller/jest.config.js
View file
Open in desktop
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.56,
branches: 93.64,
functions: 100,
lines: 98.73,
statements: 98.74,
lines: 98.76,
statements: 98.77,
},
},

Expand Down
193 changes: 109 additions & 84 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 @@ -336,9 +336,12 @@ describe('KeyringController', () => {
],
},
async ({ controller }) => {
const mockKeyring = (await controller.addNewKeyring(
await controller.addNewKeyring(MockShallowGetAccountsKeyring.type);
// TODO: This is a temporary workaround while `addNewAccountForKeyring` is not
// removed.
const mockKeyring = controller.getKeyringsByType(
MockShallowGetAccountsKeyring.type,
)) as Keyring<Json>;
)[0] as EthKeyring<Json>;

const addedAccountAddress =
await controller.addNewAccountForKeyring(mockKeyring);
Expand Down Expand Up @@ -434,6 +437,16 @@ describe('KeyringController', () => {
expect(controller.state.keyrings).toHaveLength(2);
});
});

it('should return a readonly object as metadata', async () => {
await withController(async ({ controller }) => {
const newMetadata = await controller.addNewKeyring(KeyringTypes.hd);

expect(() => {
newMetadata.name = 'new name';
}).toThrow(/Cannot assign to read only property 'name'/u);
});
});
});

describe('when there is no builder for the given type', () => {
Expand Down Expand Up @@ -491,7 +504,7 @@ describe('KeyringController', () => {
const encryptSpy = jest.spyOn(encryptor, 'encrypt');
const serializedKeyring = await controller.withKeyring(
{ type: 'HD Key Tree' },
async (keyring) => keyring.serialize(),
async ({ keyring }) => keyring.serialize(),
);
const currentSeedWord =
await controller.exportSeedPhrase(password);
Expand Down Expand Up @@ -541,16 +554,17 @@ describe('KeyringController', () => {

cacheEncryptionKey &&
it('should set encryptionKey and encryptionSalt in state', async () => {
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
withController({ cacheEncryptionKey }, async ({ controller }) => {
await controller.createNewVaultAndRestore(
password,
uint8ArraySeed,
);
expect(controller.state.encryptionKey).toBeDefined();
expect(controller.state.encryptionSalt).toBeDefined();
});
await withController(
{ cacheEncryptionKey },
async ({ controller }) => {
await controller.createNewVaultAndRestore(
password,
uint8ArraySeed,
);
expect(controller.state.encryptionKey).toBeDefined();
expect(controller.state.encryptionSalt).toBeDefined();
},
);
});
}),
);
Expand Down Expand Up @@ -623,11 +637,7 @@ describe('KeyringController', () => {
});

it('should throw error if the first account is not found on the keyring', async () => {
jest
.spyOn(HDKeyring.prototype, 'getAccounts')
// todo: keyring types are mismatched, this should be fixed in they keyrings themselves
// @ts-expect-error keyring types are mismatched
.mockResolvedValue([]);
jest.spyOn(HDKeyring.prototype, 'getAccounts').mockReturnValue([]);
await withController(
{ cacheEncryptionKey, skipVaultCreation: true },
async ({ controller }) => {
Expand Down Expand Up @@ -2179,9 +2189,9 @@ describe('KeyringController', () => {
await withController(
{ keyringBuilders: [keyringBuilderFactory(MockErc4337Keyring)] },
async ({ controller }) => {
const mockKeyring = (await controller.addNewKeyring(
const { id } = await controller.addNewKeyring(
MockErc4337Keyring.type,
)) as EthKeyring<Json>;
);
const baseUserOp = {
callData: '0x7064',
initCode: '0x22ff',
Expand All @@ -2202,24 +2212,25 @@ describe('KeyringController', () => {
data: '0x7064',
},
];
await controller.withKeyring({ id }, async ({ keyring }) => {
jest
.spyOn(keyring, 'prepareUserOperation')
.mockResolvedValueOnce(baseUserOp);

jest
.spyOn(mockKeyring, 'prepareUserOperation')
.mockResolvedValueOnce(baseUserOp);

const result = await controller.prepareUserOperation(
address,
baseTxs,
executionContext,
);
const result = await controller.prepareUserOperation(
address,
baseTxs,
executionContext,
);

expect(result).toStrictEqual(baseUserOp);
expect(mockKeyring.prepareUserOperation).toHaveBeenCalledTimes(1);
expect(mockKeyring.prepareUserOperation).toHaveBeenCalledWith(
address,
baseTxs,
executionContext,
);
expect(result).toStrictEqual(baseUserOp);
expect(keyring.prepareUserOperation).toHaveBeenCalledTimes(1);
expect(keyring.prepareUserOperation).toHaveBeenCalledWith(
address,
baseTxs,
executionContext,
);
});
},
);
});
Expand Down Expand Up @@ -2272,9 +2283,9 @@ describe('KeyringController', () => {
await withController(
{ keyringBuilders: [keyringBuilderFactory(MockErc4337Keyring)] },
async ({ controller }) => {
const mockKeyring = (await controller.addNewKeyring(
const { id } = await controller.addNewKeyring(
MockErc4337Keyring.type,
)) as EthKeyring<Json>;
);
const userOp = {
sender: '0x4584d2B4905087A100420AFfCe1b2d73fC69B8E4',
nonce: '0x1',
Expand All @@ -2291,23 +2302,25 @@ describe('KeyringController', () => {
const patch = {
paymasterAndData: '0x1234',
};
jest
.spyOn(mockKeyring, 'patchUserOperation')
.mockResolvedValueOnce(patch);
await controller.withKeyring({ id }, async ({ keyring }) => {
jest
.spyOn(keyring, 'patchUserOperation')
.mockResolvedValueOnce(patch);

const result = await controller.patchUserOperation(
address,
userOp,
executionContext,
);
const result = await controller.patchUserOperation(
address,
userOp,
executionContext,
);

expect(result).toStrictEqual(patch);
expect(mockKeyring.patchUserOperation).toHaveBeenCalledTimes(1);
expect(mockKeyring.patchUserOperation).toHaveBeenCalledWith(
address,
userOp,
executionContext,
);
expect(result).toStrictEqual(patch);
expect(keyring.patchUserOperation).toHaveBeenCalledTimes(1);
expect(keyring.patchUserOperation).toHaveBeenCalledWith(
address,
userOp,
executionContext,
);
});
},
);
});
Expand Down Expand Up @@ -2384,9 +2397,9 @@ describe('KeyringController', () => {
await withController(
{ keyringBuilders: [keyringBuilderFactory(MockErc4337Keyring)] },
async ({ controller }) => {
const mockKeyring = (await controller.addNewKeyring(
const { id } = await controller.addNewKeyring(
MockErc4337Keyring.type,
)) as EthKeyring<Json>;
);
const userOp = {
sender: '0x4584d2B4905087A100420AFfCe1b2d73fC69B8E4',
nonce: '0x1',
Expand All @@ -2401,23 +2414,25 @@ describe('KeyringController', () => {
signature: '0x',
};
const signature = '0x1234';
jest
.spyOn(mockKeyring, 'signUserOperation')
.mockResolvedValueOnce(signature);
await controller.withKeyring({ id }, async ({ keyring }) => {
jest
.spyOn(keyring, 'signUserOperation')
.mockResolvedValueOnce(signature);

const result = await controller.signUserOperation(
address,
userOp,
executionContext,
);
const result = await controller.signUserOperation(
address,
userOp,
executionContext,
);

expect(result).toStrictEqual(signature);
expect(mockKeyring.signUserOperation).toHaveBeenCalledTimes(1);
expect(mockKeyring.signUserOperation).toHaveBeenCalledWith(
address,
userOp,
executionContext,
);
expect(result).toStrictEqual(signature);
expect(keyring.signUserOperation).toHaveBeenCalledTimes(1);
expect(keyring.signUserOperation).toHaveBeenCalledWith(
address,
userOp,
executionContext,
);
});
},
);
});
Expand Down Expand Up @@ -2874,14 +2889,15 @@ describe('KeyringController', () => {
it('should rollback if an error is thrown', async () => {
await withController(async ({ controller, initialState }) => {
const selector = { type: KeyringTypes.hd };
const fn = async (keyring: EthKeyring<Json>) => {
const fn = async ({ keyring }: { keyring: EthKeyring<Json> }) => {
await keyring.addAccounts(1);
throw new Error('Oops');
};

await expect(controller.withKeyring(selector, fn)).rejects.toThrow(
'Oops',
);

expect(controller.state.keyrings[0].accounts).toHaveLength(1);
expect(await controller.getAccounts()).toStrictEqual(
initialState.keyrings[0].accounts,
Expand All @@ -2895,10 +2911,11 @@ describe('KeyringController', () => {
const fn = jest.fn();
const selector = { type: KeyringTypes.hd };
const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0];
const metadata = controller.state.keyringsMetadata[0];

await controller.withKeyring(selector, fn);

expect(fn).toHaveBeenCalledWith(keyring);
expect(fn).toHaveBeenCalledWith({ keyring, metadata });
});
});

Expand All @@ -2916,7 +2933,7 @@ describe('KeyringController', () => {
await expect(
controller.withKeyring(
{ type: KeyringTypes.hd },
async (keyring) => {
async ({ keyring }) => {
return keyring;
},
),
Expand Down Expand Up @@ -2964,10 +2981,11 @@ describe('KeyringController', () => {
address: initialState.keyrings[0].accounts[0] as Hex,
};
const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0];
const metadata = controller.state.keyringsMetadata[0];

await controller.withKeyring(selector, fn);

expect(fn).toHaveBeenCalledWith(keyring);
expect(fn).toHaveBeenCalledWith({ keyring, metadata });
});
});

Expand Down Expand Up @@ -3009,10 +3027,11 @@ describe('KeyringController', () => {
const fn = jest.fn();
const keyring = controller.getKeyringsByType(KeyringTypes.hd)[0];
const selector = { id: initialState.keyringsMetadata[0].id };
const metadata = controller.state.keyringsMetadata[0];

await controller.withKeyring(selector, fn);

expect(fn).toHaveBeenCalledWith(keyring);
expect(fn).toHaveBeenCalledWith({ keyring, metadata });
});
});

Expand All @@ -3030,8 +3049,8 @@ describe('KeyringController', () => {
const selector = { id: initialState.keyringsMetadata[0].id };

await expect(
controller.withKeyring(selector, async (selectedKeyring) => {
return selectedKeyring;
controller.withKeyring(selector, async ({ keyring }) => {
return keyring;
}),
).rejects.toThrow(KeyringControllerError.UnsafeDirectKeyringAccess);
});
Expand Down Expand Up @@ -3760,15 +3779,21 @@ describe('KeyringController', () => {
'KeyringController:qrKeyringStateChange',
listener,
);
const qrKeyring = (await signProcessKeyringController.addNewKeyring(
const { id } = await signProcessKeyringController.addNewKeyring(
KeyringTypes.qr,
)) as QRKeyring;
);

qrKeyring.getMemStore().updateState({
sync: {
reading: true,
await signProcessKeyringController.withKeyring(
{ id },
// @ts-expect-error QRKeyring is not yet compatible with Keyring type.
async ({ keyring }: { keyring: QRKeyring }) => {
keyring.getMemStore().updateState({
sync: {
reading: true,
},
});
},
});
);

expect(listener).toHaveBeenCalledTimes(1);
});
Expand Down Expand Up @@ -4126,7 +4151,7 @@ describe('KeyringController', () => {
const actionReturnValue = await messenger.call(
'KeyringController:withKeyring',
{ type: MockKeyring.type },
async (keyring) => {
async ({ keyring }) => {
expect(keyring.type).toBe(MockKeyring.type);
return keyring.type;
},
Expand Down
Loading
Loading