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 |
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is new author?A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package. Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
|
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. |
|
❌ Multichain API Spec Test Failed. View the report here. |
The permissions confirmation page currently ignores the `requestedChainIds` prop when the connection is confirmed. This hasn't resulted in a bug because this page is only used for snap permissions. Permission requests for `eth_account` or `endowment:permitted-chains` are handled by the "ChooseAccount" or "ConnectPage" components (the former if a snap is also requested alongside, the latter otherwise). This PR fixes the problem regardless, as it's confusing for the component to have this prop but to ignore it when processing the confirmation. This was extracted from #27782
The permissions confirmation page currently ignores the `requestedChainIds` prop when the connection is confirmed. This hasn't resulted in a bug because this page is only used for snap permissions. Permission requests for `eth_account` or `endowment:permitted-chains` are handled by the "ChooseAccount" or "ConnectPage" components (the former if a snap is also requested alongside, the latter otherwise). This PR fixes the problem regardless, as it's confusing for the component to have this prop but to ignore it when processing the confirmation. This was extracted from #27782
The E2E test setup function (`withFixtures`) has been updated to pass the extension ID to E2E tests. This will be useful in the near future for testing the new multichain API, which is exposed over `externally_connectable` and requires the extension ID to use. This was extracted from #27782
We appear to be adding an account to the `wallet` scope  Resulting in this failing CI for our [Multichain Flask PR](#27782) https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/125517/workflows/abdfdb4f-da74-4cc2-9c2f-cf001817f358/jobs/4559282 we should add it to the `wallet:eip155` scope (as currently done), but not just wallet. The fix involves refactoring [core](/~https://github.com/MetaMask/core) `Multichain` package so that creating the `scopeObjects` for each entry, we make sure that in `wallet` scope string, the accounts property is not populated. <!-- 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/30495?quickstart=1) ## **Related issues** Fixes: ## **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/main/.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/main/.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.
|
❌ Multichain API Spec Test Failed. View the report here. |
## **Description** The E2E test setup function (`withFixtures`) has been updated to pass the extension ID to E2E tests. This will be useful in the near future for testing the new multichain API, which is exposed over `externally_connectable` and requires the extension ID to use. [](https://codespaces.new/MetaMask/metamask-extension/pull/30539?quickstart=1) ## **Related issues** This was extracted from #27782 ## **Manual testing steps** N/A, this isn't used yet so there is nothing to manually test. ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](/~https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](/~https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](/~https://github.com/MetaMask/metamask-extension/blob/main/.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.
The permissions confirmation page currently ignores the `requestedChainIds` prop when the connection is confirmed. This hasn't resulted in a bug because this page is only used for snap permissions. Permission requests for `eth_account` or `endowment:permitted-chains` are handled by the "ChooseAccount" or "ConnectPage" components (the former if a snap is also requested alongside, the latter otherwise). This PR fixes the problem regardless, as it's confusing for the component to have this prop but to ignore it when processing the confirmation. This was extracted from #27782
| * | ||
| * @returns {Map<string, Caip25Authorization>} The current origin:authorization map. | ||
| */ | ||
| export const getAuthorizedScopesByOrigin = createDeepEqualSelector( |
There was a problem hiding this comment.
Given the discussion above, are we okay with using createSelector here? @FrederikBolding
| export const getAuthorizedScopesByOrigin = createDeepEqualSelector( | |
| export const getAuthorizedScopesByOrigin = createSelector( |
| /** | ||
| * Get the permitted chains for each subject, keyed by origin. | ||
| * The values of the returned map are immutable values from the | ||
| * PermissionController state. | ||
| * | ||
| * @returns {Map<string, string[]>} The current origin:chainIds[] map. | ||
| */ | ||
| export const getPermittedChainsByOrigin = createSelector( | ||
| export const getPermittedChainsByOrigin = createDeepEqualSelector( |
There was a problem hiding this comment.
And similar here:
| export const getPermittedChainsByOrigin = createDeepEqualSelector( | |
| export const getPermittedChainsByOrigin = createSelector( |
app/scripts/lib/rpc-method-middleware/handlers/wallet-createSession/index.ts
Show resolved
Hide resolved
app/scripts/lib/rpc-method-middleware/handlers/wallet-createSession/handler.ts
Outdated
Show resolved
Hide resolved
app/scripts/lib/rpc-method-middleware/handlers/wallet-createSession/handler.ts
Show resolved
Hide resolved
| console.log('MetaMask CAIP stream', err), | ||
| ); | ||
| pipeline(portStream, caipStream, portStream, (err: Error) => { | ||
| caipStream.substream.destroy(); |
There was a problem hiding this comment.
Is this related to other changes in this PR? Or is this a bug we found along the way?
There was a problem hiding this comment.
I suspect this is unrelated. I will try submitting this as a separate PR.
…30443) ## **Description** The permissions confirmation page currently ignores the `requestedChainIds` prop when the connection is confirmed. This hasn't resulted in a bug because this page is only used for snap permissions. Permission requests for `eth_account` or `endowment:permitted-chains` are handled by the "ChooseAccount" or "ConnectPage" components (the former if a snap is also requested alongside, the latter otherwise). This PR fixes the problem regardless, as it's confusing for the component to have this prop but to ignore it when processing the confirmation. This was extracted from #27782 [](https://codespaces.new/MetaMask/metamask-extension/pull/30443?quickstart=1) ## **Related issues** See this comment for some additional context: /~https://github.com/MetaMask/metamask-extension/pull/27782/files#r1936463248 ## **Manual testing steps** N/A, the impacted functionality is unreachable. ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](/~https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](/~https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](/~https://github.com/MetaMask/metamask-extension/blob/main/.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.
|
I have tested |
|
❌ Multichain API Spec Test Failed. View the report here. |
| delete transaction.unevaluatedProperties; | ||
| } | ||
|
|
||
| const chainIdMethod = openrpcDocument.methods.find( |
There was a problem hiding this comment.
Nit: Can we break up this function? It seems like it could become unmaintainable pretty fast.
| const parsedResult = await testDapp.getSession(); | ||
|
|
||
| const sessionScope = parsedResult.sessionScopes[DEFAULT_SCOPE]; | ||
| const expectedSessionScope = getExpectedSessionScope(DEFAULT_SCOPE, [ |
There was a problem hiding this comment.
I'm not understanding how this test works out of the box when we didn't create a session first. And how is the default scope eip155:1337? Is this test reliant on other tests to run first? It seems that we should at least be calling initCreateSessionScopes first in this test.
There was a problem hiding this comment.
Oh, maybe this works because of the caveat set in withPermissionControllerConnectedToMultichainTestDapp?
There was a problem hiding this comment.
Nit: It'd be nice if we could split this up into multiple files, to make this look like other e2e tests. I imagine that these tests don't follow the same shape and so we were forced to do it this way? If so, makes sense, it's just a bit difficult to read these tests in this form.
| extensionId, | ||
| ), | ||
| reporters: ['console-streaming'], | ||
| skip: [ |
There was a problem hiding this comment.
Nit: Some of these methods are already listed in ignoreMethods, can we reuse that variable?
|
@metamaskbot update-policies |
|
The multichain e2e test "Generate params from examples and expect results to match wallet_swapAsset example" is currently failing, perhaps due to invalid params or something. I will take a look at this tomorrow. |
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
](https://codespaces.new/MetaMask/metamask-extension/pull/27940?quickstart=1){kind=link}
](https://codespaces.new/MetaMask/metamask-extension/pull/30495?quickstart=1){kind=link}
](https://codespaces.new/MetaMask/metamask-extension/pull/30539?quickstart=1){kind=link}
](https://codespaces.new/MetaMask/metamask-extension/pull/30443?quickstart=1){kind=link}
|
If we can get MetaMask/api-specs#289 merged, it should fix the failing e2e spec. |
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