feat!: Expose Accounts-owned controller/service methods through messenger#7976
feat!: Expose Accounts-owned controller/service methods through messenger#7976GuillaumeRx wants to merge 25 commits intomainfrom
Conversation
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
There was a problem hiding this comment.
I can confirm this is not used in either extension and mobile.
| 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.
packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts
Outdated
Show resolved
Hide resolved
packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts
Outdated
Show resolved
Hide resolved
packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts
Outdated
Show resolved
Hide resolved
packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts
Outdated
Show resolved
Hide resolved
0e42ff2 to
6ea2c28
Compare
There was a problem hiding this comment.
I don't know what's going on here with the imports, I just made sure that the original export behavior was respected.
There was a problem hiding this comment.
This certainly seems cleaner :)
There was a problem hiding this comment.
I don't know what's going on here with the imports, I just made sure that the original export behavior was respected.
be1d092 to
405c239
Compare
| const { messenger, mocks } = await setup({ | ||
| accounts: [MOCK_HD_ACCOUNT_1], | ||
| }); | ||
|
|
||
| const resyncAccountsSpy = jest.spyOn(service, 'resyncAccounts'); | ||
| await messenger.call('MultichainAccountService:resyncAccounts'); | ||
| expect(resyncAccountsSpy).toHaveBeenCalled(); | ||
|
|
||
| expect(mocks.EvmAccountProvider.resyncAccounts).toHaveBeenCalled(); | ||
| expect(mocks.SolAccountProvider.resyncAccounts).toHaveBeenCalled(); |
There was a problem hiding this comment.
we can't spy resyncAccounts at this step since registeMethodActionHandlers will register the original method not the spied one
| const ensureCanUseSnapPlatformSpy = jest.spyOn( | ||
| service, | ||
| 'ensureCanUseSnapPlatform', | ||
| await rootMessenger.call( | ||
| 'MultichainAccountService:ensureCanUseSnapPlatform', | ||
| ); | ||
| await messenger.call('MultichainAccountService:ensureCanUseSnapPlatform'); | ||
| expect(ensureCanUseSnapPlatformSpy).toHaveBeenCalled(); |
There was a problem hiding this comment.
we can't spy ensureCanUseSnapPlatform at this step since registeMethodActionHandlers will register the original method not the spied one
31e2baa to
6867941
Compare
packages/multichain-account-service/src/MultichainAccountService.test.ts
Show resolved
Hide resolved
mcmire
left a comment
There was a problem hiding this comment.
Nice! The code changes themselves look good, but I had some suggestions on changelogs. (You may need to add the no-changelog label to this PR to suppress the changelog checker.)
| 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.
I can confirm this is not used in either extension and mobile.
| * @throws An error if the account ID is not found. | ||
| */ | ||
| getAccountExpect(accountId: string): InternalAccount { | ||
| #getAccountExpect(accountId: string): InternalAccount { |
5f0e503 to
2aa9997
Compare
…StorageController`
- Update CHANGELOGs - Remove non public-facing changes - Make entries clearer - Remove export of `*MethodActions`
170f273 to
92a2528
Compare
Explanation
Exposes all the public methods other than
initor any internally used methods of the accounts-owned controllers/services throught the messenger.This also refactors how actions are defined and bound to the messenger using
registerMethodActionHandlersandMESSENGER_EXPOSED_METHODS.The
generate-method-action-typescript was used to generate the action types.This is a breaking change in the
profile-sync-controllerpackage as some of the action type names changed but none of the underlying signatures changed.References
Checklist
Note
Medium Risk
Medium risk because it changes messenger exposure and public TypeScript action types across multiple packages, which can break downstream compilation or runtime wiring if action lists/types drift, though underlying method implementations are largely unchanged.
Overview
Exposes previously unexposed public methods via messenger for
AccountsController,AccountTreeController,MultichainAccountService,AuthenticationController, andUserStorageControllerby standardizing onMESSENGER_EXPOSED_METHODS+messenger.registerMethodActionHandlersand adding auto-generated*-method-action-types.tsunions/types exported from packageindex.ts.Breaking API surface cleanup/standardization: internal-only helpers are made private (e.g.
AccountsController#getAccountExpect,AccountTreeController#resolveNameConflict), andprofile-sync-controlleraction type names are standardized to end inAction, requiring downstream TypeScript import updates (action strings/signatures unchanged). Tests and downstreamAllowedActionstypings are updated accordingly, and packages add agenerate-method-action-typesscript/dependency (tsx) plus changelog entries documenting the new actions and removals.Written by Cursor Bugbot for commit 92a2528. This will update automatically on new commits. Configure here.