Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: app very slow and sluggish when switching accounts especially on android device #9250

Closed
chrisleewilcox opened this issue Apr 15, 2024 · 0 comments · Fixed by #9721
Closed
Assignees
Labels
area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. regression-RC-7.21.0 release-7.24.0 Issue or pull request that will be included in release 7.24.0 Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing team-mobile-platform type-bug Something isn't working

Comments

@chrisleewilcox
Copy link
Contributor

Describe the bug

When switching accounts on wallet view the app is very slow and sluggish to finish switching accounts.

Wallet setup:

  • 3 wallet accounts
  • 1 imported account
  • 1 imported ENS account (notice that ENS accounts in the wallet seem to contribute to slowness)
  • 2 hw accounts

Physical device:

  • Samsung A42 Android 13
  • iPhone 12 iOS 17.4

Expected behavior

Switching accounts should be very responsive in the app without noticeable delays.

Screenshots/Recordings

Uploading Screen Recording 2024-04-15 at 12.43.42 PM.mov…

Steps to reproduce

GIVEN I am on wallet view
WHEN I tap account drop-down (randomly very slow to display select account modal)
AND I tap account to select (randomly very slow to switch to accounts and select account modal goes away)
THEN wallet view is displayed (randomly wallet view is shaded before for awhile before I can tap or interact)
AND I can immediately tap or interact with wallet view (randomly very slow when I tap a component on wallet view)

Error messages or log output

No response

Version

7.21.0

Build type

None

Device

Any

Operating system

iOS, Android

Additional context

No response

Severity

No response

@chrisleewilcox chrisleewilcox added type-bug Something isn't working area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. team-mobile-platform labels Apr 15, 2024
@metamaskbot metamaskbot added the regression-prod-7.21.0 Regression bug that was found in production in release 7.21.0 label Apr 15, 2024
@gauthierpetetin gauthierpetetin added the Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking label Apr 15, 2024
@SamuelSalas SamuelSalas added Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing and removed Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking labels May 3, 2024
tommasini added a commit that referenced this issue May 29, 2024
## **Description**
This PR reduces the number of api calls that happen when switching
account and network, on cold and warm app starts.

* Transactions are being fetched for all the accounts instead of just
the selected one, that was fixed by adding a patch to the
TransactionController to only fetch per account selected (36 fetches to
eth_getTransactionReceipt and counted 40+ eth_getBlockByHash, to 0 since
when we are switching network on an account that doesn't have
transactions)
Screenshot of main branch with eth_getTransactionReceipt requests:
The selected account was not orangefox.eth

https://etherscan.io/tx/0x7ce730e2e87520e179d4e351e5cfe80565c346f9ab7688a3391306422e01f0a9
<img
src="https://github.com/MetaMask/metamask-mobile/assets/46944231/f4e2df99-20a2-4156-b16a-7fa26ed6af98"
width=650 />

* Fixed a bug on Transaction controller where the getBlockByHash request
was failing silently because it was missing one param.

* ~~ENS fetches now only once we switch network and we change the
selected account (This was looping by number of accounts and being
retriggered, one 1 second, counted 15+ fetches to [ens
contract](https://etherscan.io/address/0xa2c122be93b0074270ebee7f6b7292c7deb45047)
, now it's just fetching once it switches network and for the selected
account)~~
* Request parallelization of token balances. (This was not optimized on
number of requests but on the parallelization of the requests to have a
response quicker from all of them)
* Update pooling of token balances from 10000 to 180000 miliseconds
* Added a boolean to not let the updateBalances runs if there is an
update in progress. (Probably change this to use mutex when implementing
on core it's a good idea)

A snapshot was updated on this PR! It seems that this snapshot was
indeed wrong, and now it have the right mocked data.

Check this transaction controller core branch with the changes:
mobile-patch-performance-tx-controller-13-0-0
(Once this is approved, this changes need to be merged on the
patch/mobile-transaction-controller-13-0-0 branch)

Assets controller patch easy to ready, the core branch is this one:
patch/mobile-assets-controllers-26-performance.
(Once this is approved, this changes need to be merged on the
patch/mobile-assets-controllers-26 branch)


**Next steps**

* Postpone transactions requests to views that will show transactions
(exg: Asset View, Activity View)



**Product Changes**
* ~~ENS is now get by the selected account and not every account~~ This
was addressed in other PR already
* Transactions status is now updated only on the selected account (This
still decrease the number of requests if the user has multiple accounts
with multiple transactions)

## **Related issues**

Fixes: #9250 #9249 

## **Manual testing steps**

1. 
2.
3.

## **Screenshots/Recordings**


### **Before**

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

### **After**

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

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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 metamaskbot added the release-7.24.0 Issue or pull request that will be included in release 7.24.0 label May 29, 2024
@gauthierpetetin gauthierpetetin added regression-RC-7.21.0 and removed regression-prod-7.21.0 Regression bug that was found in production in release 7.21.0 labels Jun 28, 2024
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. regression-RC-7.21.0 release-7.24.0 Issue or pull request that will be included in release 7.24.0 Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing team-mobile-platform type-bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants