metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard
verified publisher icon MetaMask

feat: add `wallet_switchEthereumChain` permission and enforce it (behind feature flag)

Open adonesky1 opened this issue 1 year ago • 10 comments

Description

This PR introduces the wallet_switchEthereumChain permission (as an endowment type permission) behind a feature flag (CHAIN_PERMISSIONS), allowing us to remove the wallet_switchEthereum confirmation. Now instead of seeing this confirmation everytime wallet_switchEthereumChain is called for a given chain, users sees a confirmation to add a permission for the dapp to switch to this chain after which they will no longer see any confirmation at all when the dapp next attempts to switch to this chain.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2327

Manual testing steps

  1. CHAIN_PERMISSIONS=1 yarn start
  2. Go to a dapp (uniswap is good for this test) and connect the wallet
  3. Use the in dapp switch chain UI
  4. You should see a permission confirmation (instead of the old switch chain confirmation)
    • The UI is not fully implemented, this will be completed by the wallet-ux team.
  5. Manually switch to another chain in the wallet
  6. Go back to the dapp and switch to the same chain you just added permissions for
  7. You should not see any confirmation but the network should change

Screenshots/Recordings

Before

https://github.com/MetaMask/metamask-extension/assets/34557516/6f6321ef-5fb5-48db-84a2-790cf178ea7f

After

https://github.com/MetaMask/metamask-extension/assets/34557516/c94c67be-f17d-4b9d-aec0-431ea89500f5

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.

adonesky1 avatar May 07 '24 18:05 adonesky1

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 May 07 '24 18:05 github-actions[bot]

Builds ready [89955a4]
Page Load Metrics (823 ± 631 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint60215983919
domContentLoaded9341573
load4832278231313631
domInteractive9341573
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -8.2 KiB (-0.25%)
  • ui: 889 Bytes (0.01%)
  • common: 12.77 KiB (0.21%)

metamaskbot avatar May 14 '24 20:05 metamaskbot

Codecov Report

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

Project coverage is 67.43%. Comparing base (4233f9e) to head (2619ccb). Report is 30 commits behind head on develop.

:exclamation: Current head 2619ccb differs from pull request most recent head 80fbb98

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

Files Patch % Lines
...method-middleware/handlers/ethereum-chain-utils.js 57.35% 58 Missing :warning:
app/scripts/metamask-controller.js 4.44% 43 Missing :warning:
.../scripts/controllers/permissions/specifications.js 12.82% 34 Missing :warning:
...c-method-middleware/handlers/add-ethereum-chain.js 61.70% 18 Missing :warning:
...e-container/permission-page-container.component.js 0.00% 14 Missing :warning:
...scripts/controllers/permissions/caveat-mutators.js 8.33% 11 Missing :warning:
...ethod-middleware/handlers/switch-ethereum-chain.js 55.00% 9 Missing :warning:
ui/helpers/utils/permission.js 0.00% 1 Missing :warning:
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #24415      +/-   ##
===========================================
+ Coverage    67.41%   67.43%   +0.02%     
===========================================
  Files         1288     1289       +1     
  Lines        50233    50340     +107     
  Branches     13013    13051      +38     
===========================================
+ Hits         33863    33943      +80     
- Misses       16370    16397      +27

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

codecov[bot] avatar May 14 '24 20:05 codecov[bot]

Builds ready [cefcd71]
Page Load Metrics (727 ± 553 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint5812983178
domContentLoaded8311252
load4728507271152553
domInteractive8311252
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -8.2 KiB (-0.25%)
  • ui: 889 Bytes (0.01%)
  • common: 12.77 KiB (0.21%)

metamaskbot avatar May 15 '24 15:05 metamaskbot

I think this comment got dismissed due to a rebase, so restating it (re: app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.test.js):

Are there any test cases for when the chain permissions feature flag is enabled?

rekmarks avatar May 15 '24 21:05 rekmarks

Are there any test cases for when the chain permissions feature flag is enabled?

Not but I will add them. I was waiting for more general validation of the approach before proceeding with the tests. Should've added that todo context in the PR description

adonesky1 avatar May 15 '24 21:05 adonesky1

Refactored Switch and Add chain handlers, still todo:

  • [x] Add tests
  • [x] Handle permission removal when networkConfiguration is deleted

adonesky1 avatar May 16 '24 21:05 adonesky1

Builds ready [4ab4c5e]
Page Load Metrics (1368 ± 565 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint731511042210
domContentLoaded105018126
load59279413681177565
domInteractive105018126
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -7.82 KiB (-0.23%)
  • ui: 856 Bytes (0.01%)
  • common: 12.6 KiB (0.20%)

metamaskbot avatar May 16 '24 21:05 metamaskbot

Builds ready [0b05c11]
Page Load Metrics (664 ± 494 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint69133922010
domContentLoaded9411694
load5726286641029494
domInteractive9411694
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -7.58 KiB (-0.23%)
  • ui: 1012 Bytes (0.01%)
  • common: 12.86 KiB (0.21%)

metamaskbot avatar May 17 '24 20:05 metamaskbot

Builds ready [c1798c9]
Page Load Metrics (982 ± 541 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7212589147
domContentLoaded9191331
load5926409821126541
domInteractive9191331
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -7.58 KiB (-0.23%)
  • ui: 1012 Bytes (0.01%)
  • common: 12.86 KiB (0.21%)

metamaskbot avatar May 20 '24 21:05 metamaskbot

DRYing up the addEthereumChain and switchEthereumChain implementations. I recommend creating a module like lib/rpc-method-middleware/handlers/ethereum-chain-utils.js or something along those lines.

@rekmarks I've DRY'ed a bunch and made a shared module as you recommend!

adonesky1 avatar May 21 '24 21:05 adonesky1

Builds ready [df5964e]
Page Load Metrics (1308 ± 546 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint64145972512
domContentLoaded96916136
load52288613081138546
domInteractive96916136
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -9.07 KiB (-0.27%)
  • ui: 863 Bytes (0.01%)
  • common: 12.86 KiB (0.21%)

metamaskbot avatar May 21 '24 21:05 metamaskbot

Builds ready [4f5a98c]
Page Load Metrics (1242 ± 512 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint69168912211
domContentLoaded97218157
load56236912421066512
domInteractive97218157
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -9.05 KiB (-0.27%)
  • ui: 963 Bytes (0.01%)
  • common: 12.76 KiB (0.21%)

metamaskbot avatar May 23 '24 14:05 metamaskbot

Builds ready [2619ccb]
Page Load Metrics (920 ± 627 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint691771083316
domContentLoaded96123168
load5734889201306627
domInteractive96123168
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -9.06 KiB (-0.27%)
  • ui: 963 Bytes (0.01%)
  • common: 12.76 KiB (0.21%)

metamaskbot avatar May 23 '24 19:05 metamaskbot

Builds ready [ec84f4f]
Page Load Metrics (401 ± 380 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6413488199
domContentLoaded94116105
load512369401791380
domInteractive94116105
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -9.06 KiB (-0.27%)
  • ui: 963 Bytes (0.01%)
  • common: 12.74 KiB (0.21%)

metamaskbot avatar May 24 '24 02:05 metamaskbot

Codecov Report

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

Project coverage is 67.43%. Comparing base (4233f9e) to head (ec84f4f). Report is 31 commits behind head on develop.

Files Patch % Lines
...method-middleware/handlers/ethereum-chain-utils.js 57.35% 58 Missing :warning:
app/scripts/metamask-controller.js 4.44% 43 Missing :warning:
.../scripts/controllers/permissions/specifications.js 13.16% 33 Missing :warning:
...c-method-middleware/handlers/add-ethereum-chain.js 61.70% 18 Missing :warning:
...e-container/permission-page-container.component.js 0.00% 14 Missing :warning:
...scripts/controllers/permissions/caveat-mutators.js 8.33% 11 Missing :warning:
...ethod-middleware/handlers/switch-ethereum-chain.js 55.00% 9 Missing :warning:
ui/helpers/utils/permission.js 0.00% 1 Missing :warning:
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #24415      +/-   ##
===========================================
+ Coverage    67.41%   67.43%   +0.02%     
===========================================
  Files         1288     1289       +1     
  Lines        50233    50340     +107     
  Branches     13013    13050      +37     
===========================================
+ Hits         33863    33943      +80     
- Misses       16370    16397      +27

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

codecov-commenter avatar May 24 '24 02:05 codecov-commenter