feat: added rewards controller and rewards data service messenger #6493

satyajeetkolhapure · 2025-09-08T13:02:55Z

Explanation

This pull request adds Rewards Controller and messenger feature, which is used for silent login in the MetaMask Mobile app. The implementation enables automatic authentication for users when they switch accounts or unlock their wallet.

The RewardsController should:

Automatically authenticates a user through a silent signature from KeyringController if their EVM address owns a rewards profile
Skips auth for Solana and hardware wallets for now
Skips auth for 10 minutes of grace period

Note: This controller was part of metamask-mobile and moved to core as it can be used for metamask-extension too.

References

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

GuillaumeRx

In addition to the requested changes:

please review the spacing inside the functions, it's a bit out of place
There's a significant amount of logging, especially in catch blocks. I wonder if we can reduce the logging and throw regular errors instead.

packages/rewards-controller/src/RewardsController.ts

GuillaumeRx · 2025-09-10T15:53:47Z

packages/rewards-controller/src/RewardsController.ts

+    try {
+      // Generate timestamp and sign the message
+      const timestamp = Math.floor(Date.now() / 1000);
+
+      let signature;
+      try {
+        signature = await this.#signRewardsMessage(internalAccount, timestamp);
+      } catch (signError) {
+        log('RewardsController: Failed to generate signature:', signError);
+
+        // Check if the error is due to locked keyring
+        if (
+          signError &&
+          typeof signError === 'object' &&
+          'message' in signError
+        ) {
+          const errorMessage = (signError as Error).message;
+          if (errorMessage.includes('controller is locked')) {
+            log('RewardsController: Keyring is locked, skipping silent auth');
+            return; // Exit silently when keyring is locked
+          }
+        }
+
+        throw signError;
+      }


Can we avoid the nested try...catch blocks ?

packages/rewards-controller/src/RewardsController.ts

packages/rewards-controller/src/messenger/RewardsControllerMessenger.ts

packages/rewards-controller/src/services/rewards-data-service.ts

GuillaumeRx · 2025-09-10T16:08:48Z

packages/rewards-controller/src/services/rewards-data-service.ts

+
+  constructor({
+    messenger,
+    fetch: fetchFunction,


Why do we have to pass fetch in the constructor ?

Actually the fetch is just a function for handing the fetch process and url is formed in a function makeRequest.
As in rewards case the fetch handling is similar for all calls we are sending a fetch implementation in the constructor.

There's no need to pass fetch as it's already present as a global in the versions of node.js we use. I don't fully get your comment, it doesn't answer my question.

GuillaumeRx · 2025-09-10T16:28:06Z

My review was an initial pass on this PR, do not expect it to be a thorough review as this PR is pretty big.

In the future please try to keep the PR size down. This could have been split up in multiple PRs.

Co-authored-by: Guillaume Roux <guillaumeroux123@gmail.com>

GuillaumeRx

In addition, there's still a very significant amount of logging in errors

GuillaumeRx · 2025-09-11T13:16:53Z

packages/rewards-controller/src/RewardsController.ts

+    isHardwareAccount: boolean,
+  ): boolean {
+    // Skip for hardware and Solana accounts
+    if (isHardwareAccount || isSolanaAddress(address)) {


There's no need to bring a package (@solana/addresses) to validate an address with isSolanaAddress as the CAIP-10 ID contains the chain ID that could be use to determine if it's a solana account.

GuillaumeRx · 2025-09-11T13:32:46Z

packages/rewards-controller/src/feature-flags.ts

You shouldn't be completely mocking the messenger but rather creating a real restricted messenger with mocked actions inside it

GuillaumeRx · 2025-09-11T13:33:58Z

packages/rewards-controller/src/RewardsController.test.ts

+    mockMessenger = {
+      subscribe: jest.fn(),
+      call: jest.fn(),
+      registerActionHandler: jest.fn(),
+      unregisterActionHandler: jest.fn(),
+      publish: jest.fn(),
+      clearEventSubscriptions: jest.fn(),
+      registerInitialEventPayload: jest.fn(),
+      unsubscribe: jest.fn(),
+    } as unknown as jest.Mocked<RewardsControllerMessenger>;


You shouldn't be mocking completely the messenger but rather creating a real messenger with mocked actions inside it. You can look at other controllers to see how it's done.

GuillaumeRx · 2025-09-11T13:35:36Z

packages/rewards-controller/src/RewardsController.test.ts

+    } as unknown as jest.Mocked<RewardsControllerMessenger>;
+
+    // Reset feature flag to enabled by default
+    mockGetRewardsFeatureFlag.mockReturnValue(true);


We should get rid of this mock and instead do a partial implementation of the feature flag action in the test messenger.

GuillaumeRx · 2025-09-11T13:37:22Z

packages/rewards-controller/src/messenger/RewardsControllerMessenger.ts

This can be deleted now that it exists in the controller file

GuillaumeRx · 2025-09-11T13:38:06Z

packages/rewards-controller/src/services/rewards-data-service.test.ts

+  __logSpy: jest.Mock;
+};
+
+let mockMessenger: jest.Mocked<RewardsDataServiceMessenger>;


We shouldn't mock the entire messenger.

GuillaumeRx · 2025-09-11T13:45:41Z

packages/rewards-controller/src/services/rewards-data-service.test.ts

+  fetchImpl = jest
+    .fn()
+    .mockResolvedValue(
+      okJsonResponse({ sessionId: 'sess', subscription: { id: 'sub' } }),
+    ),


We don't manually mock fetch, we rely on nock to provide fetch mocks.

GuillaumeRx · 2025-09-11T13:45:56Z

packages/rewards-controller/src/services/rewards-data-service.test.ts

+  mockMessenger = {
+    registerActionHandler: jest.fn(),
+    call: jest.fn(),
+  } as unknown as jest.Mocked<RewardsDataServiceMessenger>;


We don't directly mock the messenger

GuillaumeRx · 2025-09-11T13:47:16Z

packages/rewards-controller/src/services/rewards-data-service.ts

+
+  constructor({
+    messenger,
+    fetch: fetchFunction,


There's no need to pass fetch as it's already present as a global in the versions of node.js we use. I don't fully get your comment, it doesn't answer my question.

GuillaumeRx · 2025-09-11T13:48:52Z

packages/rewards-controller/src/services/rewards-data-service.ts

+
+  readonly #environment: EnvironmentType;
+
+  readonly #getSubscriptionToken: GetSubscriptionToken;


Where does this function comes from ? If it's available through a messenger action please use the messenger rather than passing it as a parameter.

Gudahtt · 2025-09-11T14:10:58Z

packages/rewards-controller/src/types.ts

+  lastPerpsDiscountRateFetched: number | null;
+};
+
+// eslint-disable-next-line @typescript-eslint/consistent-type-definitions


This comment seems to be unnecessary, there is no lint error here

Gudahtt · 2025-09-11T14:11:27Z

packages/rewards-controller/src/types.ts

+};
+
+// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
+export type RewardsControllerState = {


Could you add descriptions to these properties? It's hard to tell what they are for

satyajeetkolhapure and others added 30 commits September 8, 2025 12:46

feat: added rewards controller and rewards data service messenger

0987569

Merge branch 'main' into feat/add-rewards-controller

6428e5d

fix: lint issues

02f32cc

Merge branch 'main' into feat/add-rewards-controller

f6c0f35

fix: reverted yarn lock

9890457

fix: reverted yarn lock to the first commit

5b75ddd

fix: removed dependencies from rewards controller package

1cac54a

fix: lint errors

a85d2f9

Merge branch 'main' into feat/add-rewards-controller

ac0b219

fix: changed solana package version

0bd1b81

fix: changed versions and added yarn lock

0d6137a

fix: added codeowner and @metamask/keyring-controller package

0bcbd36

Merge branch 'main' into feat/add-rewards-controller

06db6e8

fix: corrected the codeowner for rewards controller

973b461

Merge branch 'main' into feat/add-rewards-controller

7a30821

fix: added description in changelog

2181c1e

fix: lint issues

a9e8da1

Merge branch 'main' into feat/add-rewards-controller

89e902c

PR Review: merge controller changes

fe71c1e

Merge branch 'main' into feat/add-rewards-controller

110488e

fix: improved test coverage

84368f8

Merge branch 'main' into feat/add-rewards-controller

3622ff4

fix: added references in tsconfig.build.json

ca05e0b

fix: improved test coverage

5d2ab1a

Merge branch 'main' into feat/add-rewards-controller

0f0bfb2

fix: added getPointsEvents change

dab912c

fix: made 100% test coverage

8e8c8e1

Merge branch 'main' into feat/add-rewards-controller

5e485e6

fix: lint issues

aaadd5d

fix: formatting issue with changelog

6438b46

satyajeetkolhapure and others added 6 commits September 10, 2025 15:28

Merge branch 'main' into feat/add-rewards-controller

cd6b4df

fix: formatting issue with changelog

1719e26

fix: formatting issue with changelog

a5324a6

Merge branch 'main' into feat/add-rewards-controller

bcb55ab

Merge branch 'main' into feat/add-rewards-controller

cd16b6f

fix: formatting issue with changelog

989d789

satyajeetkolhapure added the team-rewards label Sep 10, 2025

satyajeetkolhapure self-assigned this Sep 10, 2025

satyajeetkolhapure marked this pull request as ready for review September 10, 2025 14:54

satyajeetkolhapure requested a review from a team as a code owner September 10, 2025 14:54

This comment was marked as outdated.

Sign in to view

fix: cursor review comment

b830342

jbblanc previously approved these changes Sep 10, 2025

View reviewed changes

GuillaumeRx requested changes Sep 10, 2025

View reviewed changes

fix: review comments

ac826d6

satyajeetkolhapure dismissed jbblanc’s stale review via ac826d6 September 10, 2025 17:51

satyajeetkolhapure and others added 3 commits September 10, 2025 18:52

Update packages/rewards-controller/src/RewardsController.ts

4397435

Co-authored-by: Guillaume Roux <guillaumeroux123@gmail.com>

fix: review comments

229ef99

fix: review comments

9dba63b

This comment was marked as outdated.

Sign in to view

satyajeetkolhapure and others added 3 commits September 11, 2025 13:05

fix: review comments

7d3f29f

fix: review comments

af3c35e

Merge branch 'main' into feat/add-rewards-controller

b2f541f

GuillaumeRx requested changes Sep 11, 2025

View reviewed changes

Gudahtt reviewed Sep 11, 2025

View reviewed changes

satyajeetkolhapure and others added 2 commits September 11, 2025 15:32

fix: review comments

02fe388

Merge branch 'main' into feat/add-rewards-controller

f155fa3


		readonly #environment: EnvironmentType;

		readonly #getSubscriptionToken: GetSubscriptionToken;

Uh oh!

feat: added rewards controller and rewards data service messenger #6493

Are you sure you want to change the base?

feat: added rewards controller and rewards data service messenger #6493

Conversation

satyajeetkolhapure commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Checklist

Uh oh!

This comment was marked as outdated.

Uh oh!

GuillaumeRx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeRx commented Sep 10, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

GuillaumeRx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gudahtt Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

satyajeetkolhapure commented Sep 8, 2025 •

edited

Loading

Gudahtt Sep 11, 2025 •

edited

Loading