BREAKING Adds messenger to token controller #1166

bergarces · 2023-04-05T07:54:18Z

BREAKING Adds messenger to token controller

Description

Adds a messenger to TokensController so that the confirmation can be triggered by sending a message to ApprovalController.

BREAKING:
- The new messenger field in the constructor introduces a breaking change.
CHANGED:
- It uses ApprovalController messages to initiate and complete the request instead of an event.

Checklist

Tests are included if applicable
Any added code is fully documented

Issue

Resolves https://github.com/MetaMask/MetaMask-planning/issues/409

mcmire · 2023-04-05T19:52:14Z

packages/assets-controllers/src/TokenBalancesController.test.ts

@@ -142,6 +142,7 @@ describe('TokenBalancesController', () => {
      onPreferencesStateChange: (listener) => preferences.subscribe(listener),
      onNetworkStateChange: (listener) =>
        messenger.subscribe('NetworkController:stateChange', listener),
+      messenger: undefined as any,


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.

packages/assets-controllers/src/TokenBalancesController.test.ts

packages/assets-controllers/src/TokensController.ts

bergarces · 2023-04-14T14:02:42Z

packages/assets-controllers/src/TokensController.ts

+
+          this._acceptApproval(suggestedAssetID);
+
+          const acceptedSuggestedAssetMeta = {


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

bergarces · 2023-04-14T14:03:32Z

packages/assets-controllers/src/TokensController.ts

+    const rejectedSuggestedAssetMeta = {
+      ...suggestedAssetMeta,
+      status: SuggestedAssetStatus.rejected,
+    };


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.

bergarces · 2023-04-14T14:32:52Z

packages/assets-controllers/src/TokensController.ts

+          id: suggestedAssetMeta.id,
+          origin: ORIGIN_METAMASK,
+          type: APPROVAL_TYPE,
+          requestData: {


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.

* BREAKING - add messenger to token controller * trigger confirmation via approval controller * unit tests

bergarces force-pushed the token-controller-messenger branch 2 times, most recently from 80cf8ad to 33e207d Compare April 5, 2023 19:09

bergarces changed the title ~~Add messenger to token controller~~ Adds messenger to token controller Apr 5, 2023

bergarces changed the title ~~Adds messenger to token controller~~ BREAKING Adds messenger to token controller Apr 5, 2023

bergarces marked this pull request as ready for review April 5, 2023 19:22

bergarces requested a review from a team as a code owner April 5, 2023 19:22

mcmire reviewed Apr 5, 2023

View reviewed changes

bergarces force-pushed the token-controller-messenger branch from 33e207d to 0c0e8bf Compare April 6, 2023 08:06

matthewwalsh0 reviewed Apr 11, 2023

View reviewed changes

packages/assets-controllers/src/TokenBalancesController.test.ts Outdated Show resolved Hide resolved

matthewwalsh0 reviewed Apr 11, 2023

View reviewed changes

packages/assets-controllers/src/TokensController.ts Outdated Show resolved Hide resolved

matthewwalsh0 reviewed Apr 11, 2023

View reviewed changes

packages/assets-controllers/src/TokensController.ts Outdated Show resolved Hide resolved

matthewwalsh0 reviewed Apr 11, 2023

View reviewed changes

packages/assets-controllers/src/TokensController.ts Outdated Show resolved Hide resolved

bergarces force-pushed the token-controller-messenger branch from 2c4f33f to 8a1d2b9 Compare April 11, 2023 13:46

matthewwalsh0 previously approved these changes Apr 12, 2023

View reviewed changes

bergarces dismissed matthewwalsh0’s stale review via bd297cc April 14, 2023 13:59

bergarces force-pushed the token-controller-messenger branch from 1effe0f to bd297cc Compare April 14, 2023 13:59

bergarces commented Apr 14, 2023

View reviewed changes

bergarces requested a review from matthewwalsh0 April 14, 2023 14:03

bergarces commented Apr 14, 2023

View reviewed changes

bergarces added 8 commits April 16, 2023 20:38

add messenger to token controller

80a408c

unit tests

0274581

used real messenger object

a2d1db9

review comments

2c202b5

use empty mocks for tests

145e712

prevent direct object update

06faed0

populate requestData for mobile

7facef0

fix unit tests

b0ff768

bergarces force-pushed the token-controller-messenger branch from 101114c to b0ff768 Compare April 16, 2023 19:39

matthewwalsh0 previously approved these changes Apr 17, 2023

View reviewed changes

vinistevam previously approved these changes Apr 18, 2023

View reviewed changes

Merge branch 'main' into token-controller-messenger

77afb2f

bergarces dismissed stale reviews from vinistevam and matthewwalsh0 via 77afb2f April 18, 2023 09:48

vinistevam approved these changes Apr 18, 2023

View reviewed changes

matthewwalsh0 approved these changes Apr 18, 2023

View reviewed changes

bergarces merged commit 1970dc1 into main Apr 18, 2023

bergarces deleted the token-controller-messenger branch April 18, 2023 11:15

Gudahtt mentioned this pull request Aug 21, 2023

refactor: Update core controllers (v47) MetaMask/metamask-mobile#6964

Merged

3 tasks

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

BREAKING Adds messenger to token controller (#1166)

cf1d642

* BREAKING - add messenger to token controller * trigger confirmation via approval controller * unit tests

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

BREAKING Adds messenger to token controller (#1166)

4204c5e

* BREAKING - add messenger to token controller * trigger confirmation via approval controller * unit tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING Adds messenger to token controller #1166

BREAKING Adds messenger to token controller #1166

bergarces commented Apr 5, 2023 •

edited

Loading

mcmire Apr 5, 2023

bergarces Apr 6, 2023

matthewwalsh0 Apr 11, 2023

bergarces Apr 11, 2023

mcmire Apr 11, 2023

matthewwalsh0 Apr 12, 2023 •

edited

Loading

Gudahtt Apr 20, 2023

bergarces Apr 14, 2023

bergarces Apr 14, 2023

bergarces Apr 14, 2023


		this._acceptApproval(suggestedAssetID);

		const acceptedSuggestedAssetMeta = {

BREAKING Adds messenger to token controller #1166

BREAKING Adds messenger to token controller #1166

Conversation

bergarces commented Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewwalsh0 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergarces commented Apr 5, 2023 •

edited

Loading

matthewwalsh0 Apr 12, 2023 •

edited

Loading