refactor!: Standardise SnapController action/event names and types#3907
refactor!: Standardise SnapController action/event names and types#3907Mrtenz wants to merge 14 commits intocontroller-refactorsfrom
SnapController action/event names and types#3907Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## controller-refactors #3907 +/- ##
=====================================================
Coverage 98.55% 98.55%
=====================================================
Files 425 425
Lines 12358 12360 +2
Branches 1935 1935
=====================================================
+ Hits 12180 12182 +2
Misses 178 178 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -0,0 +1,773 @@ | |||
| #!yarn tsx | |||
There was a problem hiding this comment.
Can you explain the diff between this version and the one in core? Are we intending to sync back these changes?
There was a problem hiding this comment.
The only difference is support for nested directories. The core script assumes all controllers are in the top-level src directory, which is not the case for Snaps controllers. We can sync this with core if we decide to create a package for the script.
There was a problem hiding this comment.
Definitely don't want them to diverge.
| const allSnaps = this.#messenger.call('SnapController:getAllSnaps'); | ||
| const filteredSnaps = getRunnableSnaps(allSnaps); |
There was a problem hiding this comment.
Since we are making a breaking change here we could consider swapping to use SnapController: getRunnableSnaps
There was a problem hiding this comment.
Can do it in the follow up PR for MultichainRouter.
| #getSnapsWithPermission(permissionName: string) { | ||
| const allSnaps = this.messenger.call('SnapController:getAll'); | ||
| const allSnaps = this.messenger.call('SnapController:getAllSnaps'); | ||
| const filteredSnaps = getRunnableSnaps(allSnaps); |
There was a problem hiding this comment.
Same as MultichainRouter
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
packages/snaps-controllers/src/snaps/SnapController-method-action-types.ts
Show resolved
Hide resolved
| 'incrementActiveReferences', | ||
| 'decrementActiveReferences', |
There was a problem hiding this comment.
I wonder if we should just remove this 😄
There was a problem hiding this comment.
Maybe, but seems out of scope for this pull request.
There was a problem hiding this comment.
Sure, but maybe worth doing in same major version
|
|
||
| messenger.delegate({ | ||
| actions: [ | ||
| 'PermissionController:hasPermission', |
| const originalTerminateFunction = service.terminateJob.bind(service); | ||
|
|
||
| let promise: Promise<unknown>; | ||
| let promise: Promise<unknown> = Promise.resolve(); |
There was a problem hiding this comment.
Without this, TypeScript complains about promise not being assigned.
2 participants
This renames all
SnapControlleraction and event names and types to follow theController...Actionpattern used in most other controllers. I've also added thegenerate-method-actionsscript used inMetaMask/coreto automatically generate these types.I found numerous unrelated type errors in test files that I fixed in this pull request as well, since it was a bit difficult to determine if a type error was caused by this refactor or not, in some cases.
Breaking changes
SnapControlleraction types were renamed fromDoSomethingtoSnapControllerDoSomethingAction.GetSnapis nowSnapControllerGetSnapAction.getSnapinstead ofget.HandleSnapRequestis nowSnapControllerHandleRequestAction.GetSnapStateis nowSnapControllerGetSnapStateAction.HasSnapis nowSnapControllerHasSnapAction.hasSnapinstead ofhas.UpdateSnapStateis nowSnapControllerUpdateSnapStateAction.ClearSnapStateis nowSnapControllerClearSnapStateAction.UpdateRegistryis nowSnapControllerUpdateRegistryAction.EnableSnapis nowSnapControllerEnableSnapAction.enableSnapinstead ofenable.DisableSnapis nowSnapControllerDisableSnapAction.disableSnapinstead ofdisable.RemoveSnapis nowSnapControllerRemoveSnapAction.removeSnapinstead ofremove.GetPermittedSnapsis nowSnapControllerGetPermittedSnapsAction.getPermittedSnapsinstead ofgetPermitted.GetAllSnapsis nowSnapControllerGetAllSnapsAction.getAllSnapsinstead ofgetAll.GetRunnableSnapsis nowSnapControllerGetRunnableSnapsAction.StopAllSnapsis nowSnapControllerStopAllSnapsAction.IncrementActiveReferencesis nowSnapControllerIncrementActiveReferencesAction.DecrementActiveReferencesis nowSnapControllerDecrementActiveReferencesAction.InstallSnapsis nowSnapControllerInstallSnapsAction.installSnapsinstead ofinstall.DisconnectOriginis nowSnapControllerRemoveSnapFromSubjectAction.RevokeDynamicPermissionsis nowSnapControllerRevokeDynamicSnapPermissionsAction.GetSnapFileis nowSnapControllerGetSnapFileAction.IsMinimumPlatformVersionis nowSnapControllerIsMinimumPlatformVersionAction.SetClientActiveis nowSnapControllerSetClientActiveAction.SnapControllerevent types were renamed fromOnSomethingtoSnapControllerOnSomethingEvent.SnapStateChangewas removed in favour ofSnapControllerStateChangeEvent.SnapBlockedis nowSnapControllerSnapBlockedEvent.SnapInstallStartedis nowSnapControllerSnapInstallStartedEvent.SnapInstallFailedis nowSnapControllerSnapInstallFailedEvent.SnapInstalledis nowSnapControllerSnapInstalledEvent.SnapUninstalledis nowSnapControllerSnapUninstalledEvent.SnapUnblockedis now `SnapControllerSnapUnblockedEvent.SnapUpdatedis nowSnapControllerSnapUpdatedEvent.SnapRolledbackis nowSnapControllerSnapRolledbackEvent.SnapTerminatedis nowSnapControllerSnapTerminatedEvent.SnapEnabledis nowSnapControllerSnapEnabledEvent.SnapDisabledis nowSnapControllerSnapDisabledEvent.Note
Medium Risk
Medium risk because it introduces breaking renames to
SnapControllermessenger action/event names (e.g.get→getSnap,getAll→getAllSnaps) and updates multiple controllers/tests to match; downstream integrations may fail if not migrated.Overview
Standardizes
SnapControllermessenger API naming by renaming action/event types to theSnapController...Action/SnapController...Eventpattern and updating call sites to new action names likeSnapController:getSnapandSnapController:getAllSnaps.Adds an auto-generated
SnapController-method-action-types.tsplus new workspace scripts to (re)generate and CI-check these method action types (yarn generate-method-action-types), and documents the breaking renames in the@metamask/snaps-controllerschangelog.Includes small related cleanups/fixes: updates metadata tests to use
includeInDebugSnapshot, renames a Multichain router test helper (getRootMultichainRouterMessenger→getMultichainRouterRootMessenger), and removes now-unneeded ESLint disables aroundterminateJobimplementations.Written by Cursor Bugbot for commit 4d407ac. This will update automatically on new commits. Configure here.