feat: Add CAIP-25 permission and adapters to @metamask/multichain
#4784
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 tasks
## Explanation This PR fixes a lot of the linting and typescript errors. still some left but this covers a lot of it. <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/package-a` - **<CATEGORY>**: Your change here - **<CATEGORY>**: Your change here ### `@metamask/package-b` - **<CATEGORY>**: Your change here - **<CATEGORY>**: Your change here ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Jiexi Luan <jiexiluan@gmail.com>
## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> Added ESM exports for multichain package ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/package-a` - **<CATEGORY>**: Your change here - **<CATEGORY>**: Your change here ### `@metamask/package-b` - **<CATEGORY>**: Your change here - **<CATEGORY>**: Your change here ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
jiexi
commented
Oct 11, 2024
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
adonesky1
force-pushed
the
caip-multichain
branch
from
1e2e990 to
d10f7d0
Compare
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
adonesky1
force-pushed
the
caip-multichain
branch
from
06e6dd8 to
a532409
Compare
adonesky1
reviewed
Nov 13, 2024
Comment on lines
+147
to
+163
| // Fetch EVM accounts from native wallet keyring | ||
| // These addresses are lowercased already | ||
| const existingEvmAddresses = methodHooks | ||
| .listAccounts() | ||
| .map((account) => account.address); | ||
| const ethAccounts = getEthAccounts({ | ||
| requiredScopes: normalizedRequiredScopes, | ||
| optionalScopes: normalizedOptionalScopes, | ||
| }).map((address) => address.toLowerCase() as Hex); | ||
|
|
||
| const allEthAccountsSupported = ethAccounts.every((address) => | ||
| existingEvmAddresses.includes(address), | ||
| ); | ||
| if (!allEthAccountsSupported) { | ||
| throw new Error( | ||
| `${Caip25EndowmentPermissionName} error: Received eip155 account value(s) for caveat of type "${Caip25CaveatType}" that were not found in the wallet keyring.`, | ||
| ); |
There was a problem hiding this comment.
Is there are reason we don't do this in the assertScopesSupported function like we do with methods/notifications? And why don't we use the isSupportedAccount helper? I'm not seeing it used anywhere atm?
There was a problem hiding this comment.
Also I kindof thought we were going to ignore this - just filter out requested accounts that aren't in the wallet... But probably not a big deal either way
There was a problem hiding this comment.
I don't necessarily think we need to block on this. We can resolve it in #4813
There was a problem hiding this comment.
I tried updating this to use isSupportedAccount locally, and found it was more trouble than it was worth. isSupportedAccount takes the CAIP account ID as input, but here we're filtering to just Eth accounts first. Which does seem easier, because then we're not re-checking the same account twice each time we see it referenced in a new scope.
Maybe we don't need isSupportedAccount. Seems fine delete it, at least for now. But agreed that it's not a blocker.
There was a problem hiding this comment.
I did the same and decided to leave it for now. I'll leave it in since it seems to include some filtering related to SCAs that we may want soon?
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Gudahtt
previously approved these changes
Nov 13, 2024
Gudahtt
approved these changes
Nov 13, 2024
github-merge-queue bot
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Jan 20, 2025
…27847) <!-- 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** This PR replaces the replaces the internal `eth_accounts` and `endowment:permittedChains` permission structure with a [CAIP-25](https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-25.md) endowment. It adds adapter logic to translate to and from the new internal CAIP-25 permissions. This change should be transparent to wallet users and to dapps except for ~one~ two cases, see below. This change is required in order to support CAIP-25 and CAIP-27 requests in a follow-up PR that enables the Multichain API. [](https://codespaces.new/MetaMask/metamask-extension/pull/27847?quickstart=1) ## **Related issues** Related: MetaMask/core#4784 ## **Manual testing steps** There should be no user or dapp facing difference in behavior except: * When calling `wallet_revokePermissions` and specifying either `eth_accounts` or `endowment:permitted-chains`, the entire CAIP-25 permission will be revoked. It will appear to the dapp as if both `eth_accounts` and `endowment:permitted-chains` were revoked. * When calling `wallet_getPermissions` for a permitted dapp when the wallet is **locked**, `eth_accounts` should be returned in addition to `endowment:permitted-chains`. Currently there is a regression on `main` where only `endowment:permitted-chains` gets returned when the wallet is locked. ``` await window.ethereum.request({ "method": "wallet_revokePermissions", "params": [ { eth_accounts: {} } ], }); await window.ethereum.request({ "method": "wallet_revokePermissions", "params": [ { 'endowment:permitted-chains': {} } ], }); await window.ethereum.request({ "method": "wallet_getPermissions", "params": [], }); ``` ### Locked Wallet Behavior with dapp connected Other than the two noted items below, this behavior matches that in `main` - `eth_accounts` returns [] - `wallet_getPermissions` returns permissions incl eth_accounts - `wallet_revokePermissions` works as usual and revokes eth_accounts and revoke permitted-chains together - * Note this fixes a regression in `main` where eth_accounts and permitted-chains aren't revoked as a pair if either is revoked - `eth_requestAccounts` prompts for unlock, after unlock returns accounts if any are permitted, otherwise shows connection prompt - `wallet_requestPermissions` prompts for unlock - signature methods fails with method or accounts not authorized - non-signature methods work as usual - `accountsChanged` empty array on lock. no event after revokePermissions which makes sense since the dapp was told empty array on lock and now it's actually empty array so no changes have occurred as far as the dapp should be concerned. - **CHANGED**: for dapps that were granted chain permissions via the `wallet_addEthereum` or `wallet_switchEthereumChain` flows without account permissions, these permissions will be removed with this migration. We think this ok because: - This is a very uncommon scenario for dapps to request chain switches without account permissions. - These permissions can be regained very trivially with subsequent chain switch requests. ### Testing the migration * Create a dev build from `main` * Install the dev build from the `dist/chrome` directory and proceed through onboarding * Run this command in the background console: ``` chrome.storage.local.get( null, (state) => { state.data.PermissionController = {}; // Replace this line based on instructions below chrome.storage.local.set(state, () => chrome.runtime.reload()); } ); ``` * Disable the extension * Switch to `main` and create a dev build * Enable and reload the extension * You should see in the console that migration 139 has failed Repeat the above steps but with the line above replaced with the following for example: * `state.data.NetworkController = {}; ` * `state.data.NetworkController = 'foobar'; ` * `state.data.NetworkController.selectedNetworkClientId = null;` * `state.data.NetworkController.networkConfigurationsByChainId = 'foobar';` ## **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** - [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/develop/.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/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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com> Co-authored-by: Alex <adonesky@gmail.com> Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com> Co-authored-by: Mark Stacey <markjstacey@gmail.com> Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com> Co-authored-by: Frederik Bolding <frederik.bolding@gmail.com>
](https://codespaces.new/MetaMask/metamask-extension/pull/27847?quickstart=1){kind=link}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
This PR updates
@metamask/multichainto provide types, CAIP-25 permission, and helpers/adapters for the new permission, which can be shared across the extension & mobile clients.These tools and utilities will be used in both clients (mobile + extension)'s multichain API implementations.
File Overview
packages/multichain/src/adapters/: Helpers that get and set legacy permission values from and to the new CAIP-25 permissionpackages/multichain/src/caip25Permission.ts: Constants, types, mutators, and a specification builder for a CAIP-25 permissionpackages/multichain/src/index.ts: Barrel exportpackages/multichain/src/scope/: Types for CAIP-217 and our internal normalized/flattened version of them. Additionally contains helpers for validating shape, normalizing/merging, and checking support (i.e. if the wallet is able to serve the chain with it's requested methods and notifications)References
Upstream: #4812
Downstream: #4813
Key Multichain API Standards implemented here:
scopedefintionOpen PR that uses this new package for migrating the legacy permissions to CAIP-25 permission in the extension: MetaMask/metamask-extension#27847
Changelog
@metamask/multichainChecklist