diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 6b2daaf262..e870f4914b 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -3,7 +3,10 @@ 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 { keyringBuilderFactory } from '@metamask/eth-keyring-controller'; +import { + KeyringControllerError, + keyringBuilderFactory, +} from '@metamask/eth-keyring-controller'; import { normalize, recoverPersonalSignature, @@ -13,6 +16,7 @@ import { } from '@metamask/eth-sig-util'; import type { EthKeyring } from '@metamask/keyring-api'; import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english'; +import type { KeyringClass } from '@metamask/utils'; import { isValidHexAddress, type Hex, @@ -27,7 +31,9 @@ import MockEncryptor, { MOCK_ENCRYPTION_KEY, } from '../tests/mocks/mockEncryptor'; import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring'; +import { MockKeyring } from '../tests/mocks/mockKeyring'; import MockShallowGetAccountsKeyring from '../tests/mocks/mockShallowGetAccountsKeyring'; +import { buildMockTransaction } from '../tests/mocks/mockTransaction'; import type { KeyringControllerEvents, KeyringControllerMessenger, @@ -553,49 +559,68 @@ describe('KeyringController', () => { }); describe('exportAccount', () => { - describe('when correct password is provided', () => { - describe('when correct account is provided', () => { - it('should export account', async () => { - await withController(async ({ controller, initialState }) => { - const account = initialState.keyrings[0].accounts[0]; - const newPrivateKey = await controller.exportAccount( - password, - account, - ); - expect(newPrivateKey).not.toBe(''); + describe('when the keyring for the given address supports exportAccount', () => { + describe('when correct password is provided', () => { + describe('when correct account is provided', () => { + it('should export account', async () => { + await withController(async ({ controller, initialState }) => { + const account = initialState.keyrings[0].accounts[0]; + const newPrivateKey = await controller.exportAccount( + password, + account, + ); + expect(newPrivateKey).not.toBe(''); + }); + }); + }); + + describe('when wrong account is provided', () => { + it('should throw error', async () => { + await withController(async ({ controller }) => { + await expect( + controller.exportAccount(password, ''), + ).rejects.toThrow( + 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', + ); + }); }); }); }); - describe('when wrong account is provided', () => { + describe('when wrong password is provided', () => { it('should throw error', async () => { - await withController(async ({ controller }) => { - await expect( - controller.exportAccount(password, ''), - ).rejects.toThrow( - 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', - ); - }); + await withController( + async ({ controller, initialState, encryptor }) => { + const account = initialState.keyrings[0].accounts[0]; + sinon + .stub(encryptor, 'decrypt') + .rejects(new Error('Invalid password')); + + await expect( + controller.exportAccount('', account), + ).rejects.toThrow('Invalid password'); + + await expect( + controller.exportAccount('JUNK_VALUE', account), + ).rejects.toThrow('Invalid password'); + }, + ); }); }); }); - describe('when wrong password is provided', () => { + describe('when the keyring for the given address does not support exportAccount', () => { it('should throw error', async () => { + const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19'; + stubKeyringClassWithAccount(MockKeyring, address); await withController( - async ({ controller, initialState, encryptor }) => { - const account = initialState.keyrings[0].accounts[0]; - sinon - .stub(encryptor, 'decrypt') - .rejects(new Error('Invalid password')); - - await expect(controller.exportAccount('', account)).rejects.toThrow( - 'Invalid password', - ); + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); await expect( - controller.exportAccount('JUNK_VALUE', account), - ).rejects.toThrow('Invalid password'); + controller.exportAccount(password, address), + ).rejects.toThrow(KeyringControllerError.UnsupportedExportAccount); }, ); }); @@ -613,67 +638,117 @@ describe('KeyringController', () => { }); describe('getEncryptionPublicKey', () => { - it('should return the correct encryption public key', async () => { - await withController(async ({ controller }) => { - const { importedAccountAddress } = - await controller.importAccountWithStrategy( - AccountImportStrategy.privateKey, - [privateKey], + describe('when the keyring for the given address supports getEncryptionPublicKey', () => { + it('should return the correct encryption public key', async () => { + await withController(async ({ controller }) => { + const { importedAccountAddress } = + await controller.importAccountWithStrategy( + AccountImportStrategy.privateKey, + [privateKey], + ); + + const encryptionPublicKey = await controller.getEncryptionPublicKey( + importedAccountAddress, ); - const encryptionPublicKey = await controller.getEncryptionPublicKey( - importedAccountAddress, - ); + expect(encryptionPublicKey).toBe( + 'ZfKqt4HSy4tt9/WvqP3QrnzbIS04cnV//BhksKbLgVA=', + ); + }); + }); + }); - expect(encryptionPublicKey).toBe( - 'ZfKqt4HSy4tt9/WvqP3QrnzbIS04cnV//BhksKbLgVA=', + describe('when the keyring for the given address does not support getEncryptionPublicKey', () => { + it('should throw error', async () => { + const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19'; + stubKeyringClassWithAccount(MockKeyring, address); + await withController( + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + + await expect( + controller.getEncryptionPublicKey(address), + ).rejects.toThrow( + KeyringControllerError.UnsupportedGetEncryptionPublicKey, + ); + }, ); }); }); }); describe('decryptMessage', () => { - it('should successfully decrypt a message with valid parameters and return the raw decryption result', async () => { - await withController(async ({ controller }) => { - const { importedAccountAddress } = - await controller.importAccountWithStrategy( - AccountImportStrategy.privateKey, - [privateKey], - ); - const message = 'Hello, encrypted world!'; - const encryptedMessage = encrypt({ - publicKey: await controller.getEncryptionPublicKey( - importedAccountAddress, - ), - data: message, - version: 'x25519-xsalsa20-poly1305', - }); + describe('when the keyring for the given address supports decryptMessage', () => { + it('should successfully decrypt a message with valid parameters and return the raw decryption result', async () => { + await withController(async ({ controller }) => { + const { importedAccountAddress } = + await controller.importAccountWithStrategy( + AccountImportStrategy.privateKey, + [privateKey], + ); + const message = 'Hello, encrypted world!'; + const encryptedMessage = encrypt({ + publicKey: await controller.getEncryptionPublicKey( + importedAccountAddress, + ), + data: message, + version: 'x25519-xsalsa20-poly1305', + }); - const messageParams = { - from: importedAccountAddress, - data: encryptedMessage, - }; + const messageParams = { + from: importedAccountAddress, + data: encryptedMessage, + }; + + const result = await controller.decryptMessage(messageParams); - const result = await controller.decryptMessage(messageParams); + expect(result).toBe(message); + }); + }); + + it("should throw an error if the 'from' parameter is not a valid account address", async () => { + await withController(async ({ controller }) => { + const messageParams = { + from: 'invalid address', + data: { + version: '1.0', + nonce: '123456', + ephemPublicKey: '0xabcdef1234567890', + ciphertext: '0xabcdef1234567890', + }, + }; - expect(result).toBe(message); + await expect( + controller.decryptMessage(messageParams), + ).rejects.toThrow( + 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', + ); + }); }); }); - it("should throw an error if the 'from' parameter is not a valid account address", async () => { - await withController(async ({ controller }) => { - const messageParams = { - from: 'invalid address', - data: { - version: '1.0', - nonce: '123456', - ephemPublicKey: '0xabcdef1234567890', - ciphertext: '0xabcdef1234567890', - }, - }; + describe('when the keyring for the given address does not support decryptMessage', () => { + it('should throw error', async () => { + const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19'; + stubKeyringClassWithAccount(MockKeyring, address); + await withController( + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); - await expect(controller.decryptMessage(messageParams)).rejects.toThrow( - 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', + await expect( + controller.decryptMessage({ + from: address, + data: { + version: '1.0', + nonce: '123456', + ephemPublicKey: '0xabcdef1234567890', + ciphertext: '0xabcdef1234567890', + }, + }), + ).rejects.toThrow(KeyringControllerError.UnsupportedDecryptMessage); + }, ); }); }); @@ -922,189 +997,439 @@ describe('KeyringController', () => { }); describe('removeAccount', () => { - /** - * If there is only HD Key Tree keyring with 1 account and removeAccount is called passing that account - * It deletes keyring object also from state - not sure if this is correct behavior. - * https://github.com/MetaMask/core/issues/801 - */ - 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] as Hex; - await controller.removeAccount(account); - expect(controller.state.keyrings).toHaveLength(0); + describe('when the keyring for the given address supports removeAccount', () => { + /** + * If there is only HD Key Tree keyring with 1 account and removeAccount is called passing that account + * It deletes keyring object also from state - not sure if this is correct behavior. + * https://github.com/MetaMask/core/issues/801 + */ + 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] as Hex; + await controller.removeAccount(account); + expect(controller.state.keyrings).toHaveLength(0); + }); }); - }); - it('should remove account', async () => { - await withController(async ({ controller, initialState }) => { - await controller.importAccountWithStrategy( - AccountImportStrategy.privateKey, - [privateKey], - ); - await controller.removeAccount( - '0x51253087e6f8358b5f10c0a94315d69db3357859', - ); - expect(controller.state).toStrictEqual(initialState); + it('should remove account', async () => { + await withController(async ({ controller, initialState }) => { + await controller.importAccountWithStrategy( + AccountImportStrategy.privateKey, + [privateKey], + ); + await controller.removeAccount( + '0x51253087e6f8358b5f10c0a94315d69db3357859', + ); + expect(controller.state).toStrictEqual(initialState); + }); }); - }); - it('should emit `accountRemoved` event', async () => { - await withController(async ({ controller, messenger }) => { - await controller.importAccountWithStrategy( - AccountImportStrategy.privateKey, - [privateKey], - ); - const listener = sinon.spy(); - messenger.subscribe('KeyringController:accountRemoved', listener); + it('should emit `accountRemoved` event', async () => { + await withController(async ({ controller, messenger }) => { + await controller.importAccountWithStrategy( + AccountImportStrategy.privateKey, + [privateKey], + ); + const listener = sinon.spy(); + messenger.subscribe('KeyringController:accountRemoved', listener); - const removedAccount = '0x51253087e6f8358b5f10c0a94315d69db3357859'; - await controller.removeAccount(removedAccount); + const removedAccount = '0x51253087e6f8358b5f10c0a94315d69db3357859'; + await controller.removeAccount(removedAccount); - expect(listener.calledWith(removedAccount)).toBe(true); + expect(listener.calledWith(removedAccount)).toBe(true); + }); }); - }); - it('should not remove account if wrong address is provided', async () => { - await withController(async ({ controller }) => { - await controller.importAccountWithStrategy( - AccountImportStrategy.privateKey, - [privateKey], - ); + it('should not remove account if wrong address is provided', async () => { + await withController(async ({ controller }) => { + await controller.importAccountWithStrategy( + AccountImportStrategy.privateKey, + [privateKey], + ); - 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( + '0x0000000000000000000000000000000000000000', + ), + ).rejects.toThrow( + 'KeyringController - No keyring found. Error info: There are keyrings, but none match the address', + ); - await expect(controller.removeAccount('0xDUMMY_INPUT')).rejects.toThrow( - 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', + await expect( + controller.removeAccount('0xDUMMY_INPUT'), + ).rejects.toThrow( + 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', + ); + }); + }); + }); + + describe('when the keyring for the given address does not support removeAccount', () => { + it('should throw error', async () => { + const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19'; + stubKeyringClassWithAccount(MockKeyring, address); + await withController( + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + + await expect(controller.removeAccount(address)).rejects.toThrow( + KeyringControllerError.UnsupportedRemoveAccount, + ); + }, ); }); }); }); describe('signMessage', () => { - it('should sign message', async () => { - await withController(async ({ controller, initialState }) => { - const data = - '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0'; - const account = initialState.keyrings[0].accounts[0]; - const signature = await controller.signMessage({ - data, - from: account, + describe('when the keyring for the given address supports signMessage', () => { + it('should sign message', async () => { + await withController(async ({ controller, initialState }) => { + const data = + '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0'; + const account = initialState.keyrings[0].accounts[0]; + const signature = await controller.signMessage({ + data, + from: account, + }); + expect(signature).not.toBe(''); }); - expect(signature).not.toBe(''); }); - }); - it('should not sign message if empty data is passed', async () => { - await withController(async ({ controller, initialState }) => { - await expect(() => - controller.signMessage({ - data: '', - from: initialState.keyrings[0].accounts[0], - }), - ).toThrow("Can't sign an empty message"); + it('should not sign message if empty data is passed', async () => { + await withController(async ({ controller, initialState }) => { + await expect(() => + controller.signMessage({ + data: '', + from: initialState.keyrings[0].accounts[0], + }), + ).toThrow("Can't sign an empty message"); + }); + }); + + it('should not sign message if from account is not passed', async () => { + await withController(async ({ controller }) => { + await expect( + controller.signMessage({ + data: '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0', + from: '', + }), + ).rejects.toThrow( + 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', + ); + }); }); }); - it('should not sign message if from account is not passed', async () => { - await withController(async ({ controller }) => { - await expect( - controller.signMessage({ - data: '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0', - from: '', - }), - ).rejects.toThrow( - 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', + describe('when the keyring for the given address does not support signMessage', () => { + it('should throw error', async () => { + const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19'; + stubKeyringClassWithAccount(MockKeyring, address); + await withController( + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + const inputParams = { + from: address, + data: '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0', + origin: 'https://metamask.github.io', + }; + + await expect(controller.signMessage(inputParams)).rejects.toThrow( + KeyringControllerError.UnsupportedSignMessage, + ); + }, ); }); }); }); describe('signPersonalMessage', () => { - it('should sign personal message', async () => { - await withController(async ({ controller, initialState }) => { - const data = bufferToHex(Buffer.from('Hello from test', 'utf8')); - const account = initialState.keyrings[0].accounts[0]; - const signature = await controller.signPersonalMessage({ - data, - from: account, + describe('when the keyring for the given address supports signPersonalMessage', () => { + it('should sign personal message', async () => { + await withController(async ({ controller, initialState }) => { + const data = bufferToHex(Buffer.from('Hello from test', 'utf8')); + const account = initialState.keyrings[0].accounts[0]; + const signature = await controller.signPersonalMessage({ + data, + from: account, + }); + const recovered = recoverPersonalSignature({ data, signature }); + expect(account).toBe(recovered); }); - const recovered = recoverPersonalSignature({ data, signature }); - expect(account).toBe(recovered); }); - }); - /** - * signPersonalMessage does not fail for empty data value - * https://github.com/MetaMask/core/issues/799 - */ - it('should sign personal message even if empty data is passed', async () => { - await withController(async ({ controller, initialState }) => { - const account = initialState.keyrings[0].accounts[0]; - const signature = await controller.signPersonalMessage({ - data: '', - from: account, + /** + * signPersonalMessage does not fail for empty data value + * https://github.com/MetaMask/core/issues/799 + */ + it('should sign personal message even if empty data is passed', async () => { + await withController(async ({ controller, initialState }) => { + const account = initialState.keyrings[0].accounts[0]; + const signature = await controller.signPersonalMessage({ + data: '', + from: account, + }); + const recovered = recoverPersonalSignature({ data: '', signature }); + expect(account).toBe(recovered); + }); + }); + + it('should not sign personal message if from account is not passed', async () => { + await withController(async ({ controller }) => { + await expect( + controller.signPersonalMessage({ + data: '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0', + from: '', + }), + ).rejects.toThrow( + 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', + ); }); - const recovered = recoverPersonalSignature({ data: '', signature }); - expect(account).toBe(recovered); }); }); - it('should not sign personal message if from account is not passed', async () => { - await withController(async ({ controller }) => { - await expect( - controller.signPersonalMessage({ - data: '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0', - from: '', - }), - ).rejects.toThrow( - 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', + describe('when the keyring for the given address does not support signPersonalMessage', () => { + it('should throw error', async () => { + const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19'; + stubKeyringClassWithAccount(MockKeyring, address); + await withController( + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + const inputParams = { + from: address, + data: '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0', + origin: 'https://metamask.github.io', + }; + + await expect( + controller.signPersonalMessage(inputParams), + ).rejects.toThrow( + KeyringControllerError.UnsupportedSignPersonalMessage, + ); + }, ); }); }); }); 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 = [ - { - name: 'Message', - type: 'string', - value: 'Hi, Alice!', - }, - { - name: 'A number', - type: 'uint32', - value: '1337', - }, - ]; + describe('when the keyring for the given address supports 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 = [ + { + name: 'Message', + type: 'string', + value: 'Hi, Alice!', + }, + { + name: 'A number', + type: 'uint32', + value: '1337', + }, + ]; + const account = initialState.keyrings[0].accounts[0]; + await expect( + controller.signTypedMessage( + { data: typedMsgParams, from: account }, + 'junk' as SignTypedDataVersion, + ), + ).rejects.toThrow( + "Keyring Controller signTypedMessage: Error: Unexpected signTypedMessage version: 'junk'", + ); + }, + ); + }); + + 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 = [ + { + name: 'Message', + type: 'string', + value: 'Hi, Alice!', + }, + { + name: 'A number', + type: 'uint32', + value: '1337', + }, + ]; + const account = initialState.keyrings[0].accounts[0]; + const signature = await controller.signTypedMessage( + { data: typedMsgParams, from: account }, + SignTypedDataVersion.V1, + ); + const recovered = recoverTypedSignature({ + data: typedMsgParams, + signature, + version: SignTypedDataVersion.V1, + }); + expect(account).toBe(recovered); + }, + ); + }); + + 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 = { + domain: { + chainId: 1, + name: 'Ether Mail', + verifyingContract: '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC', + version: '1', + }, + message: { + contents: 'Hello, Bob!', + from: { + name: 'Cow', + wallet: '0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826', + }, + to: { + name: 'Bob', + wallet: '0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB', + }, + }, + primaryType: 'Mail' as const, + types: { + EIP712Domain: [ + { name: 'name', type: 'string' }, + { name: 'version', type: 'string' }, + { name: 'chainId', type: 'uint256' }, + { name: 'verifyingContract', type: 'address' }, + ], + Mail: [ + { name: 'from', type: 'Person' }, + { name: 'to', type: 'Person' }, + { name: 'contents', type: 'string' }, + ], + Person: [ + { name: 'name', type: 'string' }, + { name: 'wallet', type: 'address' }, + ], + }, + }; + const account = initialState.keyrings[0].accounts[0]; + const signature = await controller.signTypedMessage( + { data: JSON.stringify(msgParams), from: account }, + SignTypedDataVersion.V3, + ); + const recovered = recoverTypedSignature({ + data: msgParams, + signature, + version: SignTypedDataVersion.V3, + }); + expect(account).toBe(recovered); + }, + ); + }); + + 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 = { + domain: { + chainId: 1, + name: 'Ether Mail', + verifyingContract: '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC', + version: '1', + }, + message: { + contents: 'Hello, Bob!', + from: { + name: 'Cow', + wallets: [ + '0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826', + '0xDeaDbeefdEAdbeefdEadbEEFdeadbeEFdEaDbeeF', + ], + }, + to: [ + { + name: 'Bob', + wallets: [ + '0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB', + '0xB0BdaBea57B0BDABeA57b0bdABEA57b0BDabEa57', + '0xB0B0b0b0b0b0B000000000000000000000000000', + ], + }, + ], + }, + primaryType: 'Mail' as const, + types: { + EIP712Domain: [ + { name: 'name', type: 'string' }, + { name: 'version', type: 'string' }, + { name: 'chainId', type: 'uint256' }, + { name: 'verifyingContract', type: 'address' }, + ], + Group: [ + { name: 'name', type: 'string' }, + { name: 'members', type: 'Person[]' }, + ], + Mail: [ + { name: 'from', type: 'Person' }, + { name: 'to', type: 'Person[]' }, + { name: 'contents', type: 'string' }, + ], + Person: [ + { name: 'name', type: 'string' }, + { name: 'wallets', type: 'address[]' }, + ], + }, + }; + + const account = initialState.keyrings[0].accounts[0]; + const signature = await controller.signTypedMessage( + { data: JSON.stringify(msgParams), from: account }, + SignTypedDataVersion.V4, + ); + const recovered = recoverTypedSignature({ + data: msgParams, + signature, + version: SignTypedDataVersion.V4, + }); + expect(account).toBe(recovered); + }, + ); + }); + + it('should fail when sign typed message format is wrong', async () => { + await withController(async ({ controller, initialState }) => { + const msgParams = [{}]; const account = initialState.keyrings[0].accounts[0]; + await expect( controller.signTypedMessage( - { data: typedMsgParams, from: account }, - 'junk' as SignTypedDataVersion, + { data: msgParams, from: account }, + SignTypedDataVersion.V1, ), - ).rejects.toThrow( - "Keyring Controller signTypedMessage: Error: Unexpected signTypedMessage version: 'junk'", - ); - }, - ); - }); + ).rejects.toThrow('Keyring Controller signTypedMessage:'); - 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 }) => { + await expect( + controller.signTypedMessage( + { data: msgParams, from: account }, + SignTypedDataVersion.V3, + ), + ).rejects.toThrow('Keyring Controller signTypedMessage:'); + }); + }); + + it('should fail in signing message when from address is not provided', async () => { + await withController(async ({ controller }) => { const typedMsgParams = [ { name: 'Message', @@ -1117,224 +1442,56 @@ describe('KeyringController', () => { value: '1337', }, ]; - const account = initialState.keyrings[0].accounts[0]; - const signature = await controller.signTypedMessage( - { data: typedMsgParams, from: account }, - SignTypedDataVersion.V1, - ); - const recovered = recoverTypedSignature({ - data: typedMsgParams, - signature, - version: SignTypedDataVersion.V1, - }); - expect(account).toBe(recovered); - }, - ); - }); - - 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 = { - domain: { - chainId: 1, - name: 'Ether Mail', - verifyingContract: '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC', - version: '1', - }, - message: { - contents: 'Hello, Bob!', - from: { - name: 'Cow', - wallet: '0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826', - }, - to: { - name: 'Bob', - wallet: '0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB', - }, - }, - primaryType: 'Mail' as const, - types: { - EIP712Domain: [ - { name: 'name', type: 'string' }, - { name: 'version', type: 'string' }, - { name: 'chainId', type: 'uint256' }, - { name: 'verifyingContract', type: 'address' }, - ], - Mail: [ - { name: 'from', type: 'Person' }, - { name: 'to', type: 'Person' }, - { name: 'contents', type: 'string' }, - ], - Person: [ - { name: 'name', type: 'string' }, - { name: 'wallet', type: 'address' }, - ], - }, - }; - const account = initialState.keyrings[0].accounts[0]; - const signature = await controller.signTypedMessage( - { data: JSON.stringify(msgParams), from: account }, - SignTypedDataVersion.V3, - ); - const recovered = recoverTypedSignature({ - data: msgParams, - signature, - version: SignTypedDataVersion.V3, - }); - expect(account).toBe(recovered); - }, - ); + await expect( + controller.signTypedMessage( + { data: typedMsgParams, from: '' }, + SignTypedDataVersion.V1, + ), + ).rejects.toThrow(/^Keyring Controller signTypedMessage:/u); + }); + }); }); - 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 = { - domain: { - chainId: 1, - name: 'Ether Mail', - verifyingContract: '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC', - version: '1', - }, - message: { - contents: 'Hello, Bob!', - from: { - name: 'Cow', - wallets: [ - '0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826', - '0xDeaDbeefdEAdbeefdEadbEEFdeadbeEFdEaDbeeF', - ], - }, - to: [ + describe('when the keyring for the given address does not support signTypedMessage', () => { + it('should throw error', async () => { + const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19'; + stubKeyringClassWithAccount(MockKeyring, address); + await withController( + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + const inputParams = { + from: address, + data: [ { - name: 'Bob', - wallets: [ - '0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB', - '0xB0BdaBea57B0BDABeA57b0bdABEA57b0BDabEa57', - '0xB0B0b0b0b0b0B000000000000000000000000000', - ], + type: 'string', + name: 'Message', + value: 'Hi, Alice!', + }, + { + type: 'uint32', + name: 'A number', + value: '1337', }, ], - }, - primaryType: 'Mail' as const, - types: { - EIP712Domain: [ - { name: 'name', type: 'string' }, - { name: 'version', type: 'string' }, - { name: 'chainId', type: 'uint256' }, - { name: 'verifyingContract', type: 'address' }, - ], - Group: [ - { name: 'name', type: 'string' }, - { name: 'members', type: 'Person[]' }, - ], - Mail: [ - { name: 'from', type: 'Person' }, - { name: 'to', type: 'Person[]' }, - { name: 'contents', type: 'string' }, - ], - Person: [ - { name: 'name', type: 'string' }, - { name: 'wallets', type: 'address[]' }, - ], - }, - }; - - const account = initialState.keyrings[0].accounts[0]; - const signature = await controller.signTypedMessage( - { data: JSON.stringify(msgParams), from: account }, - SignTypedDataVersion.V4, - ); - const recovered = recoverTypedSignature({ - data: msgParams, - signature, - version: SignTypedDataVersion.V4, - }); - expect(account).toBe(recovered); - }, - ); - }); - - it('should fail when sign typed message format is wrong', async () => { - await withController(async ({ controller, initialState }) => { - const msgParams = [{}]; - const account = initialState.keyrings[0].accounts[0]; - - await expect( - controller.signTypedMessage( - { data: msgParams, from: account }, - SignTypedDataVersion.V1, - ), - ).rejects.toThrow('Keyring Controller signTypedMessage:'); - - await expect( - controller.signTypedMessage( - { data: msgParams, from: account }, - SignTypedDataVersion.V3, - ), - ).rejects.toThrow('Keyring Controller signTypedMessage:'); - }); - }); + origin: 'https://metamask.github.io', + }; - it('should fail in signing message when from address is not provided', async () => { - await withController(async ({ controller }) => { - const typedMsgParams = [ - { - name: 'Message', - type: 'string', - value: 'Hi, Alice!', - }, - { - name: 'A number', - type: 'uint32', - value: '1337', + await expect( + controller.signTypedMessage(inputParams, SignTypedDataVersion.V1), + ).rejects.toThrow( + KeyringControllerError.UnsupportedSignTypedMessage, + ); }, - ]; - await expect( - controller.signTypedMessage( - { data: typedMsgParams, from: '' }, - SignTypedDataVersion.V1, - ), - ).rejects.toThrow(/^Keyring Controller signTypedMessage:/u); + ); }); }); }); describe('signTransaction', () => { - it('should sign transaction', async () => { - await withController(async ({ controller, initialState }) => { - const account = initialState.keyrings[0].accounts[0]; - const txParams = { - chainId: 5, - data: '0x1', - from: account, - gasLimit: '0x5108', - gasPrice: '0x5108', - to: '0x51253087e6f8358b5f10c0a94315d69db3357859', - value: '0x5208', - }; - const unsignedEthTx = TransactionFactory.fromTxData(txParams, { - common: new Common(commonConfig), - freeze: false, - }); - expect(unsignedEthTx.v).toBeUndefined(); - const signedTx = await controller.signTransaction( - unsignedEthTx, - account, - ); - expect(signedTx.v).toBeDefined(); - expect(signedTx).not.toBe(''); - }); - }); - - it('should not sign transaction if from account is not provided', async () => { - await withController(async ({ controller, initialState }) => { - await expect(async () => { + describe('when the keyring for the given address supports signTransaction', () => { + it('should sign transaction', async () => { + await withController(async ({ controller, initialState }) => { const account = initialState.keyrings[0].accounts[0]; const txParams = { chainId: 5, @@ -1350,24 +1507,71 @@ describe('KeyringController', () => { freeze: false, }); expect(unsignedEthTx.v).toBeUndefined(); - await controller.signTransaction(unsignedEthTx, ''); - }).rejects.toThrow( - 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', - ); + const signedTx = await controller.signTransaction( + unsignedEthTx, + account, + ); + expect(signedTx.v).toBeDefined(); + expect(signedTx).not.toBe(''); + }); + }); + + it('should not sign transaction if from account is not provided', async () => { + await withController(async ({ controller, initialState }) => { + await expect(async () => { + const account = initialState.keyrings[0].accounts[0]; + const txParams = { + chainId: 5, + data: '0x1', + from: account, + gasLimit: '0x5108', + gasPrice: '0x5108', + to: '0x51253087e6f8358b5f10c0a94315d69db3357859', + value: '0x5208', + }; + const unsignedEthTx = TransactionFactory.fromTxData(txParams, { + common: new Common(commonConfig), + freeze: false, + }); + expect(unsignedEthTx.v).toBeUndefined(); + await controller.signTransaction(unsignedEthTx, ''); + }).rejects.toThrow( + 'KeyringController - No keyring found. Error info: The address passed in is invalid/empty', + ); + }); + }); + + /** + * Task added to improve check for valid transaction in signTransaction method + * https://github.com/MetaMask/core/issues/800 + */ + it('should not sign transaction if transaction is not valid', async () => { + 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'); + }); }); }); - /** - * Task added to improve check for valid transaction in signTransaction method - * https://github.com/MetaMask/core/issues/800 - */ - it('should not sign transaction if transaction is not valid', async () => { - 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'); + describe('when the keyring for the given address does not support signTransaction', () => { + it('should throw if the keyring for the given address does not support signTransaction', async () => { + const address = '0x5aC6d462f054690A373Fabf8cc28E161003aEB19'; + stubKeyringClassWithAccount(MockKeyring, address); + await withController( + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + + await expect( + controller.signTransaction(buildMockTransaction(), address), + ).rejects.toThrow( + KeyringControllerError.UnsupportedSignTransaction, + ); + }, + ); }); }); }); @@ -2564,6 +2768,25 @@ type WithControllerArgs = | [WithControllerCallback] | [WithControllerOptions, WithControllerCallback]; +/** + * Stub the `getAccounts` and `addAccounts` methods of the given keyring class to return the given + * account. + * + * @param keyringClass - The keyring class to stub. + * @param account - The account to return. + */ +function stubKeyringClassWithAccount( + keyringClass: KeyringClass, + account: string, +) { + jest + .spyOn(keyringClass.prototype, 'getAccounts') + .mockResolvedValue([account]); + jest + .spyOn(keyringClass.prototype, 'addAccounts') + .mockResolvedValue([account]); +} + /** * Build a controller messenger that includes all events used by the keyring * controller. diff --git a/packages/keyring-controller/tests/mocks/mockKeyring.ts b/packages/keyring-controller/tests/mocks/mockKeyring.ts new file mode 100644 index 0000000000..9ce4b50caf --- /dev/null +++ b/packages/keyring-controller/tests/mocks/mockKeyring.ts @@ -0,0 +1,35 @@ +import type { EthKeyring } from '@metamask/keyring-api'; +import type { Json, Hex } from '@metamask/utils'; + +export class MockKeyring implements EthKeyring { + static type = 'Mock Keyring'; + + public type = 'Mock Keyring'; + + #accounts: Hex[] = []; + + constructor(options: Record | undefined = {}) { + // eslint-disable-next-line @typescript-eslint/no-floating-promises, @typescript-eslint/promise-function-async + this.deserialize(options); + } + + async init() { + return Promise.resolve(); + } + + async addAccounts(_: number): Promise { + return Promise.resolve(this.#accounts); + } + + async getAccounts() { + return Promise.resolve(this.#accounts); + } + + async serialize() { + return Promise.resolve({}); + } + + async deserialize(_: unknown) { + return Promise.resolve(); + } +} diff --git a/packages/keyring-controller/tests/mocks/mockTransaction.ts b/packages/keyring-controller/tests/mocks/mockTransaction.ts new file mode 100644 index 0000000000..5548fa0a00 --- /dev/null +++ b/packages/keyring-controller/tests/mocks/mockTransaction.ts @@ -0,0 +1,18 @@ +import { TransactionFactory, type TxData } from '@ethereumjs/tx'; + +/** + * Build a mock transaction, optionally overriding + * any of the default values. + * + * @param options - The transaction options to override. + * @returns The mock transaction. + */ +export const buildMockTransaction = (options: TxData = {}) => + TransactionFactory.fromTxData({ + to: '0xB1A13aBECeB71b2E758c7e0Da404DF0C72Ca3a12', + value: '0x0', + data: '0x', + gasPrice: '0x0', + nonce: '0x0', + ...options, + });