fix(account-tree-controller): prevent :account{Added,Removed} if init has not been called yet

[ccharly]

Explanation

I initially thought calling init implicitly would solve some issues, but it actually adds one when it comes to automatically migrating accounts from v1 state.

In the clients we usually use the tree like so:

this.accountsController.updateAccounts();
this.accountTreeController.init();

The updateAccounts part re-creates the list of internal accounts and potentially migrate them if needed (in the case of BIP-44, this is needed to inject the options.entropy for those accounts).

If a KeyingController:stateChange happens before updateAccounts, this might trigger some :accountAdded event, but some accounts might not have been automatically migrated yet (since updateAccounts has not been called at this point). Thus, this can lead the AccountTreeController to misuse those accounts and put them in wrong wallet (like older HD accounts had not options.entropy objects, thus they are not considered BIP-44 accounts yet).

To avoid such issues (which seems to mainly happen for E2E), we wait for init to have been called explicitly.

In anyway, calling updateAccounts right before init would make sure that the list of accounts is up-to-date AND migrated.

References

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
• I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

[ccharly]

@metamaskbot publish-preview

[ccharly]

@metamaskbot publish-preview

[mathieuartu]

I'm writing this comment last, and writing the other comments before made me realize that this might be a breaking change?

[ccharly]

This could be seen as a breaking change in terms of behavior, but I actually think the current behavior is not valid either 😅

Also, "we" are the consumer the AccountsController and clients don't really have control over those :account{Added,Removed} event strictly speaking. I think it's implied that init has to be called before being able to use the tree anyway.

Lastly, making this a "true breaking change" would quickly bubble up to other controllers, while the public interface remains the same.

But I get your point and I'm on a fence here 😅 for simplicity, I'd would just treat it as a fix (given it does not require any change as of now from any consumers).

Would that be ok?

[mathieuartu]

I agree with all your points! Thanks for elaborating ❤️ Yep that'd be alright with me!