[keyring-controller] addAccountWithoutUpdate now updates the PreferencesController state

[Gudahtt]

The KeyringController method addAccountWithoutUpdate no longer works; it does add the account, but not without updating state to include thew new account.

This was broken in @metamask/[email protected]

[Gudahtt]

Maybe we can remove this method instead of fixing it. Will investigate if it's still needed.

It was added originally for this mobile feature: MetaMask/metamask-mobile#1879
But I'm not sure what negative impact temporarily adding the account has; maybe it's OK to add it then remove it during the import process.

Alternatively, we could update the keyring API to add a method that returns the next account address without adding it. That would solve the problem as well.

[mikesposito]

Alternatively, we could update the keyring API to add a method that returns the next account address without adding it. That would solve the problem as well.

@Gudahtt This could be similar to what also hardware keyrings currently do with getPreviousPage, getNextPage, getFirstPage: they provide a way to seek for accounts before adding them.

Perhaps we could think of a way to standardise that and add it to the general Keyring API? It might be a way to fix this issue while also making the Hardware keyrings easier to use, and a bit more aligned with how other keyrings work

[Gudahtt]

Good point, yes it's exactly like that

[desi]

@Gudahtt has offered to test on mobile to determine if the method is even needed.

[mikesposito]

@Gudahtt Regardless of the test I think we can close this and simply remove the method: addNewAccountWithoutUpdate never worked as intended since calling persistAllKeyrings also updates the controller state along with the vault

[Gudahtt]

This method was intended just to avoid updating PreferencesController state, not all state. The KeyringController state update was not intended to be prevented. I do think it had worked prior to v7 of the PreferencesController.

[mcmire]

@mikesposito @Gudahtt Just checking, is this ticket still relevant?

[desi]

We believe this isn't an issue but we should verify and close.