Skip vault creation if one already exists

[mikesposito]

Description

This PR changes the KeyringController.createNewVaultAndRestore method to not create a new vault if there is already an existing one. In this case, it will simply return the existing vault.

Changes

• CHANGED: createNewVaultAndRestore from KeyringController, to not overwrite existing vault.

References

• Fixes KeyringController: createNewVaultAndKeychain shouldn't create a vault if one already exists #1250

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
• I've highlighted breaking changes using the "BREAKING" category above as appropriate

[Gudahtt]

LGTM!

[mcmire]

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.

[Gudahtt]

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.

[Gudahtt]

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.

[mcmire]

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?)

[Gudahtt]

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?

[mcmire]

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.

[Gudahtt]

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).