feat: Add Multichain API to Flask #27782
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. |
|
@metamaskbot update-policies |
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
|
this PR needs the patches from here: https://github.com/MetaMask/metamask-extension/pull/27847/files#r1801195961 |
jiexi
changed the base branch from
caip-multichain
to
caip25-permission-migration
|
|
app/scripts/lib/rpc-method-middleware/handlers/wallet-createSession/handler.js
Outdated
Show resolved
Hide resolved
app/scripts/lib/rpc-method-middleware/handlers/wallet-createSession/handler.js
Outdated
Show resolved
Hide resolved
app/scripts/lib/rpc-method-middleware/handlers/wallet-createSession/handler.js
Outdated
Show resolved
Hide resolved
app/scripts/lib/rpc-method-middleware/handlers/wallet-createSession/handler.js
Outdated
Show resolved
Hide resolved
|
Done here #29003 |
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [](https://codespaces.new/MetaMask/metamask-extension/pull/27940?quickstart=1) ## **Related issues** Fixes: #27782 (comment) ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). 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.
|
@metamaskbot update-policies |
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
|
❌ Multichain API Spec Test Failed. View the report here. |
|
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected] |
|
❌ Multichain API Spec Test Failed. View the report here. |
|
New problem: |
|
Yeah, I'm not sure what changed with |
|
Looks like it's currently working in the branch being used in the Frankenstein SIP-26 environment: https://github.com/MetaMask/MetaMask-planning/issues/3989 |
|
I think I got the One thing I'm noticing, however, is that the tests for |
Builds ready [0b26c90]
Page Load Metrics (1740 ± 103 ms)
|
| }), | ||
| }), | ||
| ); | ||
|
|
There was a problem hiding this comment.
I just realized that a subset of the Snaps API is not made available over the multichain API. Not sure if that is intended, I was under the impression that we would still support wallet_* methods over the multichain API.
Probably not a blocker for merging though.
There was a problem hiding this comment.
Which methods specifically?
There was a problem hiding this comment.
wallet_getSnaps, wallet_invokeSnap, wallet_requestSnaps etc.
There was a problem hiding this comment.
ah, yes that does seem like an oversight. Should be fairly easy to fix though. Agreed it shouldn't be a blocker for this PR
There was a problem hiding this comment.
I will create a ticket for this
There was a problem hiding this comment.
That was actually intended. We expected to keep those in the "legacy API". But, we should discuss whether that is limiting is some way.
There was a problem hiding this comment.
| @@ -30,6 +30,7 @@ | |||
| "etherscan.io", | |||
| "execution.metamask.io", | |||
| "fonts.gstatic.com", | |||
| "foo.io", | |||
There was a problem hiding this comment.
Hm, this endpoint should be mocked or the api-specs file updated. I have zero trust for the current owner of foo.io to not mess with our testing infrastructure.
There are TLDs that are specifically reserved for use in examples and tests, and have been guaranteed by IETF to never be registered as real TLDs:
- .example
- .invalid
- .localhost
- .test
shared/constants/app.ts
Outdated
| WATCH_ASSET: 'wallet_watchAsset', | ||
| WALLET_CREATE_SESSION: 'wallet_createSession', | ||
| WALLET_GET_SESSION: 'wallet_getSession', | ||
| WALLET_INVOKE_METHOD: 'wallet_invokeMethod', | ||
| WALLET_REVOKE_SESSION: 'wallet_revokeSession', | ||
| WALLET_SESSION_CHANGED: 'wallet_sessionChanged', |
There was a problem hiding this comment.
nit: these new constants should be grouped with the other WALLET_ constant. Probably just a merge issue, but it'd be nice if they were together.
There was a problem hiding this comment.
Makes sense! Updated in 6b690a1.
app/scripts/background.js
Outdated
| connectExternalCaip(...args); | ||
| } else { | ||
| const isDappConnecting = port.sender.tab?.id; | ||
| if (!process.env.MULTICHAIN_API || !isDappConnecting) { |
There was a problem hiding this comment.
nit: using double negation with || here is every so slightly harder to read than a logical AND. Any reason for inverting the original condition (and if not, can you change it back pretty please :-D ).
Builds ready [63f0ba4]
Page Load Metrics (1797 ± 138 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
](https://codespaces.new/MetaMask/metamask-extension/pull/27940?quickstart=1){kind=link}
Builds ready [c700391]
Page Load Metrics (1978 ± 109 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| await hooks.requestPermissionApprovalForOrigin({ | ||
| [Caip25EndowmentPermissionName]: { | ||
| caveats: [ | ||
| { | ||
| type: Caip25CaveatType, | ||
| value: requestedCaip25CaveatValueWithSupportedEthAccounts, | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
|
|
||
| const approvedCaip25Permission = | ||
| approvedPermissions[Caip25EndowmentPermissionName]; | ||
| const approvedCaip25CaveatValue = approvedCaip25Permission?.caveats?.find( | ||
| (caveat) => caveat.type === Caip25CaveatType, | ||
| )?.value as Caip25CaveatValue; | ||
| if (!approvedCaip25CaveatValue) { | ||
| throw rpcErrors.internal(); | ||
| } | ||
|
|
||
| const sessionScopes = getSessionScopes(approvedCaip25CaveatValue); | ||
|
|
||
| hooks.grantPermissions({ | ||
| subject: { | ||
| origin, | ||
| }, | ||
| approvedPermissions, | ||
| }); |
There was a problem hiding this comment.
We should modify this to use the PermissionController.requestPermissions directly rather than ApprovalController -> PermissionController.grantPermissions
Same idea as the refactor we did here: https://github.com/MetaMask/metamask-extension/pull/30042/files#diff-6fbff2cfe97ac01b77296ef2122c7e0a5b3ff6a84b584b4d1a87482f35eea3d6
There was a problem hiding this comment.
Its ok if we do this in a follow up PR
Description
This branch adds support for the Multichain API to the Flask build of the Extension.
The existing API (via injected provider) should be completely unchanged.
(Very Briefly) What is the MetaMask Multichain API
externally_connectable. Not accessible via an injected global likewindow.ethereumKey Documents/Standards
mip = MetaMask Improvement Proposal
Manual testing steps
Then
(RECOMMENDED) Use the Multichain Test Dapp
OR
Form requests manually
Pre-merge author checklist
Pre-merge reviewer checklist