Skip to content

fix(accounts-controller): do not fire events during update blocks#5555

Merged
montelaidev merged 13 commits into
mainfrom
fix/accounts-controller-events-during-updates
Apr 1, 2025
Merged

fix(accounts-controller): do not fire events during update blocks#5555
montelaidev merged 13 commits into
mainfrom
fix/accounts-controller-events-during-updates

Conversation

@ccharly

@ccharly ccharly commented Mar 26, 2025

Copy link
Copy Markdown
Contributor

Explanation

Events should be fired after making a state update. Otherwise we could end up with race conditions like some other component react to AccountsController:accountAdded and try to call getAccount(account.id), it might not the same value compared to the account being passed during the event (since the state update has not been persisted yet).

This PR now moves all events firing after the state update.

It also includes a small refactor of the KeyringController:stateChange handler (which made the new events firing logic, a bit easier).

Test E2E PR:

References

N/A

Changelog

N/A

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 highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@ccharly ccharly force-pushed the fix/accounts-controller-events-during-updates branch from 326cb1c to d8c068a Compare March 28, 2025 11:45
@ccharly ccharly self-assigned this Mar 28, 2025
@ccharly ccharly marked this pull request as ready for review March 28, 2025 11:45
@ccharly ccharly requested a review from a team as a code owner March 28, 2025 11:45
@ccharly

ccharly commented Mar 28, 2025

Copy link
Copy Markdown
Contributor Author

@metamaskbot publish-preview

@ccharly ccharly force-pushed the fix/accounts-controller-events-during-updates branch from d8c068a to aa7690c Compare March 28, 2025 11:51
@ccharly

ccharly commented Mar 28, 2025

Copy link
Copy Markdown
Contributor Author

@metamaskbot publish-preview

@github-actions

Copy link
Copy Markdown
Contributor

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask-previews/accounts-controller": "27.0.0-preview-aa7690c1",
  "@metamask-previews/address-book-controller": "6.0.3-preview-aa7690c1",
  "@metamask-previews/announcement-controller": "7.0.3-preview-aa7690c1",
  "@metamask-previews/approval-controller": "7.1.3-preview-aa7690c1",
  "@metamask-previews/assets-controllers": "55.0.1-preview-aa7690c1",
  "@metamask-previews/base-controller": "8.0.0-preview-aa7690c1",
  "@metamask-previews/bridge-controller": "11.0.0-preview-aa7690c1",
  "@metamask-previews/bridge-status-controller": "10.0.0-preview-aa7690c1",
  "@metamask-previews/build-utils": "3.0.3-preview-aa7690c1",
  "@metamask-previews/chain-agnostic-permission": "0.3.0-preview-aa7690c1",
  "@metamask-previews/composable-controller": "11.0.0-preview-aa7690c1",
  "@metamask-previews/controller-utils": "11.6.0-preview-aa7690c1",
  "@metamask-previews/earn-controller": "0.10.0-preview-aa7690c1",
  "@metamask-previews/eip1193-permission-middleware": "0.1.0-preview-aa7690c1",
  "@metamask-previews/ens-controller": "16.0.0-preview-aa7690c1",
  "@metamask-previews/eth-json-rpc-provider": "4.1.8-preview-aa7690c1",
  "@metamask-previews/gas-fee-controller": "23.0.0-preview-aa7690c1",
  "@metamask-previews/json-rpc-engine": "10.0.3-preview-aa7690c1",
  "@metamask-previews/json-rpc-middleware-stream": "8.0.7-preview-aa7690c1",
  "@metamask-previews/keyring-controller": "21.0.1-preview-aa7690c1",
  "@metamask-previews/logging-controller": "6.0.4-preview-aa7690c1",
  "@metamask-previews/message-manager": "12.0.1-preview-aa7690c1",
  "@metamask-previews/multichain": "4.0.0-preview-aa7690c1",
  "@metamask-previews/multichain-api-middleware": "0.1.1-preview-aa7690c1",
  "@metamask-previews/multichain-network-controller": "0.3.0-preview-aa7690c1",
  "@metamask-previews/multichain-transactions-controller": "0.8.0-preview-aa7690c1",
  "@metamask-previews/name-controller": "8.0.3-preview-aa7690c1",
  "@metamask-previews/network-controller": "23.1.0-preview-aa7690c1",
  "@metamask-previews/notification-services-controller": "5.0.1-preview-aa7690c1",
  "@metamask-previews/permission-controller": "11.0.6-preview-aa7690c1",
  "@metamask-previews/permission-log-controller": "3.0.3-preview-aa7690c1",
  "@metamask-previews/phishing-controller": "12.4.1-preview-aa7690c1",
  "@metamask-previews/polling-controller": "13.0.0-preview-aa7690c1",
  "@metamask-previews/preferences-controller": "17.0.0-preview-aa7690c1",
  "@metamask-previews/profile-sync-controller": "11.0.0-preview-aa7690c1",
  "@metamask-previews/queued-request-controller": "10.0.0-preview-aa7690c1",
  "@metamask-previews/rate-limit-controller": "6.0.3-preview-aa7690c1",
  "@metamask-previews/remote-feature-flag-controller": "1.6.0-preview-aa7690c1",
  "@metamask-previews/sample-controllers": "0.0.0-preview-aa7690c1",
  "@metamask-previews/selected-network-controller": "22.0.0-preview-aa7690c1",
  "@metamask-previews/signature-controller": "27.0.0-preview-aa7690c1",
  "@metamask-previews/token-search-discovery-controller": "2.1.0-preview-aa7690c1",
  "@metamask-previews/transaction-controller": "52.2.0-preview-aa7690c1",
  "@metamask-previews/user-operation-controller": "31.0.0-preview-aa7690c1"
}

Comment thread packages/accounts-controller/src/AccountsController.ts
Comment on lines +756 to +776
for (const keyring of keyrings) {
const patch = patchOf(keyring.type);

// ^NOTE: This will be removed when normal accounts also implement internal accounts
// finding all the normal accounts that were added
for (const account of updatedNormalKeyringAddresses) {
if (
!this.state.internalAccounts.accounts[
getUUIDFromAddressOfNormalAccount(account.address)
]
) {
addedAccounts.push(account);
}
}
for (const accountAddress of keyring.accounts) {
// Lower-case address to use it in the `previous` map.
const address = accountAddress.toLowerCase();
const account = patch.previous[address];

// finding all the snap accounts that were added
for (const account of updatedSnapKeyringAddresses) {
if (
!previousSnapInternalAccounts.find(
(internalAccount: InternalAccount) =>
internalAccount.address.toLowerCase() ===
account.address.toLowerCase(),
)
) {
addedAccounts.push(account);
if (account) {
// If the account exists before, this might be an update.
patch.updated.push(account);
} else {
// Otherwise, that's a new account.
patch.added.push({
address,
type: keyring.type,
});
}

// Keep track of those address to check for removed accounts later.
addresses.add(address);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just add it directly into the set. The normal account shouldn't change, so we don't need to add it to the updated key of patch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note:
This logic will be deprecated when the KeyringController emits an account added event

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, as discussed together, we can keep this logic as-is for now, but indeed, most of this logic won't apply anymore once we track "account updates" on the KeyringController side (which probably depends on using KeyringAccount for every keyrings first)

Comment thread packages/accounts-controller/src/AccountsController.ts Outdated
Co-authored-by: Monte Lai <monte.lai@consensys.net>
@montelaidev montelaidev enabled auto-merge (squash) April 1, 2025 09:13
@montelaidev montelaidev merged commit 87ebc4b into main Apr 1, 2025
@montelaidev montelaidev deleted the fix/accounts-controller-events-during-updates branch April 1, 2025 09:37
github-merge-queue Bot pushed a commit to MetaMask/metamask-extension that referenced this pull request May 15, 2025
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

Updating `@metamask/keyring-controller` to `^22.0.0`:

```markdown
## [22.0.0]

### Changed

- **BREAKING** `keyringsMetadata` has been removed from the controller state ([#5725](MetaMask/core#5725))
  - The metadata is now stored in each keyring object in the `state.keyrings` array.
  - When updating to this version, we recommend removing the `keyringsMetadata` state and all state referencing a keyring ID with a migration. New metadata will be generated for each keyring automatically after the update.
### Fixed
- Keyrings with duplicate accounts are skipped as unsupported on unlock ([#5775](MetaMask/core#5775))

## [21.0.6]

### Changed

- Prevent emitting `:stateChange` from `withKeyring` unnecessarily ([#5732](MetaMask/core#5732))

```

and `@metamask/accounts-controller` to `^29.0.0`:

```markdown
## [29.0.0]

### Changed

- **BREAKING:** bump `@metamask/keyring-controller` peer dependency to `^22.0.0` ([#5802](MetaMask/core#5802))

## [28.0.0]

### Added
- Add new `setAccountNameAndSelectAccount` action ([#5714](MetaMask/core#5714))
- Add `entropySource` and `derivationPath` to EVM HD account options ([#5618](MetaMask/core#5618))
### Changed
- **BREAKING:** Bump `@metamask/snaps-controllers` peer dependency from `^9.19.0` to `^11.0.0` ([#5639](MetaMask/core#5639))
- **BREAKING:** Bump `@metamask/providers` peer dependency from `^18.1.0` to `^21.0.0` ([#5639](MetaMask/core#5639))
- Bump `@metamask/base-controller` from `^8.0.0` to `^8.0.1` ([#5722](MetaMask/core#5722))
- Bump `@metamask/snaps-sdk` from `^6.17.1` to `^6.22.0` ([#5639](MetaMask/core#5639))
- Bump `@metamask/snaps-utils` from `^8.10.0` to `^9.2.0` ([#5639](MetaMask/core#5639))
- Bump `@metamask/eth-snap-keyring` from `^12.0.0` to `^12.1.1` ([#5565](MetaMask/core#5565))
- Bump `@metamask/keyring-api` from `^17.2.0` to `^17.4.0` ([#5565](MetaMask/core#5565))
- Bump `@metamask/keyring-internal-api` from `^6.0.0` to `^6.0.1` ([#5565](MetaMask/core#5565))
### Fixed
- Do not fire events during `update` blocks ([#5555](MetaMask/core#5555))
- Prevent unnecessary state updates when updating `InternalAccount.metadata.snap` ([#5735](MetaMask/core#5735))
```

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/32621?quickstart=1)

## **Related issues**

Fixes: #32935

## **Manual testing steps**

### 1. Add duplicate to the wallet state
1.1. Switch to the `v11.7.3` branch, build locally, and install the
extension from `chrome://extensions`
1.2. Onboard with an SRP of which we know the second account (`child
guilt hollow arrive average popular nasty soon summer like scheme diary
pill country rapid`)
1.3. Import an account that is part of the mnemonic (
`0x80842b7e3cfb1118e86a427cdec418e3b4179ef5bbbfd71c02a76349831c8a8b`
which is the account at index 2 of the above SRP)
1.4. Add a new account on the main HD
1.5. Switch to `Version-v12.17.1` branch, and refresh the extension in
`chrome://extensions`
1.6. Unlock the wallet, you should see duplicates in your accounts list
and you won't be able to add new accounts

### 2. Test the fix
2.1. Switch to this branch, build locally, and refresh the extension in
`chrome://extensions`
2.2 Unlock the wallet, you shouldn't see duplicate accounts anymore, and
you should be able to add new accounts

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Mark Stacey <mark.stacey@consensys.net>
DDDDDanica pushed a commit to MetaMask/metamask-extension that referenced this pull request May 15, 2025
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

Updating `@metamask/keyring-controller` to `^22.0.0`:

```markdown

- **BREAKING** `keyringsMetadata` has been removed from the controller state ([#5725](MetaMask/core#5725))
  - The metadata is now stored in each keyring object in the `state.keyrings` array.
  - When updating to this version, we recommend removing the `keyringsMetadata` state and all state referencing a keyring ID with a migration. New metadata will be generated for each keyring automatically after the update.
- Keyrings with duplicate accounts are skipped as unsupported on unlock ([#5775](MetaMask/core#5775))

- Prevent emitting `:stateChange` from `withKeyring` unnecessarily ([#5732](MetaMask/core#5732))

```

and `@metamask/accounts-controller` to `^29.0.0`:

```markdown

- **BREAKING:** bump `@metamask/keyring-controller` peer dependency to `^22.0.0` ([#5802](MetaMask/core#5802))

- Add new `setAccountNameAndSelectAccount` action ([#5714](MetaMask/core#5714))
- Add `entropySource` and `derivationPath` to EVM HD account options ([#5618](MetaMask/core#5618))
- **BREAKING:** Bump `@metamask/snaps-controllers` peer dependency from `^9.19.0` to `^11.0.0` ([#5639](MetaMask/core#5639))
- **BREAKING:** Bump `@metamask/providers` peer dependency from `^18.1.0` to `^21.0.0` ([#5639](MetaMask/core#5639))
- Bump `@metamask/base-controller` from `^8.0.0` to `^8.0.1` ([#5722](MetaMask/core#5722))
- Bump `@metamask/snaps-sdk` from `^6.17.1` to `^6.22.0` ([#5639](MetaMask/core#5639))
- Bump `@metamask/snaps-utils` from `^8.10.0` to `^9.2.0` ([#5639](MetaMask/core#5639))
- Bump `@metamask/eth-snap-keyring` from `^12.0.0` to `^12.1.1` ([#5565](MetaMask/core#5565))
- Bump `@metamask/keyring-api` from `^17.2.0` to `^17.4.0` ([#5565](MetaMask/core#5565))
- Bump `@metamask/keyring-internal-api` from `^6.0.0` to `^6.0.1` ([#5565](MetaMask/core#5565))
- Do not fire events during `update` blocks ([#5555](MetaMask/core#5555))
- Prevent unnecessary state updates when updating `InternalAccount.metadata.snap` ([#5735](MetaMask/core#5735))
```

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/32621?quickstart=1)

Fixes: #32935

1.1. Switch to the `v11.7.3` branch, build locally, and install the
extension from `chrome://extensions`
1.2. Onboard with an SRP of which we know the second account (`child
guilt hollow arrive average popular nasty soon summer like scheme diary
pill country rapid`)
1.3. Import an account that is part of the mnemonic (
`0x80842b7e3cfb1118e86a427cdec418e3b4179ef5bbbfd71c02a76349831c8a8b`
which is the account at index 2 of the above SRP)
1.4. Add a new account on the main HD
1.5. Switch to `Version-v12.17.1` branch, and refresh the extension in
`chrome://extensions`
1.6. Unlock the wallet, you should see duplicates in your accounts list
and you won't be able to add new accounts

2.1. Switch to this branch, build locally, and refresh the extension in
`chrome://extensions`
2.2 Unlock the wallet, you shouldn't see duplicate accounts anymore, and
you should be able to add new accounts

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

<!-- [screenshots/recordings] -->

<!-- [screenshots/recordings] -->

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Mark Stacey <mark.stacey@consensys.net>
@gantunesr gantunesr added team-accounts area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. labels Jun 24, 2025
ameliejyc added a commit that referenced this pull request Sep 3, 2025
## Explanation

To remove our dependency on AccountsController and use the new BIP-44
enabled AccountTreeController, this PR contains two main changes:

- Swaps out the listener `AccountsController:selectedAccountChange` for
`AccountTreeController:selectedAccountGroupChange`
- Swaps out the action `AccountsController:getSelectedAccount` for
`AccountTreeController:getAccountsFromSelectedAccountGroup` from which
we derive the EVM-compatible account

This relies on the latest 0.12.1 release of the account-tree-controller
package.

Small things of note:
- The account listener no longer needs the account payload to update the
account address as the race condition of concern [has been
fixed](#5555) (this is relevant to
mention still because despite using the new AccountTreeController event,
it still uses AccountsController under the hood)
- In the new `getSelectedEvmAccount()` function I initially handled the
case of no account being found (which shouldn't be possible) by failing
early with a 'No EVM-compatible account address found' error. But on
seeing how this case is handled by consuming functions (sometimes
returning early, sometimes returning empty objects, sometimes throwing
error) I opted to keep it consistent with the current code

## References

Fixes the core side of this issue:
https://consensyssoftware.atlassian.net/browse/TAT-1315?atlOrigin=eyJpIjoiMmQ2MDY1ZjQ4MDU5NDdiYmJhMjRhYzNiMThhMjEwYzIiLCJwIjoiaiJ9

And should be tested alongside the accompanying mobile repo update:
MetaMask/metamask-mobile#19160

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs),
highlighting breaking changes as necessary
- [x] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. team-accounts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants