Any chance we could provide a real messenger object instead of nothing? We'd like to avoid using any across the monorepo if we can, even in tests.

I've created real messenger objects for those tests to avoid use of any.

Having said that, I don't think it's going to be scalable if we have to instantiate real dependencies instead of mocking them. If we are instantiating real TokensController with real messengers for other controllers, these are no longer unit tests.

Is there a compromise where we can still provide a mock for these arguments to reduce coupling, but cast to the correct type rather than any, so a as unknown as TokensControllerMessenger?

I've pushed another commit with the as unknown as TokensControllerMessenger. The messenger is not even used during those tests, so it's set to undefined.

We should not have to use real object instantiations on unit tests when that is not the subject of the test.

I see what you mean, but I'm not so sure that using a real messenger would make this an integration test. Unless we're using it to talk to another controller — that would be unnecessary. But if we're just using it to satisfy the argument, then I don't see why we can't use a real one.

I'd argue it's just a matter of future-proofing and minimising the code being executed in the test for the sake of avoiding fragility and ensuring we're testing a isolated unit whenever possible, rather than dependency code, in this case the ControllerMessenger.

As an example, if we were to change the code in future to invoke a method in the TokensController from the TokenBalancesController that did use the messenger, any related errors in the messenger code would needlessly cause the TokenBalancesController tests to fail and they would be harder to diagnose as the errors would be originating from code the test wasn't intending to validate.

There's also a performance argument with this pattern if we're consistently loading additional dependencies and code that isn't functionally required for a test.

This change seems fine to me as a short-term fix because the tests shouldn't have a TokenController in the fist place, but fixing that would be out-of-scope of this PR.

But in general I would recommend using a real controller messenger in tests, if only because it's such a simple construct that mocking it would add complexity. The messenger is meant as a way of doing dependency injection, it's a way for us to pass mocks into the code under test.

It's the use of additional controllers that aren't under test that makes this example needlessly complex. Not the messenger. In your example here @matthewwalsh0 you seem to be talking about one such case. I don't think @mcmire meant to suggest that using real controllers that aren't under-test is a good idea.

I had to remove the line that updates status directly (suggestedAssetMeta.status = SuggestedAssetStatus.accepted;) and replace it by a new object using the spread operator, as otherwise that line throws a read error when using this controller in mobile codebase with react-native.

Attempted to assign to readonly property

Same as with the previous one, I had to create a new object instead of updating status directly to prevent an error in react-native codebase.

For mobile, we will need requestData to be populated with the following values, as it currently relies on the suggestedAssetMeta object sent by the pendingSuggestedAsset event.

Unfortunately, the type SuggestedAssetMeta is not compatible with requestData as it has several optional and an unknown field within the Token type, so we are sending precisely the data it needs.