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

refactor: wallet_createSession should call PermissionController.requestPermissions instead of ApprovalController.addAndShowApprovalRequest #30908

Conversation

ffmcgee725
Copy link
Member

@ffmcgee725 ffmcgee725 commented Mar 11, 2025

Description

We created a merger for the Caip25Permission specification in order to enable wallet_addEthereumChain and wallet_switchEthereumChain to call to the PermissionController rather than the ApprovalController for incremental permission requests (i.e. for the permittedChains flow).

PRs implementing these changes
core: MetaMask/core#5283
extension: #30042

We also changed eth_requestAccounts and wallet_requestPermissions handlers to call the PermissionController, but we should now do the change to wallet_createSession handler as well and address this feedback on the Flask Multichain PR (at the moment this is only available on the Flask Multichain PR)

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

yarn start:flask

Then

(RECOMMENDED) Use the Multichain Test Dapp

OR

Form requests manually

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

@ffmcgee725 ffmcgee725 marked this pull request as ready for review March 11, 2025 15:46
@ffmcgee725 ffmcgee725 requested a review from a team March 11, 2025 15:46
@metamaskbot
Copy link
Collaborator

Builds ready [1240e5b]
Page Load Metrics (2149 ± 113 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint43125642028410197
domContentLoaded18162460207418086
load187428082149234113
domInteractive2711742209
backgroundConnect15449779244
firstReactRender16125392713
getState6156383416
initialActions01000
loadScripts13771939159014469
setupStore85817147
uiStartup202442502546469225

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [7346118]
Page Load Metrics (1823 ± 70 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31521291740350168
domContentLoaded16212063175511153
load16362139182314670
domInteractive25124352512
backgroundConnect18251746833
firstReactRender1571352110
getState5132373919
initialActions01000
loadScripts1207159313319344
setupStore869252110
uiStartup182632602226426205

@adonesky1 adonesky1 merged commit b710fbb into jl/caip-multichain-migrate-core Mar 11, 2025
66 of 68 checks passed
@adonesky1 adonesky1 deleted the refactor/wallet-createSession-call-permission-controller branch March 11, 2025 18:54
@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants