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

Jump to bottom

importAccountWithStrategy shouldn't select address #1309

Merged
merged 4 commits into from
May 5, 2023
Merged

importAccountWithStrategy shouldn't select address #1309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dc2cef4
feat: importAccountWithStrategy doesn't select new address
mikesposito May 3, 2023
edd0460
test: add test case for json import
mikesposito May 4, 2023
4616c98
Merge branch 'main' into feat/1249-importaccountwithstrategy
mikesposito May 4, 2023
e8aacec
Merge branch 'main' into feat/1249-importaccountwithstrategy
mikesposito May 5, 2023
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
231 changes: 126 additions & 105 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 @@ -283,125 +283,146 @@ 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.');
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
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 () => {
Copy link
Member

@Gudahtt Gudahtt May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: Should we repeat this test for the JSON input as well? Or maybe bring it to the top-level of the importAccountWithStrategy block to show that it's not expected to differ between strategies?

Copy link
Member Author

@mikesposito mikesposito May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Uhm, good point - I think it would make more sense to add it for the JSON input, rather than moving it to the top-level group because otherwise it would go along with:

  • using strategy privateKey
  • using strategy json
  • using unrecognized strategy

EDIT: Also, the actual test would be different, as we would use a different strategy - so I think it makes even more sense to repeat it for JSON

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';
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 { keyringState, importedAccountAddress } =
await keyringController.importAccountWithStrategy(
AccountImportStrategy.json,
[input, somePassword],
);
it('should not select imported account', async () => {
const somePassword = 'holachao123';
await keyringController.importAccountWithStrategy(
AccountImportStrategy.json,
[input, somePassword],
);
expect(preferences.setSelectedAddress.called).toBe(false);
});
});

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'");
});
});
});

Expand Down
1 change: 0 additions & 1 deletion packages/keyring-controller/src/KeyringController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down