From dc2cef41e93b725d447d0ec3c97078d291af6f9c Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 3 May 2023 13:47:06 +0200 Subject: [PATCH 1/2] feat: importAccountWithStrategy doesn't select new address --- .../src/KeyringController.test.ts | 224 +++++++++--------- .../src/KeyringController.ts | 1 - 2 files changed, 118 insertions(+), 107 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index cab6094f95..4813c42339 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -283,125 +283,137 @@ describe('KeyringController', () => { }); describe('importAccountWithStrategy', () => { - it('should import account with strategy privateKey', async () => { - await expect( - keyringController.importAccountWithStrategy( - AccountImportStrategy.privateKey, - [], - ), - ).rejects.toThrow('Cannot import an empty key.'); + describe('using strategy privateKey', () => { + describe('when correct key is provided', () => { + it('should import account', async () => { + const address = '0x51253087e6f8358b5f10c0a94315d69db3357859'; + const newKeyring = { accounts: [address], type: 'Simple Key Pair' }; + const { keyringState, importedAccountAddress } = + await keyringController.importAccountWithStrategy( + AccountImportStrategy.privateKey, + [privateKey], + ); + const modifiedState = { + ...initialState, + keyrings: [initialState.keyrings[0], newKeyring], + }; + expect(keyringState).toStrictEqual(modifiedState); + expect(importedAccountAddress).toBe(address); + }); - await expect( - keyringController.importAccountWithStrategy( - AccountImportStrategy.privateKey, - ['123'], - ), - ).rejects.toThrow( - 'Expected private key to be an Uint8Array with length 32', - ); + it('should not select imported account', async () => { + await keyringController.importAccountWithStrategy( + AccountImportStrategy.privateKey, + [privateKey], + ); + expect(preferences.setSelectedAddress.called).toBe(false); + }); + }); - await expect( - keyringController.importAccountWithStrategy( - AccountImportStrategy.privateKey, - ['0xblahblah'], - ), - ).rejects.toThrow('Cannot import invalid private key.'); - - const address = '0x51253087e6f8358b5f10c0a94315d69db3357859'; - const newKeyring = { accounts: [address], type: 'Simple Key Pair' }; - const { keyringState, importedAccountAddress } = - await keyringController.importAccountWithStrategy( - AccountImportStrategy.privateKey, - [privateKey], - ); - const modifiedState = { - ...initialState, - keyrings: [initialState.keyrings[0], newKeyring], - }; - expect(keyringState).toStrictEqual(modifiedState); - expect(importedAccountAddress).toBe(address); - }); + describe('when wrong key is provided', () => { + it('should not import account', async () => { + await expect( + keyringController.importAccountWithStrategy( + AccountImportStrategy.privateKey, + [], + ), + ).rejects.toThrow('Cannot import an empty key.'); - it('should not import account with strategy privateKey if wrong data is provided', async () => { - await expect( - keyringController.importAccountWithStrategy( - AccountImportStrategy.privateKey, - [], - ), - ).rejects.toThrow('Cannot import an empty key.'); + await expect( + keyringController.importAccountWithStrategy( + AccountImportStrategy.privateKey, + ['123'], + ), + ).rejects.toThrow( + 'Expected private key to be an Uint8Array with length 32', + ); - await expect( - keyringController.importAccountWithStrategy( - AccountImportStrategy.privateKey, - ['123'], - ), - ).rejects.toThrow( - 'Expected private key to be an Uint8Array with length 32', - ); + await expect( + keyringController.importAccountWithStrategy( + AccountImportStrategy.privateKey, + ['0xblahblah'], + ), + ).rejects.toThrow('Cannot import invalid private key.'); - await expect( - keyringController.importAccountWithStrategy( - AccountImportStrategy.privateKey, - [privateKey.slice(1)], - ), - ).rejects.toThrow('Cannot import invalid private key.'); + await expect( + keyringController.importAccountWithStrategy( + AccountImportStrategy.privateKey, + [privateKey.slice(1)], + ), + ).rejects.toThrow('Cannot import invalid private key.'); + }); + }); }); - it('should import account with strategy json', async () => { - const somePassword = 'holachao123'; - - await expect( - keyringController.importAccountWithStrategy( - AccountImportStrategy.json, - ['', somePassword], - ), - ).rejects.toThrow('Unexpected end of JSON input'); - - const address = '0xb97c80fab7a3793bbe746864db80d236f1345ea7'; - - const { keyringState, importedAccountAddress } = - await keyringController.importAccountWithStrategy( - AccountImportStrategy.json, - [input, somePassword], - ); + describe('using strategy json', () => { + describe('when correct data is provided', () => { + it('should import account', async () => { + const somePassword = 'holachao123'; + const address = '0xb97c80fab7a3793bbe746864db80d236f1345ea7'; + + const { keyringState, importedAccountAddress } = + await keyringController.importAccountWithStrategy( + AccountImportStrategy.json, + [input, somePassword], + ); + + const newKeyring = { accounts: [address], type: 'Simple Key Pair' }; + const modifiedState = { + ...initialState, + keyrings: [initialState.keyrings[0], newKeyring], + }; + expect(keyringState).toStrictEqual(modifiedState); + expect(importedAccountAddress).toBe(address); + }); + }); - const newKeyring = { accounts: [address], type: 'Simple Key Pair' }; - const modifiedState = { - ...initialState, - keyrings: [initialState.keyrings[0], newKeyring], - }; - expect(keyringState).toStrictEqual(modifiedState); - expect(importedAccountAddress).toBe(address); - }); + describe('when wrong data is provided', () => { + it('should not import account with empty json', async () => { + const somePassword = 'holachao123'; + await expect( + keyringController.importAccountWithStrategy( + AccountImportStrategy.json, + ['', somePassword], + ), + ).rejects.toThrow('Unexpected end of JSON input'); + }); - it('should throw when passed an unrecognized strategy', async () => { - const somePassword = 'holachao123'; - await expect( - keyringController.importAccountWithStrategy( - 'junk' as AccountImportStrategy, - [input, somePassword], - ), - ).rejects.toThrow("Unexpected import strategy: 'junk'"); - }); + it('should not import account with wrong password', async () => { + const somePassword = 'holachao12'; - it('should import account with strategy json wrong password', async () => { - const somePassword = 'holachao12'; + await expect( + keyringController.importAccountWithStrategy( + AccountImportStrategy.json, + [input, somePassword], + ), + ).rejects.toThrow( + 'Key derivation failed - possibly wrong passphrase', + ); + }); - await expect( - keyringController.importAccountWithStrategy( - AccountImportStrategy.json, - [input, somePassword], - ), - ).rejects.toThrow('Key derivation failed - possibly wrong passphrase'); + it('should not import account with empty password', async () => { + await expect( + keyringController.importAccountWithStrategy( + AccountImportStrategy.json, + [input, ''], + ), + ).rejects.toThrow( + 'Key derivation failed - possibly wrong passphrase', + ); + }); + }); }); - it('should import account with strategy json empty password', async () => { - await expect( - keyringController.importAccountWithStrategy( - AccountImportStrategy.json, - [input, ''], - ), - ).rejects.toThrow('Key derivation failed - possibly wrong passphrase'); + describe('using unrecognized strategy', () => { + it('should throw an unexpected import strategy error', async () => { + const somePassword = 'holachao123'; + await expect( + keyringController.importAccountWithStrategy( + 'junk' as AccountImportStrategy, + [input, somePassword], + ), + ).rejects.toThrow("Unexpected import strategy: 'junk'"); + }); }); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 70b3b290f9..db64868587 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -431,7 +431,6 @@ export class KeyringController extends BaseController< const accounts = await newKeyring.getAccounts(); const allAccounts = await this.#keyring.getAccounts(); this.updateIdentities(allAccounts); - this.setSelectedAddress(accounts[0]); return { keyringState: await this.fullUpdate(), importedAccountAddress: accounts[0], From edd04601739107117e251bbee339d20c6cb4b507 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 4 May 2023 09:19:30 +0200 Subject: [PATCH 2/2] test: add test case for json import --- .../keyring-controller/src/KeyringController.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 4813c42339..2f05c1ee4e 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -365,6 +365,15 @@ describe('KeyringController', () => { expect(keyringState).toStrictEqual(modifiedState); expect(importedAccountAddress).toBe(address); }); + + it('should not select imported account', async () => { + const somePassword = 'holachao123'; + await keyringController.importAccountWithStrategy( + AccountImportStrategy.json, + [input, somePassword], + ); + expect(preferences.setSelectedAddress.called).toBe(false); + }); }); describe('when wrong data is provided', () => {