chore: Expose Accounts-owned controller/service methods through messenger#7976
chore: Expose Accounts-owned controller/service methods through messenger#7976GuillaumeRx wants to merge 3 commits intomainfrom
Conversation
GuillaumeRx
force-pushed
the
gr/expose-accounts-methods-messenger
branch
from
7de355d to
0e42ff2
Compare
| controller.setAccountGroupName(groupId, 'Suffix Test', true); | ||
|
|
||
| // Test with no conflicts: should return "Unique Name (2)" | ||
| const uniqueName = controller.resolveNameConflict( | ||
| wallet, | ||
| groupId, | ||
| 'Unique Name', | ||
| ); | ||
| expect(uniqueName).toBe('Unique Name (2)'); | ||
| const collidingGroupObject = controller.getAccountGroupObject(groupId); | ||
|
|
||
| expect(collidingGroupObject?.metadata.name).toBe('Suffix Test (3)'); | ||
|
|
||
| // Test with no conflicts: should return "Unique Name" | ||
| controller.setAccountGroupName(groupId, 'Unique Name', true); | ||
|
|
||
| const uniqueGroupObject = controller.getAccountGroupObject(groupId); | ||
|
|
||
| expect(uniqueGroupObject?.metadata.name).toBe('Unique Name'); |
There was a problem hiding this comment.
resolveNameConflict is only internally used. It doesn't make sense to have it publicly only to test its behavior in unit tests. We can do that via the public methods that uses it
| controller.setAccountGroupName(groupId, 'Suffix Test', true); | ||
|
|
||
| // Test with no conflicts: should return "Unique Name (2)" | ||
| const uniqueName = controller.resolveNameConflict( | ||
| wallet, | ||
| groupId, | ||
| 'Unique Name', | ||
| ); | ||
| expect(uniqueName).toBe('Unique Name (2)'); | ||
| const collidingGroupObject = controller.getAccountGroupObject(groupId); | ||
|
|
||
| expect(collidingGroupObject?.metadata.name).toBe('Suffix Test (3)'); | ||
|
|
||
| // Test with no conflicts: should return "Unique Name" | ||
| controller.setAccountGroupName(groupId, 'Unique Name', true); | ||
|
|
||
| const uniqueGroupObject = controller.getAccountGroupObject(groupId); | ||
|
|
||
| expect(uniqueGroupObject?.metadata.name).toBe('Unique Name'); |
There was a problem hiding this comment.
This assertion was very misleading by the way. If there's no conflict the method shouldn't be called therefore no suffix should be added.
| * @throws An error if the account ID is not found. | ||
| */ | ||
| getAccountExpect(accountId: string): InternalAccount { | ||
| #getAccountExpect(accountId: string): InternalAccount { |
There was a problem hiding this comment.
We don't need this method to be public, it's only used internally.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ActionsObj['performBatchDeleteStorage']; | ||
| export type UserStorageControllerGetStorageKey = ActionsObj['getStorageKey']; | ||
| export type UserStoragePerformDeleteStorageAllFeatureEntries = | ||
| ActionsObj['performDeleteStorageAllFeatureEntries']; |
There was a problem hiding this comment.
Inconsistent type name missing "Controller" prefix
Low Severity
The newly exported type UserStoragePerformDeleteStorageAllFeatureEntries is missing "Controller" in its name. Every other exported type alias in this file follows the UserStorageController* naming convention (e.g., UserStorageControllerPerformDeleteStorage, UserStorageControllerListEntropySources). This inconsistency makes the type harder to discover for consumers who follow the established naming pattern.
There was a problem hiding this comment.
This is true. These types are incorrectly named. But fixing this is out of scope for this PR.
| ActionsObj['performBatchDeleteStorage']; | ||
| export type UserStorageControllerGetStorageKey = ActionsObj['getStorageKey']; | ||
| export type UserStoragePerformDeleteStorageAllFeatureEntries = | ||
| ActionsObj['performDeleteStorageAllFeatureEntries']; |
There was a problem hiding this comment.
This wasn't exposed before, why's it added?
There was a problem hiding this comment.
The idea is to have everything called externally being called via the messenger. This is a preliminary step to ensure that we expose all public methods through the messenger.
| handler: AccountTreeController['setAccountGroupPinned']; | ||
| }; | ||
|
|
||
| export type AccountTreeControllerGetAccountWalletObjectAction = { |
There was a problem hiding this comment.
What are your thoughts on using yarn generate-action-method-types to automatically generate these types? Essentially what this does is generate a file (in this case called AccountsController-method-action-types.ts) which will hold the types for all actions you've added to MESSENGER_EXPOSED_METHODS. The file exports the types it generates as well as a union called AccountsControllerMethodActionTypes.
Using this script has the advantage that if the methods are JSDoc'd, then the doc blocks will be copied over to the types. You can see how we typically plug in the *MethodActionTypes type here:
There was a problem hiding this comment.
I wasn't aware of yarn generate-action-method-types and it seems relevant to use this tool here.
| | 'getUserProfileLineage' | ||
| | 'isSignedIn' | ||
| >; | ||
| type ActionsObj = CreateActionsObj<(typeof MESSENGER_EXPOSED_METHODS)[number]>; |
There was a problem hiding this comment.
Similar for this. The generate-method-action-types package script will give us an AuthenticationControllerMethodActions type that replaces this one, and we should be able to use it within the Actions type.
| | 'performBatchDeleteStorage' | ||
| | 'getStorageKey' | ||
| >; | ||
| type ActionsObj = CreateActionsObj<(typeof MESSENGER_EXPOSED_METHODS)[number]>; |
3 participants
Explanation
Exposes all the public methods other than
initor any internally used methods of the accounts-owned controllers/services throught the messenger.References
Checklist
Note
Medium Risk
Changes the cross-controller messaging surface and registration mechanism; misconfiguration of the exposed-method allowlists could break runtime message calls despite minimal business-logic changes.
Overview
Standardizes messenger wiring across accounts-owned packages by replacing per-action
registerActionHandlerblocks withregisterMethodActionHandlers+ centralizedMESSENGER_EXPOSED_METHODSallowlists inAccountTreeController,AccountsController,MultichainAccountService,AuthenticationController, andUserStorageController.Expands the messenger-exposed surface area for account tree operations (e.g., getters for wallet/group objects,
clearState, and user-storage sync entrypoints) and forAccountsControlleraddsloadBackupto the action types; internal-only helpers likeresolveNameConflictandgetAccountExpectare made private, with tests updated accordingly (switching togetAccount()and exercising conflict handling via public APIs).Written by Cursor Bugbot for commit 0e42ff2. This will update automatically on new commits. Configure here.