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

Lazy load Brave Crypto Wallets #5513

Merged
merged 7 commits into from
May 15, 2020
Merged

Lazy load Brave Crypto Wallets #5513

merged 7 commits into from
May 15, 2020

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented May 11, 2020

Fix brave/brave-browser#9757
Fix brave/brave-browser#8121
Fix brave/brave-browser#6595
Fix brave/brave-browser#8860
Fix brave/brave-browser#8925

Submitter Checklist:

Test Plan:

See first issue above.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@bbondy bbondy force-pushed the lazy-load-wallet branch 2 times, most recently from 8fd5675 to 5b74160 Compare May 11, 2020 19:40
@bbondy bbondy requested a review from ryanml May 12, 2020 19:27
@bbondy bbondy self-assigned this May 12, 2020
@bbondy bbondy marked this pull request as ready for review May 12, 2020 19:28
@bbondy bbondy requested a review from bridiver as a code owner May 12, 2020 19:28
Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

++ good stuff

@bbondy bbondy merged commit f79f8f4 into master May 15, 2020
@bbondy bbondy deleted the lazy-load-wallet branch May 15, 2020 15:41
@bbondy bbondy added this to the 1.11.x - Nightly milestone May 15, 2020
brave-builds pushed a commit that referenced this pull request May 22, 2020
@srirambv
Copy link
Contributor

Verification passed on

Brave 1.11.25 Chromium: 81.0.4044.138 (Official Build) nightly (64-bit)
Revision 8c6c7ba89cc9453625af54f11fd83179e23450fa-refs/branch-heads/4044@{#999}
OS Linux

Test for #9757

Scenario 1:

  • Verified on a fresh profile there is no process started until opted in.
  • Verified closing the browser and reopening doesn't list Crypto Wallet process
  • Verified closing and reopening browser and visiting brave://wallet start the process
  • Verified with process running, visiting Crypto Kitties doesn't show info bar and window.web3 is available in the tab
  • Verified closing and relaunching browser and visiting Crypto Kitties shows info bar to start CW and reload the tab
  • Verified setting process to always load works between relaunches

Scenario 2:

  • Verified on a clean profile visiting CryptoKittes brings up info bar to setup Crypto
  • Verified clicking on setup in info bar loads brave://wallet in new tab and shows opt-in page
  • Verified after opt-in reloading the page provides window.web3 on the tab

Scenario 3:

  • Verified installing MM and setting it as default doesn't start the process.
  • Verified visiting CryptoKittes and setting MM as default also sets in setting
  • Close and relaunch browser doesn't start the process
  • Verified opting to CW doesn't change settings for default web3 provider

Scenario 4:

  • Verified installing MM on a clean profile and visiting CryptoKitties shows info bar to choose default Dapp provider. Selecting CW sets its as default Web3 provider in settings
  • Verified closing and relaunching the browser and visiting CryptoKitties shows info bar asking to restart the CW process

Scenario 5:

  • Verified from an existing profile where CW is fully setup, upgrading shows no process for CW in task manager
  • Verified visiting brave://wallet loads the extension and works fine

Scenario 6:

  • Verified from an existing profile where CW is loaded but not setup, upgrading shows no process for CW in task manager
  • Verified visiting brave://wallet shows opt-in page

Ran into #9821 for scenarios 5/6 which is fixed in #5591


Test for #8121

  • Verified on a clean profile visiting CryptoKitties shows info bar and selecting CW loads brave://wallet in a new tab and goes into opt-in state

Test for #6595

  • Verified having a window size of 400px by 400px and visiting brave://wallet loads the opt-in webUI which is responsive and scrollable

Test for #8925

Verified there is no constant polling in the background when the process is not running


Test for #8860

  • Verified there is no Infura calls when the process is not running

Test for #8120

  • Verified visiting a Dapp automatically triggers info bar to setup CryptoWallet
  • Verified if MM is installed and visiting a Dapp automatically triggers info bar to choose between CW and MM as default Dapp provider

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment