Going through this flow again, I think we can/should clean up in a slightly different way perhaps.

So as you've put it here we call clearKeyrings first, then we call persistKeyrings and all that it does is set this.password to the user input password, it does not persist anything to the vault field in state because - having just cleared the keyrings array - there are no keyrings in the keyrings array to enter the map loop on line 528.

Then we enter createFirstKeyTree which also calls clearKeyrings before doing anything else (repeating the first step).

I'm not seeing why we shouldn't just skip straight to the createFirstKeyTree call as the first method call in this chain:

 createNewVaultAndKeychain (password) {
     return this.createFirstKeyTree()
      .then(this.persistAllKeyrings.bind(this, password))
      .then(this.setUnlocked.bind(this))
      .then(this.fullUpdate.bind(this))
  }

@jpuri @Gudahtt @danjm am I misreading this?

@adonesky1 That does seem better to me, except that setting a password is required before calling createFirstKeyTree. persistAllKeyrings needs to have a password set, and that's called within createFirstKeyTree.

Perhaps the password could be passed into createFirstKeyTree, and from there to persistAllKeyrings. That should work, and it would prevent the duplicate call to clearKeyrings.

@adonesky1 , @Gudahtt : I spent more time looking at the code, change suggested above will require changing method addNewKeyring at https://github.com/MetaMask/KeyringController/blob/main/index.js#L198, we need to pass password to the method which will be in-turn passed in call to persistAllKeyrings.

This method is called from other places also, thus I would avoid making this change, please suggest.

@jpuri could we just pass the password to createFirstKeyTree and then set it first thing:

createFirstKeyTree(password) {
    this.password = password;
    this.clearKeyrings();
    return this.addNewKeyring('HD Key Tree', { numberOfAccounts: 1 })
      .then((keyring) => {
        return keyring.getAccounts();
      })
      .then(([firstAccount]) => {
        if (!firstAccount) {
          throw new Error('KeyringController - No account found on keychain.');
        }
        const hexAccount = normalizeAddress(firstAccount);
        this.emit('newVault', hexAccount);
        return null;
      });
  }

then we don't need to change the function signature of addNewKeyring?

So createNewVaultAndKeychain would be:

createNewVaultAndKeychain(password) {
    return this.createFirstKeyTree(password)
      .then(this.persistAllKeyrings.bind(this, password))
      .then(this.setUnlocked.bind(this))
      .then(this.fullUpdate.bind(this));
  }

I believe this resolves the flow/duplication issues without needing to drastic a change?

cc @Gudahtt @danjm

@adonesky1 : I just updated the PR please check.

@jpuri looks like there's a lint issue.

@Gudahtt can you provide feedback on the approach I've suggested here?