Skip to content
This repository was archived by the owner on Oct 7, 2024. It is now read-only.

Set password sooner to avoid redundant persistance #154

Merged
Gudahtt merged 1 commit into from
Nov 7, 2022
Merged

Set password sooner to avoid redundant persistance #154

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 8 additions & 17 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,9 @@ class KeyringController extends EventEmitter {
* @returns {Promise<Object>} A Promise that resolves to the state.
*/
async createNewVaultAndKeychain(password) {
await this.createFirstKeyTree(password);
await this.persistAllKeyrings(password);
this.password = password;

await this.createFirstKeyTree();
Copy link
Member Author

@Gudahtt Gudahtt Nov 7, 2022

Choose a reason for hiding this comment

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

createFirstKeyTree only modifies this.keyrings via addNewKeyring, which calls persistAllKeyrings internally after adding the keyring. That's why the persistAllKeyrings call was eliminated here.

this.setUnlocked();
this.fullUpdate();
}
Expand Down Expand Up @@ -116,9 +117,9 @@ class KeyringController extends EventEmitter {
throw new Error('Seed phrase is invalid.');
}

this.clearKeyrings();
this.password = password;

await this.persistAllKeyrings(password);
Copy link
Member Author

@Gudahtt Gudahtt Nov 7, 2022

Choose a reason for hiding this comment

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

I'm guessing this call was primarily meant to set the password, so it has been removed.

It did have another functional impact, in that it ensured the clearKeyrings operation was reflected in the vault, in case the attempt to create a new vault fails. But... that situation is not anticipated. At this point we have validated that the SRP is valid, and the password is text, so there aren't any more additional failure conditions.

Moreover; leaving an existing vault in state isn't an awful end result to encountering an unexpected error. If anything it might aid with recovery, if we do somehow end up accidentally bricking the wallet with an update.

this.clearKeyrings();
const firstKeyring = await this.addNewKeyring(
KEYRINGS_TYPE_MAP.HD_KEYRING,
{
Expand All @@ -130,8 +131,6 @@ class KeyringController extends EventEmitter {
if (!firstAccount) {
throw new Error('KeyringController - First Account not found.');
}

await this.persistAllKeyrings(password);
Copy link
Member Author

@Gudahtt Gudahtt Nov 7, 2022

Choose a reason for hiding this comment

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

Removed because the last time this.keyrings is modified is within addNewKeyring, which calls persistAllKeyrings internally.

this.setUnlocked();
return this.fullUpdate();
}
Expand Down Expand Up @@ -487,11 +486,9 @@ class KeyringController extends EventEmitter {
* - Faucets that account on testnet
* - Puts the current seed words into the state tree
*
* @param {string} password - The keyring controller password.
* @returns {Promise<void>} - A promise that resolves if the operation was successful.
*/
async createFirstKeyTree(password) {
this.password = password;
async createFirstKeyTree() {
this.clearKeyrings();

const keyring = await this.addNewKeyring(KEYRINGS_TYPE_MAP.HD_KEYRING);
Expand All @@ -513,15 +510,9 @@ class KeyringController extends EventEmitter {
* encrypts that array with the provided `password`,
* and persists that encrypted string to storage.
*
* @param {string} password - The keyring controller password.
* @returns {Promise<boolean>} Resolves to true once keyrings are persisted.
*/
async persistAllKeyrings(password = this.password) {
if (typeof password !== 'string') {
throw new Error('KeyringController - password is not a string');
}

this.password = password;
async persistAllKeyrings() {
const serializedKeyrings = await Promise.all(
this.keyrings.map(async (keyring) => {
const [type, data] = await Promise.all([
Expand Down Expand Up @@ -752,7 +743,7 @@ class KeyringController extends EventEmitter {
forgetKeyring(keyring) {
if (keyring.forgetDevice) {
keyring.forgetDevice();
this.persistAllKeyrings.bind(this)();
Copy link
Member Author

@Gudahtt Gudahtt Nov 7, 2022

Choose a reason for hiding this comment

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

This .bind was redundant, as we're calling persistAllKeyrings on this anyway. So it's a no-op.

this.persistAllKeyrings();
this._updateMemStoreKeyrings.bind(this)();
} else {
throw new Error(
Expand Down