fix(accounts-controller): fire :accountAdded before :selectedAccountChange on KeyringController:stateChange
#6567
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hange on KeyringController:stateChange
ccharly
force-pushed
the
fix/accounts-controller-selected-account-change
branch
from
9b3135c to
1f2cfed
Compare
Contributor
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
ccharly
commented
Sep 11, 2025
Comment on lines
+885
to
+899
| // Will get executed after the update, but before re-selecting an account in case | ||
| // the current one is not valid anymore. | ||
| () => { | ||
| // Now publish events | ||
| for (const id of diff.removed) { | ||
| this.messagingSystem.publish('AccountsController:accountRemoved', id); | ||
| } | ||
|
|
||
| for (const account of diff.added) { | ||
| this.messagingSystem.publish('AccountsController:accountAdded', account); | ||
| } | ||
| for (const account of diff.added) { | ||
| this.messagingSystem.publish( | ||
| 'AccountsController:accountAdded', | ||
| account, | ||
| ); | ||
| } | ||
| }, |
Contributor
Author
There was a problem hiding this comment.
This new callback will get executed prior to the account selection. This solves the issue during onboarding when the keyring creates its initial account (which triggers a :stateChange).
With this change, we will be firing :accountAdded before :selectedAccount, thus allowing the account-tree-controller to first create its group before trying to mirror the selected account group.
ccharly
commented
Sep 11, 2025
|
|
||
| this.#update((state) => { | ||
| const { internalAccounts } = state; | ||
| this.#update( |
Contributor
Author
There was a problem hiding this comment.
Watchout, this call now takes 2 callbacks:
- The actual update to run
- The pre-step before selecting the new account that can result from the state update
mathieuartu
approved these changes
Sep 11, 2025
Contributor
mathieuartu
left a comment
mathieuartu
left a comment
There was a problem hiding this comment.
LGTM, have manually tested in extension and works as intended
hmalik88
approved these changes
Sep 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
We were firing
:selectedAccountChangebefore firing:accountAddedwhich does have undesired side-effect in theAccountTreeController.Mainly because the tree will try to get the associated selected account group, but this account group does not exist yet (since it's using
:accountAddedto create the account groups).This is more like a temporary solution, and
#updatecould be slightly refactored to a more robust solution later on.References
N/A
Checklist