refactor: migrate wallet handlers (and unit tests) #5420
Merged
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
ffmcgee725
force-pushed
the
refactor/migrate-wallet-permission-handlers
branch
from
08eb8d8 to
a2fc641
Compare
ffmcgee725
force-pushed
the
refactor/migrate-wallet-permission-handlers
branch
from
a2fc641 to
e98eb52
Compare
Member
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
mcmire
previously approved these changes
Mar 5, 2025
Contributor
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Had some minor comments, but nothing blocking. Looks good.
Comment on lines
1
to
12
| export const CaveatTypes = Object.freeze({ | ||
| restrictReturnedAccounts: 'restrictReturnedAccounts' as const, | ||
| restrictNetworkSwitching: 'restrictNetworkSwitching' as const, | ||
| }); | ||
|
|
||
| export const EndowmentTypes = Object.freeze({ | ||
| permittedChains: 'endowment:permitted-chains', | ||
| }); | ||
|
|
||
| export const RestrictedMethods = Object.freeze({ | ||
| eth_accounts: 'eth_accounts', | ||
| }); |
Contributor
There was a problem hiding this comment.
Nit: This would fit more closely to how we write enums across the monorepo, e.g. here, so should we use this instead? (We also don't need the as const after each value.)
Suggested change
| export const CaveatTypes = Object.freeze({ | |
| restrictReturnedAccounts: 'restrictReturnedAccounts' as const, | |
| restrictNetworkSwitching: 'restrictNetworkSwitching' as const, | |
| }); | |
| export const EndowmentTypes = Object.freeze({ | |
| permittedChains: 'endowment:permitted-chains', | |
| }); | |
| export const RestrictedMethods = Object.freeze({ | |
| eth_accounts: 'eth_accounts', | |
| }); | |
| export enum CaveatTypes { | |
| RestrictReturnedAccounts = 'restrictReturnedAccounts', | |
| RestrictNetworkSwitching = 'restrictNetworkSwitching', | |
| } | |
| export enum EndowmentTypes { | |
| PermittedChains = 'endowment:permitted-chains', | |
| } | |
| export enum RestrictedMethods { | |
| EthAccounts = 'eth_accounts', | |
| } |
If we want "quasi-enums" instead of real enums (there are good reasons for doing so), then what about:
Suggested change
| export const CaveatTypes = Object.freeze({ | |
| restrictReturnedAccounts: 'restrictReturnedAccounts' as const, | |
| restrictNetworkSwitching: 'restrictNetworkSwitching' as const, | |
| }); | |
| export const EndowmentTypes = Object.freeze({ | |
| permittedChains: 'endowment:permitted-chains', | |
| }); | |
| export const RestrictedMethods = Object.freeze({ | |
| eth_accounts: 'eth_accounts', | |
| }); | |
| export const CaveatTypes = Object.freeze({ | |
| RestrictReturnedAccounts: 'restrictReturnedAccounts', | |
| RestrictNetworkSwitching: 'restrictNetworkSwitching', | |
| }); | |
| export type CaveatTypes = (typeof CaveatTypes)[keyof typeof CaveatTypes]; | |
| export const EndowmentTypes = Object.freeze({ | |
| PermittedChains: 'endowment:permitted-chains', | |
| }); | |
| export type EndowmentTypes = | |
| (typeof EndowmentTypes)[keyof typeof EndowmentTypes]; | |
| export const RestrictedMethods = Object.freeze({ | |
| EthAccounts: 'eth_accounts', | |
| }); | |
| export type RestrictedMethods = | |
| (typeof RestrictedMethods)[keyof typeof RestrictedMethods]; |
Member
Author
There was a problem hiding this comment.
Thank you for the reference.
If you could provide an explanation as to why there are good reasons for us wanting "quasi-enums" in this case, would be much appreciated.
For now I followed through with the first suggestion, mostly due to less type gymnastic and easier readability (although I am aware of Typescript Enums not being the best), but we can re-visit if necessary.
Resolved in 047d5b0
Contributor
There was a problem hiding this comment.
Moving away from enums is a topic that's been tossed around for a while. You have to be careful using them — numeric enums, in particular, are a footgun, and the reverse mapping that TypeScript creates is kind of weird — and they're not part of the ECMAScript spec, they're a TypeScript-addon.
We have an ESLint rule which enforces using string enums, which helps. But, we don't really need them. Node >= 22 recently introduced an --experimental-strip-types which makes it so that you can run TypeScript files in Node without transpiling by erasing type syntax on the fly, and TypeScript 5.8 adds an --erasableSyntaxOnly flag which enforces any syntax that can be erasable. If we went that direction — and it would be very nice, as we wouldn't need to use tsx or the like — then we would need to drop use of enums and other things which we don't use anyway.
There's some more discussion here: https://consensys.slack.com/archives/C1L7H42BT/p1740524804317039
| }; | ||
|
|
||
| /** | ||
| * Get Permissions implementation to be used in JsonRpcEngine middleware. |
Contributor
There was a problem hiding this comment.
Nit: It would be nice to explain that this handler is for the wallet_getPermissions RPC method, and that it makes use of a CAIP-25 endowment permission returned by getPermissionsForOrigin if it exists, or something like that. But perhaps we can add that later.
| >[0]; | ||
|
|
||
| /** | ||
| * Request Permissions implementation to be used in JsonRpcEngine middleware. |
Contributor
There was a problem hiding this comment.
Nit: It would be nice to explain that the request object is expected to contain a CAIP-25 endowment permission. But perhaps we can add that later.
| * @param end - JsonRpcEngine end() callback | ||
| * @param options - Method hooks passed to the method implementation | ||
| * @param options.revokePermissionsForOrigin - A hook that revokes given permission keys for an origin | ||
| * @returns A promise that resolves to nothing |
Contributor
There was a problem hiding this comment.
Nit: This isn't an async method, so maybe we should say:
Suggested change
| * @returns A promise that resolves to nothing | |
| * @returns nothing. |
Member
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
ffmcgee725
added a commit
that referenced
this pull request
Mar 6, 2025
## Explanation - Updates the @metamask/multichain package to use new hooks provided by the Snaps MultichainRouter to enable non-evm support on the Multichain API. ([#5191](#5191)) - Migrates wallet permission handlers from `extension` repo. ([#5420](#5420)) <!-- 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 - **BREAKING**: `getSessionScopes()` now expects an additional hooks object as its last param. The hooks object should have a `getNonEvmSupportedMethods` property whose value should be a function that accepts a `CaipChainId` and returns an array of supported methods. ([#5191](#5191)) - **BREAKING**: `caip25CaveatBuilder()` now expects two additional properties it's singular param object. The param object should now also have a `isNonEvmScopeSupported` property whose value should be a function that accepts a `CaipChainId` and returns a boolean, and a `getNonEvmAccountAddresses` property whose value should be a function that accepts a `CaipChainId` and returns an array of CAIP-10 account addresses. ([#5191](#5191)) - The CAIP-25 caveat specification now also validates if non-evm scopes and accounts are supported - **BREAKING**: The `wallet_getSession` handler now expects `getNonEvmSupportedMethods` to be provided in it's hooks. ([#5191](#5191)) - The handler now resolves methods for non-evm scopes in the returned `sessionScopes` result - **BREAKING**: The `wallet_invokeMethod` handler now expects `getNonEvmSupportedMethods` and `handleNonEvmRequestForOrigin` to be provided in it's hooks. `handleNonEvmRequestForOrigin` should be a function with the following signature: - ``` handleNonEvmRequestForOrigin: (params: { connectedAddresses: CaipAccountId[]; scope: CaipChainId; request: JsonRpcRequest; }) => Promise<Json>; ``` - The handler now supports handling non-evm requests - **BREAKING**: `assertScopeSupported()` now expects the following hooks params object as it's last param: - ``` { isChainIdSupported: (chainId: Hex) => boolean; isEvmChainIdSupported: (chainId: Hex) => boolean; isNonEvmScopeSupported: (scope: CaipChainId) => boolean; getNonEvmSupportedMethods: (scope: CaipChainId) => string[]; } ``` - **BREAKING**: `assertScopesSupported()` now expects the following hooks params object as it's last param: - ``` { isChainIdSupported: (chainId: Hex) => boolean; isEvmChainIdSupported: (chainId: Hex) => boolean; isNonEvmScopeSupported: (scope: CaipChainId) => boolean; getNonEvmSupportedMethods: (scope: CaipChainId) => string[]; } ``` - **BREAKING**: `bucketScopes()` now expects the following hooks params object as it's last param: - ``` { isEvmChainIdSupported: (chainId: Hex) => boolean; isEvmChainIdSupportable: (chainId: Hex) => boolean; isNonEvmScopeSupported: (scope: CaipChainId) => boolean; getNonEvmSupportedMethods: (scope: CaipChainId) => string[]; } ``` - **BREAKING**: `bucketScopesBySupport()` now expects the following hooks params object as it's last param: - ``` { isEvmChainIdSupported: (chainId: Hex) => boolean; isNonEvmScopeSupported: (scope: CaipChainId) => boolean; getNonEvmSupportedMethods: (scope: CaipChainId) => string[]; } ``` - **BREAKING**: `getSessionScopes()` now expects a hooks object as its last param. The hooks object should have a `getNonEvmSupportedMethods` property whose value should be a function that accepts a `CaipChainId` and returns an array of supported methods. - **BREAKING**: `isSupportedScopeString()` now expects the a hooks params object as it's last param with the following properties: - ``` { isEvmChainIdSupported: (chainId: Hex) => boolean; isNonEvmScopeSupported: (scope: CaipChainId) => boolean; } ``` - **BREAKING**: `isSupportedAccount()` now expects the a hooks params object as it's last param with the following properties: - ``` { getEvmInternalAccounts: () => { type: string; address: Hex }[]; getNonEvmAccountAddresses: (scope: CaipChainId) => string[]; } ``` - **BREAKING**: `isSupportedMethod()` now expects the a hooks params object as it's last param with the following properties: - ``` { getNonEvmSupportedMethods: (scope: CaipChainId) => string[]; } ``` - Added `wallet_getPermissions` handler (originally migrated from extension repo) ([#5420](#5420)) - Added `wallet_requestPermissions` handler (originally migrated from extension repo) ([#5420](#5420)) - Added `wallet_revokePermissions` handler (originally migrated from extension repo) ([#5420](#5420)) <!-- 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". --> ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
matthewwalsh0
pushed a commit
that referenced
this pull request
Mar 6, 2025
## Explanation - Updates the @metamask/multichain package to use new hooks provided by the Snaps MultichainRouter to enable non-evm support on the Multichain API. ([#5191](#5191)) - Migrates wallet permission handlers from `extension` repo. ([#5420](#5420)) <!-- 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 - **BREAKING**: `getSessionScopes()` now expects an additional hooks object as its last param. The hooks object should have a `getNonEvmSupportedMethods` property whose value should be a function that accepts a `CaipChainId` and returns an array of supported methods. ([#5191](#5191)) - **BREAKING**: `caip25CaveatBuilder()` now expects two additional properties it's singular param object. The param object should now also have a `isNonEvmScopeSupported` property whose value should be a function that accepts a `CaipChainId` and returns a boolean, and a `getNonEvmAccountAddresses` property whose value should be a function that accepts a `CaipChainId` and returns an array of CAIP-10 account addresses. ([#5191](#5191)) - The CAIP-25 caveat specification now also validates if non-evm scopes and accounts are supported - **BREAKING**: The `wallet_getSession` handler now expects `getNonEvmSupportedMethods` to be provided in it's hooks. ([#5191](#5191)) - The handler now resolves methods for non-evm scopes in the returned `sessionScopes` result - **BREAKING**: The `wallet_invokeMethod` handler now expects `getNonEvmSupportedMethods` and `handleNonEvmRequestForOrigin` to be provided in it's hooks. `handleNonEvmRequestForOrigin` should be a function with the following signature: - ``` handleNonEvmRequestForOrigin: (params: { connectedAddresses: CaipAccountId[]; scope: CaipChainId; request: JsonRpcRequest; }) => Promise<Json>; ``` - The handler now supports handling non-evm requests - **BREAKING**: `assertScopeSupported()` now expects the following hooks params object as it's last param: - ``` { isChainIdSupported: (chainId: Hex) => boolean; isEvmChainIdSupported: (chainId: Hex) => boolean; isNonEvmScopeSupported: (scope: CaipChainId) => boolean; getNonEvmSupportedMethods: (scope: CaipChainId) => string[]; } ``` - **BREAKING**: `assertScopesSupported()` now expects the following hooks params object as it's last param: - ``` { isChainIdSupported: (chainId: Hex) => boolean; isEvmChainIdSupported: (chainId: Hex) => boolean; isNonEvmScopeSupported: (scope: CaipChainId) => boolean; getNonEvmSupportedMethods: (scope: CaipChainId) => string[]; } ``` - **BREAKING**: `bucketScopes()` now expects the following hooks params object as it's last param: - ``` { isEvmChainIdSupported: (chainId: Hex) => boolean; isEvmChainIdSupportable: (chainId: Hex) => boolean; isNonEvmScopeSupported: (scope: CaipChainId) => boolean; getNonEvmSupportedMethods: (scope: CaipChainId) => string[]; } ``` - **BREAKING**: `bucketScopesBySupport()` now expects the following hooks params object as it's last param: - ``` { isEvmChainIdSupported: (chainId: Hex) => boolean; isNonEvmScopeSupported: (scope: CaipChainId) => boolean; getNonEvmSupportedMethods: (scope: CaipChainId) => string[]; } ``` - **BREAKING**: `getSessionScopes()` now expects a hooks object as its last param. The hooks object should have a `getNonEvmSupportedMethods` property whose value should be a function that accepts a `CaipChainId` and returns an array of supported methods. - **BREAKING**: `isSupportedScopeString()` now expects the a hooks params object as it's last param with the following properties: - ``` { isEvmChainIdSupported: (chainId: Hex) => boolean; isNonEvmScopeSupported: (scope: CaipChainId) => boolean; } ``` - **BREAKING**: `isSupportedAccount()` now expects the a hooks params object as it's last param with the following properties: - ``` { getEvmInternalAccounts: () => { type: string; address: Hex }[]; getNonEvmAccountAddresses: (scope: CaipChainId) => string[]; } ``` - **BREAKING**: `isSupportedMethod()` now expects the a hooks params object as it's last param with the following properties: - ``` { getNonEvmSupportedMethods: (scope: CaipChainId) => string[]; } ``` - Added `wallet_getPermissions` handler (originally migrated from extension repo) ([#5420](#5420)) - Added `wallet_requestPermissions` handler (originally migrated from extension repo) ([#5420](#5420)) - Added `wallet_revokePermissions` handler (originally migrated from extension repo) ([#5420](#5420)) <!-- 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". --> ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
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
Original ticket
Part of the scope of https://github.com/MetaMask/MetaMask-planning/issues/4129 is having the currently existing handlers for:
wallet_getPermissionswallet_revokePermissionswallet_requestPermissionsAvailable in the mobile codebase.
This raises an issue, where if future changes are required in any of these handlers, those would need to be done on both extension and mobile codebases, instead of having one single source of truth. Therefore, as part of this https://github.com/MetaMask/MetaMask-planning/issues/4129, we could extract the existing handlers on extension repo over to core and create a release for that so they can be imported in whichever code base needs them.
As part of this work, create another refactor ticket for these to later be imported on extension repo and removed from that codebase altogether (not urgent at the moment).
References
Original files from
extensionrepo:wallet_requestPermissions
wallet_requestPermissions test file
wallet_revokePermissions
wallet_revokePermissions test file
wallet_getPermissions
wallet_getPermissions test file
Changelog
@metamask/multichain-apiwallet_getPermissionshandler (originally migrated fromextensionrepo)wallet_requestPermissionshandler (originally migrated fromextensionrepo)wallet_revokePermissionshandler (originally migrated fromextensionrepo)Checklist