metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Published 20 hours ago • verified publisher icon MetaMask

feat: Prevent rpc node requests on locked state

Open pedronfigueiredo opened this issue 1 year ago • 7 comments

Description

See the task below for more details.

Related issues

Fixes: #2151

Manual testing steps

  1. Checkout this branch
  2. Build the app
  3. Lock the wallet
  4. Open the dev tools on the network tab
  5. Refresh the wallet
  6. No requests to rpc node should be visible

Screenshots/Recordings

N/A

Pre-merge author checklist

  • [ ] I’ve followed MetaMask Coding Standards.
  • [ ] I've clearly explained what problem this PR is solving and how it is solved.
  • [ ] I've linked related issues
  • [ ] I've included manual testing steps
  • [ ] I've included screenshots/recordings if applicable
  • [ ] 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.
  • [ ] I’ve properly set the pull request status:
    • [ ] In case it's not yet "ready for review", I've set it to "draft".
    • [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

pedronfigueiredo avatar Feb 26 '24 15:02 pedronfigueiredo

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.

github-actions[bot] avatar Feb 26 '24 15:02 github-actions[bot]

Builds ready [1f57f78]
Page Load Metrics (1010 ± 401 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint721631122311
domContentLoaded106127189
load5618851010835401
domInteractive106127189
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 49 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 29 '24 09:02 metamaskbot

Codecov Report

Attention: Patch coverage is 65.00000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 68.69%. Comparing base (3d861ca) to head (900034f).

:exclamation: Current head 900034f differs from pull request most recent head 8560806

Please upload reports for the commit 8560806 to get more accurate results.

Files Patch % Lines
app/scripts/metamask-controller.js 65.00% 7 Missing :warning:
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #23175      +/-   ##
===========================================
+ Coverage    65.77%   68.69%   +2.92%     
===========================================
  Files         1366     1098     -268     
  Lines        54255    43132   -11123     
  Branches     14102    11523    -2579     
===========================================
- Hits         35685    29627    -6058     
+ Misses       18570    13505    -5065

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 29 '24 09:02 codecov[bot]

Builds ready [2614f5f]
Page Load Metrics (1474 ± 329 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint862681655125
domContentLoaded1778462210
load7220201474684329
domInteractive1778462210
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 49 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 29 '24 13:02 metamaskbot

Builds ready [9509369]
Page Load Metrics (1544 ± 295 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint612051453517
domContentLoaded1085382311
load4920851544614295
domInteractive1085382311
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 209 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 29 '24 18:02 metamaskbot

Builds ready [900034f]
Page Load Metrics (1071 ± 427 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint742681425426
domContentLoaded1088302210
load6122131071889427
domInteractive1088302210
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 209 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Mar 01 '24 18:03 metamaskbot

Recap from a Slack conversation to address comment by @danjm here.

Given performance concerns we will introduce a setting Optimize performance when opening MetaMask which reflects the current behavior of pulling data while MetaMask is locked; To avoid the UI getting bogged down and latency affecting users' ability to provide their password.

The setting would be offered as Opt-out and will be included in onboarding and the Settings menu inside Extension itself.

This work is pending an update to Advanced Configuration.

hesterbruikman avatar Mar 06 '24 14:03 hesterbruikman

I attempted at a clean merge to sync with develop in f465de5fbe08e4f6747b3ddb3a24df89881d7c0d but it seems like there is something left to be done, looking at e2e results.

legobeat avatar May 30 '24 04:05 legobeat

Closing this PR as the work has been handed over to the Wallet UX team.

pedronfigueiredo avatar Jun 25 '24 15:06 pedronfigueiredo