feat: Add messenger delegate and revoke methods

[Gudahtt]

Explanation

The methods delegate and revoke have been added to the Messenger class. These methods replace the need for the RestrictedMessenger. The getRestricted method, and the RestrictedMessenger class, have been removed as obsolete.

Note that the parent constructor parameter described in the ADR is not implemented yet, that will come in the next PR.

References

See this ADR PR for details: MetaMask/decisions#53
Relates to #5626

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
• I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

[mcmire]

I had some comments, mostly around documentation, but I was also curious about some behavior in delegate which seems to be replicated in #registerInitialEventPayload.

[mcmire]

This seems to allow any kind of action handler, but if ActionType must refer to an action that this messenger recognizes, then shouldn't the handler be connected to that action?

Suggested change

	handler:
	| ActionHandler<Action, ActionType>
	| ActionHandler<ActionConstraint, ActionType>,
	handler: ActionHandler<Action, ActionType>,

[Gudahtt]

I had to widen the type because of contravarient types. Essentially, TypeScript was complaining in some situations because the messenger we were delegating to had additional actions registered that the delegating messenger's type was not aware of. This doesn't present a problem for us in practice, but it did cause a type mismatch.

I added a comment to explain this.

Also I experimented with adding type tests in a separate branch: https://github.com/MetaMask/core/tree/demonstrate-type-error

Notice the two commits: first I narrowed this type, and wrote type tests to demonstrate the error. Then the next commit (the last one) makes the minimal changes necessary to fix the errors. Try experimenting with it locally, every single change in that last commit was necessary.

[mcmire]

Interesting! Thanks for making that branch and writing up those tests, that helped a lot. (tstyche is pretty cool, too.) I am not fully convinced that this is the best solution long-term, but it does make sense, and in the end I don't think it impacts type-safety.

(On a long-term solution: I am curious whether the fact that we divorce the type of an action from its handler throughout the entire Messenger class may be the real obstacle here. I feel like if we received and passed the Action or Event type as one unit then it could work better and TypeScript would be able to understand that we are comparing a subset of all actions and types that are involved. But I haven't tested that out yet, so we might end up in the same place.)

(On type-safety: One drawback from widening these types is that now you won't get a type error if you try to delegate an action or event to a messenger that doesn't actually support it. It's not a big deal because if you tried to use the delegated messenger to call the action or subscribe to the event later it wouldn't work anyway, but it's something to keep in mind.)

[Gudahtt]

One drawback from widening these types is that now you won't get a type error if you try to delegate an action or event to a messenger that doesn't actually support it

Hmm. Good point. This does mean delegate isn't really type-safe. I will revisit this. If we can't get type safety here, this could at least be simplified.

I am curious whether the fact that we divorce the type of an action from its handler throughout the entire Messenger class may be the real obstacle here. I feel like if we received and passed the Action or Event type as one unit then it could work better and TypeScript would be able to understand that we are comparing a subset of all actions and types that are involved

Could you elaborate on what you had in mind here? I'm not sure I follow what you mean when you talk about passing them as "one unit". Are you talking about how we're using the type union, or how we're referencing individual properties of an action or event?

The problem as I see it is that "comparing a subset" fundamentally doesn't work with contravarient types. But maybe I'm missing something.

[mcmire]

The type for registerActionHandler, publish, subscribe, call, _internalRegisterDelegatedActionHandler, etc. take just the name of an action or event and then derive an action/event handler type from it if necessary. I was suggesting that if it is possible for these methods to take the entire action or event object type then perhaps TypeScript could compare the subset of actions and events more easily between the DelegatedMessenger type and the Messenger type. But perhaps you are right; at the end of the day it would probably still have to compare function types, so perhaps it wouldn't matter.

[Gudahtt]

I managed to find a way to type check the delegate and revoke parameters using a conditional type. The DelegatedMesseger type is still broadened for internal references past that point, but at least this type safety issue should be resolved. I've pushed updated tests to this branch as well: https://github.com/MetaMask/core/tree/demonstrate-type-error

[Gudahtt]

I had to refactor it again. The old branch I used with the type tests wasn't easy to rebase onto this implementation because the way I produced an error was no longer applicable, so I squashed the tests together and moved them to this new branch to demonstrate that the types still work:

https://github.com/MetaMask/core/tree/add-messenger-type-tests

[mcmire]

Out of curiosity, did you plan on merging these type tests? Or were they just for exploring a path forward? The tstyche package you found seems very useful at least.

[Gudahtt]

I wanted to test whether vite was a better alternative or not first. And an obstacle to that would be figuring out whether vite itself is a suitable replacement for Jest or not in this repo.

I guess we could add tstyche directly in the short term at least, maybe I'll propose that instead

[mcmire]

I had one more comment, but it is non-blocking.

LGTM!

[cursor]

Bug: Concurrent Modification Causes Unpredictable Behavior

The clearActions and clearEventSubscriptions methods modify their respective internal Maps (#actions and #events) while iterating over them, leading to unpredictable behavior where entries can be skipped.

Specifically:

• clearActions: Some action handlers and their associated delegation targets may not be properly unregistered.
• clearEventSubscriptions: Some non-delegated event subscriptions may not be cleared. Additionally, for these uncleared subscriptions with selectors, their cached values in #eventPayloadCache are not removed, leading to a memory leak.

Additional Locations (1)

• packages/messenger/src/Messenger.ts#L338-L343