Set password sooner to avoid redundant persistance #154
Conversation
| @@ -752,7 +743,7 @@ class KeyringController extends EventEmitter { | |||
| forgetKeyring(keyring) { | |||
| if (keyring.forgetDevice) { | |||
| keyring.forgetDevice(); | |||
| this.persistAllKeyrings.bind(this)(); | |||
There was a problem hiding this comment.
This .bind was redundant, as we're calling persistAllKeyrings on this anyway. So it's a no-op.
| await this.persistAllKeyrings(password); | ||
| this.password = password; | ||
|
|
||
| await this.createFirstKeyTree(); |
There was a problem hiding this comment.
createFirstKeyTree only modifies this.keyrings via addNewKeyring, which calls persistAllKeyrings internally after adding the keyring. That's why the persistAllKeyrings call was eliminated here.
|
|
||
| await this.persistAllKeyrings(password); |
There was a problem hiding this comment.
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.
| @@ -130,8 +131,6 @@ class KeyringController extends EventEmitter { | |||
| if (!firstAccount) { | |||
| throw new Error('KeyringController - First Account not found.'); | |||
| } | |||
|
|
|||
| await this.persistAllKeyrings(password); | |||
There was a problem hiding this comment.
Removed because the last time this.keyrings is modified is within addNewKeyring, which calls persistAllKeyrings internally.
The vault password is now always set in the method responsible for creating the vault. Previously it was set when the vault was persisted, or when the first keyring was created. This led to redundant calls to persist the keyrings, just to achieve the side-effect of setting the password. This change eliminates those redundant calls, and a few more that had no obvious purpose. Note that the password is also set on unlock, and that has not changed here.
Gudahtt
force-pushed
the
set-password-when-creating-vault
branch
from
5905de9
to
eb3b986
Compare
The vault password is now always set in the method responsible for creating the vault. Previously it was set when the vault was persisted, or when the first keyring was created. This led to redundant calls to persist the keyrings, just to achieve the side-effect of setting the password.
This change eliminates those redundant calls, and a few more that had no obvious purpose.
Note that the password is also set on unlock, and that has not changed here.