chore: cherry-pick #9764 - #9765
Merged
Merged
Conversation
<!-- 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.
owencraston
marked this pull request as ready for review
May 28, 2024 01:12
tommasini
approved these changes
May 28, 2024
Contributor
Author
|
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
This PR cherry-picks #9764