refactor(snaps-rpc-methods)!: Use messenger for restricted method implementations#3968
refactor(snaps-rpc-methods)!: Use messenger for restricted method implementations#3968FrederikBolding merged 21 commits intomainfrom
Conversation
e80f1b8 to
bd01104
Compare
| actionNames: [ | ||
| 'ApprovalController:addRequest', | ||
| 'SnapInterfaceController:createInterface', | ||
| 'SnapInterfaceController:getInterface', | ||
| 'SnapInterfaceController:setInterfaceDisplayed', | ||
| ], |
There was a problem hiding this comment.
Maybe to match the UI messenger:
| actionNames: [ | |
| 'ApprovalController:addRequest', | |
| 'SnapInterfaceController:createInterface', | |
| 'SnapInterfaceController:getInterface', | |
| 'SnapInterfaceController:setInterfaceDisplayed', | |
| ], | |
| capabilities: { | |
| actions: [ | |
| 'ApprovalController:addRequest', | |
| 'SnapInterfaceController:createInterface', | |
| 'SnapInterfaceController:getInterface', | |
| 'SnapInterfaceController:setInterfaceDisplayed', | |
| ], | |
| } |
There was a problem hiding this comment.
createRestrictedMethodMessenger uses the actionNames naming, that's why I chose to name it like that for the specifications. Do you think it is cleaner to do: actionNames: capabilities.actions?
There was a problem hiding this comment.
No strong opinion, actionNames is fine with me.
37fbc73 to
5aa93a9
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3968 +/- ##
==========================================
+ Coverage 98.56% 98.58% +0.02%
==========================================
Files 429 427 -2
Lines 12365 12399 +34
Branches 1944 1948 +4
==========================================
+ Hits 12187 12223 +36
+ Misses 178 176 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const { content, message, title, footerLink } = | ||
| validatedParams as NotificationArgs & { | ||
| content?: string; | ||
| title?: string; | ||
| footerLink?: { href: string; text: string }; | ||
| }; |
There was a problem hiding this comment.
This mess is copied from the extension, suggestions for improvements welcome 🫠
| getUnlockPromise, | ||
| getClientCryptography, | ||
| }: GetBip32EntropyMethodHooks) { | ||
| methodHooks: { getUnlockPromise, getClientCryptography }, |
There was a problem hiding this comment.
Wondering if we should add an action to the Snap controller for this, so we can fully remove the hooks?
There was a problem hiding this comment.
The problem with these hooks is that they are client-specific. There is no platform-agnostic handler for getUnlockPromise that can also show the popup when needed on extension.
There was a problem hiding this comment.
Ah, good point. That may be something to implement in the wallet service for each client, but right now this is fine.
| | ManageStateMessengerActions | ||
| | NotifyMessengerActions; | ||
|
|
||
| export type RestrictedMethodMessenger = Messenger< |
There was a problem hiding this comment.
Does this mean we create a messenger that has access to all the actions used by these methods? Should it be scoped to individual methods?
There was a problem hiding this comment.
It is narrowed to the individual methods when building the specifications for the PermissionController here: https://github.com/MetaMask/snaps/pull/3968/files#diff-610e0a55f8b1fdc2b25489c558d3338b11c4c91e5dae790f891de1e97d6109bdR84-R88
There was a problem hiding this comment.
I don't love that we have to copy (and maintain) a bunch of types. We should definitely reconsider a messenger types package for each controller.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit de25c13. Configure here.

Migrate the restricted method implementations to use the
messengerwhere applicable.https://consensyssoftware.atlassian.net/browse/WPC-995
Note
Medium Risk
Refactors multiple restricted RPC methods to use messenger-based controller actions instead of direct hook functions, which could subtly change runtime behavior and error handling across permissions, dialogs/notifications, and keyring-derived entropy. Broad test updates mitigate risk but the surface area is large and touches keyring/approval/state controllers.
Overview
Migrates restricted RPC methods to messenger-based actions.
snap_dialog,snap_notify,wallet_snap,snap_manageState, and entropy/key derivation methods now accept aMessengerand call controller actions (e.g., approvals, snap interface, snap state, keyring access, rate limiting) instead of invoking dedicated hook functions.Centralizes entropy-source access via new
getMnemonic/getMnemonicSeedutilities (backed byKeyringController:withKeyring) and adds shared action type definitions insrc/types.ts. Permission specification building is updated to create per-method restricted messengers using declaredactionNames, and tests are rewritten accordingly (with new negative cases for invalid keyrings/entropy sources). Coverage thresholds are adjusted slightly, and snaps-simulation removes the obsoleteget-mnemonic-seedand interface hook implementations/tests.Reviewed by Cursor Bugbot for commit de25c13. Bugbot is set up for automated code reviews on this repo. Configure here.