Use messenger actions instead of KeyringController as constructor param

[mikesposito]

Explanation

The SignatureController accepts an entire KeyringController instance as a constructor option, but would be better to use messenger actions instead, since this controller is a BaseControllerV2.

References

• Related to Migrate all remaining @metamask/eth-keyring-controller interactions metamask-extension#20510

Changelog

@metamask/signature-controller

• BREAKING: Remove keyringController constructor option: messenger actions are now being used

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 highlighted breaking changes using the "BREAKING" category above as appropriate

[Gudahtt]

Have you considered using actions instead, via the messaging system? As we migrate each controller to BaseControllerV2 and the controller messenger, the intent was to eventually replace all of these controller-method-constructor-parameters with actions. That way the messenger types represent a complete mapping of all controller relationships.

[mikesposito]

Actions needed in this PR have been added to KeyringController by #1654

[gantunesr]

LGTM

[cryptodev-2s]

LGTM!

[matthewwalsh0]

I'm not sure I see the value in this. The benefit of the callbacks is we are entirely decoupled from the signing itself as we can provide any logic we need and the client doesn't even have to use the KeyringController.

In the mobile client for example, we manipulate the arguments to support the core KeyringController where as the extension originally used their own KeyringController and this supported that with minimal effort.

I appreciate we're now using the core KeyringController in the extension, but this still feels like we're losing flexibility by limiting ourselves to a single "signer", plus introducing a new breaking change, but only gaining the convenience of one less constructor argument?

Perhaps the interface is poorly named, but the SignatureController doesn't depend on an entire KeyringController instance as the PR description suggests, rather only an interface with 3 simple methods. It could just as easily have been called Signer or SigningProvider for example.

[Gudahtt]

I am not sure I follow. How does this limit the controller to a single signer? You could still invoke other types of signatures within #signMessage and other methods.

For context, we have been trying to avoid passing entire controllers as constructor parameters for many years now because we found it makes it difficult to understand how the controllers depend on each other. Quite a few times we've tried to make a breaking change to a controller and missed a spot where it was used because of controller references being passed around with different names, making them easier to miss.

The messaging system itself was a solution to this problem as well; the goal being eventually that once all inter-controller communication is through this system, we could see in one single place how they relate to each other (and maybe even generate an image!), and use the type system to keep it up-to-date.

[matthewwalsh0]

Apologies for the confusion, I'm not referring to the types of signatures as in personal or typed for example, but I'm meaning the entity that performs the actual signing.

Previously, it was explicitly defined by the KeyringController interface without any coupling to the actual KeyringController itself, other than the name.

My worry is simply that by replacing it with the messenger, are we not coupling it further and saying the only thing that can sign things for the SignatureController is the core KeyringController? Or at least something that goes to the effort of defining handlers for all the various messages?

I agree with the principle that if you want to explicitly communicate with a controller, it is best to use the messenger, but is it the best use case here where the interface is simple and already explicitly defined and typed?

To reframe my concern with another example, consider all the places we need to get the chain ID. This is a simple contract where we have no arguments but need to retrieve a string or a number. We typically use a constructor callback argument so it can be abstracted away, but would you argue it would be be better to use the messenger for that to request it from the NetworkController directly? Is that not introducing more coupling and adding more complexity?

[Gudahtt]

I was interpreting the code on main as "explicitly communicating with a controller" already, I thought we had crossed that bridge already. This move to use the controller messenger was a refactor of that pre-existing pattern that aligns better with our conventions. I hadn't realized that that instance was thought of more as an abstract interface.

Though on that subject, I'm not sure we want to abstract signing from keyrings either. Keyrings are signers, that is effectively their sole purpose. Even snap keyrings are going to be exposed via the keyring controller. And the keyring controller itself acts to decouple individual keyrings from other parts of the codebase that require signatures.

Could you elaborate further on the need for another intermediary interface/abstraction? I can't think of how that might be useful.

[matthewwalsh0]

I didn't have a specific use case in mind, I'm just trying to think of each controller as decoupled as possible.

The interface existed initially to abstract the variations between the extension and core keyring controllers but as we're now using the core one exclusively, I do agree it would have minimal benefit here if the core KeyringController itself can accommodate any future variations in signing logic through the keyrings.

Thanks for the discussion Mark, all seems good after all 😄