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

fix: App slower when changing account and switching network #9721

Merged
merged 16 commits into from
May 29, 2024

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented May 22, 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
  • 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 , 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

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • 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 format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). 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.

…he selected one, that was fixed by adding a patch to the TransactionController to only fetch per account selected
@tommasini tommasini added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-platform labels May 22, 2024
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@tommasini tommasini added the Run Smoke E2E Triggers smoke e2e on Bitrise label May 23, 2024
Copy link
Contributor

github-actions bot commented May 23, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 5833f01
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/88996dcb-9360-4380-8eb9-06a1b669121d

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels May 23, 2024
Copy link
Contributor

github-actions bot commented May 23, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 5501acc
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ab791257-d9c4-4bfe-9dac-edfaf38d0234

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@sethkfman
Copy link
Contributor

@tommasini Can you quantify the number of API calls you have reduced by this change? Is it possible that we will see improvements on UI load performance in the Wallet View?

@tommasini
Copy link
Contributor Author

tommasini commented May 24, 2024

Yeah I didn't take screenshots but we can do a comparison with the main branch using flipper @sethkfman
I can also try to quantify, I need to review the scenarios. Added to the description some counting that I did throught flipper

@tommasini tommasini marked this pull request as ready for review May 27, 2024 11:49
@tommasini tommasini requested review from a team as code owners May 27, 2024 11:49
@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels May 27, 2024
Copy link
Contributor

github-actions bot commented May 27, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 9645cb9
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/51897b36-69b3-49ec-9def-7afabd31f3d6

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.24%. Comparing base (678c2f6) to head (8f15d00).
Report is 47 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9721      +/-   ##
==========================================
+ Coverage   46.66%   47.24%   +0.58%     
==========================================
  Files        1343     1370      +27     
  Lines       32805    33304     +499     
  Branches     3527     3586      +59     
==========================================
+ Hits        15307    15736     +429     
- Misses      16554    16607      +53     
- Partials      944      961      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels May 28, 2024
Copy link
Contributor

github-actions bot commented May 28, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 8f15d00
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/5a686d52-8505-41f7-844b-e3994aa10279

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link

sonarcloud bot commented May 29, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Cal-L Cal-L added the No QA Needed Apply this label when your PR does not need any QA effort. label May 29, 2024
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM! Are these changes also applied to core already?

@tommasini
Copy link
Contributor Author

On the transaction controller changes, the way that transaction statuses are being fetched really change on next versions, so there was not a follow-up to core, althought this will be saved on a core branch to help next upgrades (The core branch is: patch/mobile-transaction-controller-13-0-0)

Regarding the assets controllers, this should be redirected to the core repo! I believe since now the focus is the controller upgrades, we can do it when we update assets-controllers to the latest version, begin to remove all the patch with core PRs
The work is saved on this branch: patch/mobile-assets-controllers-26

@tommasini tommasini merged commit 0cb5f6d into main May 29, 2024
53 checks passed
@tommasini tommasini deleted the fix/9249-app-very-slow branch May 29, 2024 10:44
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 29, 2024
@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
No QA Needed Apply this label when your PR does not need any QA effort. release-7.24.0 Issue or pull request that will be included in release 7.24.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

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