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

Skip vault creation if one already exists #1324

Merged
merged 6 commits into from
May 5, 2023
Merged

Skip vault creation if one already exists #1324

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
30c9a60
feat: don't create new vault if one already exists
mikesposito May 4, 2023
7398719
Merge branch 'main' into feat/1250-skip-create-vault-if-exists
mikesposito May 4, 2023
bc9ea62
fix: use internal fullUpdate
mikesposito May 4, 2023
7deb9e2
Merge branch 'main' into feat/1250-skip-create-vault-if-exists
mikesposito May 4, 2023
401452a
Merge branch 'main' into feat/1250-skip-create-vault-if-exists
mikesposito May 5, 2023
58cf5cf
Merge branch 'main' into feat/1250-skip-create-vault-if-exists
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
56 changes: 41 additions & 15 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 @@ -10,7 +10,7 @@ import { TransactionFactory } from '@ethereumjs/tx';
import { MetaMaskKeyring as QRKeyring } from '@keystonehq/metamask-airgapped-keyring';
import { CryptoHDKey, ETHSignature } from '@keystonehq/bc-ur-registry-eth';
import * as uuid from 'uuid';
import { NetworkType } from '@metamask/controller-utils';
import { isValidHexAddress, NetworkType } from '@metamask/controller-utils';
import { keyringBuilderFactory } from '@metamask/eth-keyring-controller';
import MockEncryptor from '../tests/mocks/mockEncryptor';
import {
Expand Down Expand Up @@ -217,20 +217,46 @@ describe('KeyringController', () => {
});

describe('createNewVaultAndKeychain', () => {
it('should create new vault, mnemonic and keychain', async () => {
const initialSeedWord = await keyringController.exportSeedPhrase(
password,
);
expect(initialSeedWord).toBeDefined();
const currentState = await keyringController.createNewVaultAndKeychain(
password,
);
expect(initialState).not.toBe(currentState);
const currentSeedWord = await keyringController.exportSeedPhrase(
password,
);
expect(currentSeedWord).toBeDefined();
expect(initialSeedWord).not.toBe(currentSeedWord);
describe('when there is no existing vault', () => {
it('should create new vault, mnemonic and keychain', async () => {
const cleanKeyringController = new KeyringController(
preferences,
baseConfig,
);
const initialSeedWord = await keyringController.exportSeedPhrase(
password,
);
const currentState =
Copy link
Contributor

Choose a reason for hiding this comment

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

We are testing the return value of the method, which is good, but should we also test the state of the controller directly as well? Same goes for the next test. (Unless exportSeedPhrase returns a part of the state already?)

Copy link
Member

Choose a reason for hiding this comment

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

The return value is the controller state. At least it's the in-memory state, which is the more informative piece (the persisted state is just the encrypted vault).

Is there something else in particular you had in mind that we should be testing?

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the case then I suppose it's not required. My thought was that if we ask the controller directly it makes this test more future-proof. But I suppose this would do.

Copy link
Member

Choose a reason for hiding this comment

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

Making this access the state in a more conventional way sounds like a good idea to me too. But this is a bit of a weird case. The "real" controller state right now is just the persisted state; the in-memory state would need to be accessed via a custom property (not exactly future proof).

await cleanKeyringController.createNewVaultAndKeychain(password);
const currentSeedWord = await cleanKeyringController.exportSeedPhrase(
password,
);
expect(initialSeedWord).toBeDefined();
expect(initialState).not.toBe(currentState);
expect(currentSeedWord).toBeDefined();
expect(initialSeedWord).not.toBe(currentSeedWord);
expect(isValidHexAddress(currentState.keyrings[0].accounts[0])).toBe(
true,
);
});
});

describe('when there is an existing vault', () => {
it('should return existing vault', async () => {
const initialSeedWord = await keyringController.exportSeedPhrase(
password,
);
const currentState = await keyringController.createNewVaultAndKeychain(
password,
);
const currentSeedWord = await keyringController.exportSeedPhrase(
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we are testing the results of exportSeedPhrase in these tests, but the describe block has to do with createNewVaultAndKeychain. I'm wondering if there is another way to test the effects of createNewVaultAndKeychain without depending on other methods. Or, perhaps we should test the results of exportSeedPhrase in the tests for that method? Out of scope for this PR, but something to think about.

Copy link
Member

Choose a reason for hiding this comment

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

The effect of creating a new vault would be along the lines of:

  • Generate a new SRP / HD keyring
  • Encrypt the keyring with the given password
  • Add the resulting encrypted string to the persisted store

Calling exportSeedPhrase does seem like a reasonably direct way to test the first two parts. It might be worth verifying that the vault was updated in the persisted store as well though.

Copy link
Member

Choose a reason for hiding this comment

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

We could consider manually "decrypting" the vault here, as an alternative to using exportSeedPhrase. That would also provide the SRP, and would verify the password; all three effects verified in one assertion.

That seems reasonable 🤔 It would require a bit more more setup though; we'd want to pass in a mock encryption module, so that the test isn't coupled with browser-passworder internal implementation details. That would let us use a "fake" encryption/decryption as well, for a faster test with easier-to-read data.

password,
);
expect(initialSeedWord).toBeDefined();
expect(initialState).toBe(currentState);
expect(currentSeedWord).toBeDefined();
expect(initialSeedWord).toBe(currentSeedWord);
});
});
});

Expand Down
13 changes: 10 additions & 3 deletions packages/keyring-controller/src/KeyringController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,16 @@ export class KeyringController extends BaseController<
async createNewVaultAndKeychain(password: string) {
const releaseLock = await this.mutex.acquire();
try {
const vault = await this.#keyring.createNewVaultAndKeychain(password);
this.updateIdentities(await this.#keyring.getAccounts());
this.fullUpdate();
let vault;
const accounts = await this.getAccounts();
if (accounts.length > 0) {
vault = await this.fullUpdate();
} else {
vault = await this.#keyring.createNewVaultAndKeychain(password);
this.updateIdentities(await this.getAccounts());
this.fullUpdate();
}

return vault;
} finally {
releaseLock();
Expand Down