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

[IMPROVEMENT] Use Set when filtering blocklist #5641

Merged

Conversation

Cal-L
Copy link
Contributor

@Cal-L Cal-L commented Jan 30, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

This PR patches the PhishingController to use Set when filtering on the blocklist. The improvement here (at least testing locally on iOS) is that look up in Set vs using includes while filtering reduces same business logic processing time by 4x. This translates to a user experiencing 4x faster cold start times than before.

Issue

Progresses #5382

Test

We should expect cold start times on iOS to be ~4seconds, while Android (depending on device) shouldn't take more than 10 seconds. (Both times reported by Curtis). Relative to the previous app version, the cold start up times should be noticeably lower.

@Cal-L Cal-L added needs-qa Any New Features that needs a full manual QA prior to being added to a release. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-client labels Jan 30, 2023
@Cal-L Cal-L requested a review from a team as a code owner January 30, 2023 20:16
@github-actions
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.

@sethkfman sethkfman added Mobile QA board release-6.0.0 PRs for v6.0.0 release and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-client labels Jan 30, 2023
@chrisleewilcox chrisleewilcox added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 31, 2023
@chrisleewilcox
Copy link
Contributor

Test Scenarios:

  • Multiple (numerous) browser tabs opened and connected to websites and dapps (opensea, uni, 1inch, reddit, etc...)
  • Navigate through opensea
  • Navigate throughout app while numerous browser tabs connected (Settings, Activity, Wallet view, Import and Request tokens, etc...)
  • Execute transactions while numerous browser tabs connected (Send, Swap, QR scan, HW transactions, etc...)

Expected: mobile app usage is not negatively impacted

@chrisleewilcox
Copy link
Contributor

Issue #1: Browser: opensea.io: tapping MetaMask icon on connect wallet takes you download MetaMask page.
Expected: Wallet connects to opensea
https://recordit.co/cVKsYZSFgx

Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

Please review issues.

@chrisleewilcox
Copy link
Contributor

chrisleewilcox commented Jan 31, 2023

Issue 2: Uniswap: Swaps: UI components for token rate on swap screen are not loading
https://recordit.co/a5Gi54FKyu
https://recordit.co/oRTeCz3M7C

NOTE: This currently exist in prod but sites/dapps that are not loading UI fully may be contributing to slowness. This is not a Uniswap specific issue but this issue is using Uniswap as a specific example.

@chrisleewilcox chrisleewilcox added the QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed label Feb 1, 2023
@cortisiko
Copy link
Member

Regarding Issue 1 I cannot reproduce it on a physical device: http://recordit.co/qzgkprczPr
And sim: http://recordit.co/kKp5jKeIo7

Maybe opensea was having a connectivity issue earlier in the day.

Regarding issue 2, This seems dapp specific. Uniswap has Skelton loaders that appear for swap prices.

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed QA in Progress QA has started on the feature. labels Feb 1, 2023
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@chrisleewilcox chrisleewilcox merged commit 1f3e3c7 into main Feb 1, 2023
@chrisleewilcox chrisleewilcox deleted the improvement/5382-improve-phishing-controller-load-time branch February 1, 2023 18:03
@github-actions github-actions bot locked and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Performance Improvement QA Passed A successful QA run through has been done release-6.0.0 PRs for v6.0.0 release team-mobile-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants