feat: add wallet_switchEthereumChain permission and enforce it (behind feature flag)
#24415
Conversation
|
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. |
adonesky1
changed the title
wallet_switchEthereumChain permission
Builds ready [df5964e]
Page Load Metrics (1308 ± 546 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [4f5a98c]
Page Load Metrics (1242 ± 512 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
There was a problem hiding this comment.
I've tested this out locally and it seems to work as advertised.
Heads up that I have a PR open to group network configurations by chain ID. I've left some notes on how this code will change once that PR is merged. (Don't worry, it will be a while, as we are waiting for UX, but I thought you might want to know.)
Only piece of feedback that we could consider changing now is the networkConfigurationId argument in switchChain.
| import { PermissionNames } from '../../../controllers/permissions'; | ||
| import { getValidUrl } from '../../util'; | ||
|
|
||
| export function findExistingNetwork(chainId, findNetworkConfigurationBy) { |
There was a problem hiding this comment.
Again this will change following MetaMask/core#4286 — Infura networks will be integrated into network configurations and it will be possible to look it up by calling getNetworkConfigurationByChainId on the network controller. So you will be able to simplify this to:
export function findExistingNetwork(chainId, getNetworkConfigurationByChainId) {
return getNetworkConfigurationByChainId(chainId);
}or even just inline this function into wherever it's being used.
app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.js
Outdated
Show resolved
Hide resolved
| }), | ||
| ); | ||
| } | ||
| const networkClientIdToSwitchTo = |
There was a problem hiding this comment.
Heads up that this will change following MetaMask/core#4286, and you'll probably want something like:
const networkConfigurationForRequestedChainId =
getNetworkConfigurationByChainId(chainId);
const networkClientIdToSwitchTo = networkConfigurationForRequestedChainId
.rpcEndpointsByUrl[networkConfigurationForRequestedChainId.defaultRpcEndpointUrl]
.networkClientId;| 'nativeCurrency', | ||
| ]), | ||
| const currentChainIdForDomain = getCurrentChainIdForDomain(origin); | ||
| const currentNetworkConfiguration = findExistingNetwork( |
There was a problem hiding this comment.
Heads up that this will change following MetaMask/core#4286, and you'll probably want something like:
const currentNetworkConfiguration = getNetworkConfigurationByChainId(chainIdForDomain);(and similar for below)
| if (currentChainId === _chainId && currentRpcUrl === firstValidRPCUrl) { | ||
| return end(); | ||
| } | ||
| if (!existingNetwork || existingNetwork.rpcUrl !== firstValidRPCUrl) { |
There was a problem hiding this comment.
This will also change after MetaMask/core#4286. Network configurations will have multiple RPC endpoints and also a default RPC endpoint to use. I'm not sure exactly how this will look but you may be able to simplify this.
| fromNetworkConfiguration: currentNetworkConfiguration, | ||
| }; | ||
| } else { | ||
| networkConfigurationId = existingNetwork.id ?? existingNetwork.type; |
There was a problem hiding this comment.
Also following MetaMask/core#4286, the network configuration won't have an id property; rather, RPC endpoints will have an associated networkClientId.
| ); | ||
| } | ||
|
|
||
| removeNetworkConfiguration(networkConfigurationId) { |
There was a problem hiding this comment.
This whole function will probably change following MetaMask/core#4286. There will only ever be one network configuration associated with a chain. removeNetworkConfiguration will be replaced with removeNetwork and take a chainId rather than a networkConfigurationId. So we may simply be able to say:
removeNetwork(chainId) {
this.removeAllChainIdPermissions(chainId);
this.networkController.removeNetwork(chainId);
}
Builds ready [2619ccb]
Page Load Metrics (920 ± 627 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…ed test isolation
…ed test isolation - switchEthereumChain handler tests
This reverts commit 85dbab8.
Builds ready [ec84f4f]
Page Load Metrics (401 ± 380 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
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 ☔ View full report in Codecov by Sentry. |
metamaskbot
added
the
release-11.18.0
Description
This PR introduces the
wallet_switchEthereumChainpermission (as anendowmenttype permission) behind a feature flag (CHAIN_PERMISSIONS), allowing us to remove thewallet_switchEthereumconfirmation. Now instead of seeing this confirmation everytimewallet_switchEthereumChainis 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.Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2327
Manual testing steps
CHAIN_PERMISSIONS=1 yarn startwallet-uxteam.Screenshots/Recordings
Before
Screen.Recording.2024-05-14.at.2.57.09.PM.mov
After
Screen.Recording.2024-05-14.at.3.01.13.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist