fix(accounts-controller): do not fire events during update blocks#5555
Conversation
326cb1c to
d8c068a
Compare
|
@metamaskbot publish-preview |
d8c068a to
aa7690c
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Note:
This logic will be deprecated when the KeyringController emits an account added event
There was a problem hiding this comment.
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)
Co-authored-by: Monte Lai <monte.lai@consensys.net>
<!-- 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)) ``` [](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>
<!-- 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)) ``` [](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>
## 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
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:accountAddedand try to callgetAccount(account.id), it might not the same value compared to theaccountbeing 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:stateChangehandler (which made the new events firing logic, a bit easier).Test E2E PR:
References
N/A
Changelog
N/A
Checklist