fix: update handleAccountRemoved logic #4322
Merged
Conversation
This file contains 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
owencraston
previously approved these changes
May 27, 2024
|
@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. |
7 tasks
owencraston
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
May 28, 2024
<!-- 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** 1. What is the reason for the change? - This error occurs one a fresh install after importing an account from SRP that has multiple accounts WITH balances (In my case I was testing an account that had 5 addresses with balances). The reason for this is because when importing an SRP, each account gets imported one by one until we find an address that has a zero balance. [Once we find the address with a zero balance, we remove that account and the account import is complete](https://github.com/MetaMask/metamask-mobile/blob/main/app/util/importAdditionalAccounts.js#L49-L61). This unveiled an issue in the AccountsController (which is not being used to display the wallet info) because when a new account is added we [eagerly set the latest account as the selected account](https://github.com/MetaMask/core/blob/v123.0.0/packages/accounts-controller/src/AccountsController.ts#L751). This caused two issues. - The selected account after an import would be the last account in the list. This differs from production which defaults to the first account. - Since the last account in the list was set to selected, when it gets removed in this step [here](https://github.com/MetaMask/metamask-mobile/blob/main/app/util/importAdditionalAccounts.js#L59), the UI/Controller are still referencing the account that just got deleted, causing a race condition and resulting in the crash we see in the video. 3. What is the improvement/solution? - To solve this bug I am doing one simple thing, not setting the latest added account to selected. This comes in the form of a patch due to the urgency of this fix but we will work to find a better more long term solution in the latest accounts controller release. - We are already working on a longer term fix [here](MetaMask/core#4322). - This does not effect the selected account logic in the UI since the [front end is already manually setting an account to selected when a new account is added](https://github.com/MetaMask/metamask-mobile/blob/e0a4bec1ce82415a3be892e719194ff829beb05f/app/components/Views/AddAccountActions/AddAccountActions.tsx#L44). This can be seen in my vide below. ## **Related issues** Fixes: #9749 ## **Manual testing steps** 1. Run this branch without any previous instance of metamask installed 3. import a wallet from SRP that has multiple addresses with a balance. This is essential to truly test this feature. 4. after importing the account, you should be sent to the wallet home screen 5. wait for things to load because this screen is very very slow. 6. NOTICE that the selected account is always account 1 7. open the account picker and notice that their are multiple addresses created with the correct balances 8. there should be no crash 9. click on the account picker and add a new account 10. this account should get created and should now become the selected account 11. force close the app by swiping it away and then reopen 12. upon unlocking the wallet the selected account should be the same account that was selected before. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** https://github.com/MetaMask/metamask-mobile/assets/22918444/2396608d-c30e-4421-88e3-e14b21a799e8 ### **After** https://github.com/MetaMask/metamask-mobile/assets/22918444/9b0ec7d2-4d48-4142-b2b8-dd680a392b3a NOTE: The performance of this home screen is terrible and is not related to the AccountsController. @Cal-L has [separate incoming fix for this](#9753). ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/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-mobile/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.
metamaskbot
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
May 28, 2024
<!-- 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** 1. What is the reason for the change? - This error occurs one a fresh install after importing an account from SRP that has multiple accounts WITH balances (In my case I was testing an account that had 5 addresses with balances). The reason for this is because when importing an SRP, each account gets imported one by one until we find an address that has a zero balance. [Once we find the address with a zero balance, we remove that account and the account import is complete](https://github.com/MetaMask/metamask-mobile/blob/main/app/util/importAdditionalAccounts.js#L49-L61). This unveiled an issue in the AccountsController (which is not being used to display the wallet info) because when a new account is added we [eagerly set the latest account as the selected account](https://github.com/MetaMask/core/blob/v123.0.0/packages/accounts-controller/src/AccountsController.ts#L751). This caused two issues. - The selected account after an import would be the last account in the list. This differs from production which defaults to the first account. - Since the last account in the list was set to selected, when it gets removed in this step [here](https://github.com/MetaMask/metamask-mobile/blob/main/app/util/importAdditionalAccounts.js#L59), the UI/Controller are still referencing the account that just got deleted, causing a race condition and resulting in the crash we see in the video. 3. What is the improvement/solution? - To solve this bug I am doing one simple thing, not setting the latest added account to selected. This comes in the form of a patch due to the urgency of this fix but we will work to find a better more long term solution in the latest accounts controller release. - We are already working on a longer term fix [here](MetaMask/core#4322). - This does not effect the selected account logic in the UI since the [front end is already manually setting an account to selected when a new account is added](https://github.com/MetaMask/metamask-mobile/blob/e0a4bec1ce82415a3be892e719194ff829beb05f/app/components/Views/AddAccountActions/AddAccountActions.tsx#L44). This can be seen in my vide below. ## **Related issues** Fixes: #9749 ## **Manual testing steps** 1. Run this branch without any previous instance of metamask installed 3. import a wallet from SRP that has multiple addresses with a balance. This is essential to truly test this feature. 4. after importing the account, you should be sent to the wallet home screen 5. wait for things to load because this screen is very very slow. 6. NOTICE that the selected account is always account 1 7. open the account picker and notice that their are multiple addresses created with the correct balances 8. there should be no crash 9. click on the account picker and add a new account 10. this account should get created and should now become the selected account 11. force close the app by swiping it away and then reopen 12. upon unlocking the wallet the selected account should be the same account that was selected before. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** https://github.com/MetaMask/metamask-mobile/assets/22918444/2396608d-c30e-4421-88e3-e14b21a799e8 ### **After** https://github.com/MetaMask/metamask-mobile/assets/22918444/9b0ec7d2-4d48-4142-b2b8-dd680a392b3a NOTE: The performance of this home screen is terrible and is not related to the AccountsController. @Cal-L has [separate incoming fix for this](#9753). ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/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-mobile/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.
Andepande
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
May 28, 2024
<!-- 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** 1. What is the reason for the change? - This error occurs one a fresh install after importing an account from SRP that has multiple accounts WITH balances (In my case I was testing an account that had 5 addresses with balances). The reason for this is because when importing an SRP, each account gets imported one by one until we find an address that has a zero balance. [Once we find the address with a zero balance, we remove that account and the account import is complete](https://github.com/MetaMask/metamask-mobile/blob/main/app/util/importAdditionalAccounts.js#L49-L61). This unveiled an issue in the AccountsController (which is not being used to display the wallet info) because when a new account is added we [eagerly set the latest account as the selected account](https://github.com/MetaMask/core/blob/v123.0.0/packages/accounts-controller/src/AccountsController.ts#L751). This caused two issues. - The selected account after an import would be the last account in the list. This differs from production which defaults to the first account. - Since the last account in the list was set to selected, when it gets removed in this step [here](https://github.com/MetaMask/metamask-mobile/blob/main/app/util/importAdditionalAccounts.js#L59), the UI/Controller are still referencing the account that just got deleted, causing a race condition and resulting in the crash we see in the video. 3. What is the improvement/solution? - To solve this bug I am doing one simple thing, not setting the latest added account to selected. This comes in the form of a patch due to the urgency of this fix but we will work to find a better more long term solution in the latest accounts controller release. - We are already working on a longer term fix [here](MetaMask/core#4322). - This does not effect the selected account logic in the UI since the [front end is already manually setting an account to selected when a new account is added](https://github.com/MetaMask/metamask-mobile/blob/e0a4bec1ce82415a3be892e719194ff829beb05f/app/components/Views/AddAccountActions/AddAccountActions.tsx#L44). This can be seen in my vide below. ## **Related issues** Fixes: #9749 ## **Manual testing steps** 1. Run this branch without any previous instance of metamask installed 3. import a wallet from SRP that has multiple addresses with a balance. This is essential to truly test this feature. 4. after importing the account, you should be sent to the wallet home screen 5. wait for things to load because this screen is very very slow. 6. NOTICE that the selected account is always account 1 7. open the account picker and notice that their are multiple addresses created with the correct balances 8. there should be no crash 9. click on the account picker and add a new account 10. this account should get created and should now become the selected account 11. force close the app by swiping it away and then reopen 12. upon unlocking the wallet the selected account should be the same account that was selected before. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** https://github.com/MetaMask/metamask-mobile/assets/22918444/2396608d-c30e-4421-88e3-e14b21a799e8 ### **After** https://github.com/MetaMask/metamask-mobile/assets/22918444/9b0ec7d2-4d48-4142-b2b8-dd680a392b3a NOTE: The performance of this home screen is terrible and is not related to the AccountsController. @Cal-L has [separate incoming fix for this](#9753). ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/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-mobile/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.
|
I think that we should still merge this PR since it is a better long term fix for the related issue. |
montelaidev
force-pushed
the
fix/set-selectedAccount-and-remove-in-the-same-step
branch
from
2008174 to
c439acc
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. |
owencraston
approved these changes
Jun 5, 2024
owencraston
previously approved these changes
Jun 12, 2024
|
@metamaskbot publish-preview |
|
@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. |
Gudahtt
reviewed
Jun 18, 2024
| this.update((currentState: Draft<AccountsControllerState>) => { | ||
| delete currentState.internalAccounts.accounts[accountId]; | ||
| const updatedState = deepCloneDraft(currentState); |
There was a problem hiding this comment.
Why would we deep clone the state here? This is an Immer draft, it's perfectly safe to mutate. That's the entire purpose of Immer drafts, to convert mutations into immutable state changes.
There was a problem hiding this comment.
Its more to get around the type error. MetaMask/utils#168. I left a comment here to remove it once the error is gone
There was a problem hiding this comment.
I am not following. How does deep cloning state get around at type error? I understand that the deepCloneDraft function itself has type assertions that may be needed due to a type error, but deep cloning happens at runtime. We don't need to do anything at runtime to work around type errors that happen at build time. We work around this type error everywhere else using a ts-expect-error comment, when it does get in our way.
Why would we deep clone here at all? Why would anyone ever want to deep clone an Immer draft? It seems like a misunderstanding of what this draft is for.
There was a problem hiding this comment.
You're right, we should use ts-expect-error instead. Its been changed in this commit 2ff7fce
Gudahtt
reviewed
Jun 18, 2024
| @@ -731,25 +731,25 @@ export class AccountsController extends BaseController< | |||
|
|
|||
| // handle if the selected account was deleted | |||
| if (!this.getAccount(this.state.internalAccounts.selectedAccount)) { | |||
There was a problem hiding this comment.
It looks like the selected account is already being updated in this block. So now it's being updated twice redundantly.
Could you elaborate further on what this was intended to accomplish?
There was a problem hiding this comment.
I see, it was to ensure that the account was removed in the first update. That makes sense, though surely we can refactor this to avoid making the same state update again redundantly.
There was a problem hiding this comment.
The onKeyringStateChange has been changed to only update once at the end. 3f27d64
|
@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. |
|
@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. |
|
@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. |
ccharly
reviewed
Jun 24, 2024
Comment on lines
+1869
to
+1882
| expect(accountsController.getSelectedAccount()).toStrictEqual({ | ||
| id: '', | ||
| address: '', | ||
| options: {}, | ||
| methods: [], | ||
| type: EthAccountType.Eoa, | ||
| metadata: { | ||
| name: '', | ||
| keyring: { | ||
| type: '', | ||
| }, | ||
| importTime: 0, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
I've seen multiple times already, we should probably export a const EMPTY_ACCOUNT alongside the AccountsController so that we can check against this value (in the tests, but potentially elsewhere too).
Overall, I really feel this is a bit fragile, but this is not the scope of this PR.
I'll try to make a separate PR to address this.
ccharly
approved these changes
Jun 25, 2024
|
@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. |
montelaidev
deleted the
fix/set-selectedAccount-and-remove-in-the-same-step
branch
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
This PR modifies the handleAccountRemoved function to update the selected account to the most recent one, if the account being removed is currently the selected account, all within the same update.
References
Related to MetaMask/metamask-mobile#9749
Changelog
@metamask/accounts-controllergetSelectedAccountto handle case when there are no accounts fromgetAccountExpectChecklist