Improve sample-controllers to be more useful examples #6168

mcmire · 2025-07-22T19:17:43Z

Explanation

The controllers and services in sample-controllers are not as useful as examples as they could be. They do not follow the best practices that we want others to follow, so we cannot ask teams to copy them, and they do not follow patterns that are standard in the product.

This commit makes the following changes:

The SampleGasPricesService now makes use of the messenger system.
SampleGasPricesController and SamplePetnamesController now exposes their methods as actions, which follows the typical controller.
The AbstractSampleGasPricesService has been removed, and SampleGasPricesService is now expected to be used via the messaging system.
SampleGasPricesService now uses the data services pattern, specifically, createServicePolicy.
The tests for SampleGasPricesController and SamplePetnamesController now use the withController pattern.
The tests for SampleGasPricesController and SamplePetnamesController now set up a "unrestricted" messenger rather than a "root" messenger.
@metamask/network-controller types are now used directly instead of being copied (it was already a peer dependency).
fetchGasPrices in SampleGasPricesController is now called automatically when the globally selected chain changes, and also now takes a chain ID to match.
getDefaultGasPricesControllerState and getDefaultPetnamesControllerState are now placed in the "STATE" section.
getDefaultPetnamesControllerState is now exported.

References

Closes #6166.

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

The controllers and services in `sample-controllers` are not as useful as examples as they could be. They do not follow the best practices that we want others to follow, so we cannot ask teams to copy them, and they do not follow patterns that are standard in the product. This commit makes the following changes: - The `SampleGasPricesService` now makes use of the messenger system. - `SampleGasPricesController` and `SamplePetnamesController` now exposes their methods as actions, which is more typical of a conventional controller. - The `AbstractSampleGasPricesService` has been removed, and `SampleGasPricesService` is now expected to be used via the messaging system. - `SampleGasPricesService` now uses the data services pattern, specifically, `createServicePolicy`. - The tests for `SampleGasPricesController` and `SamplePetnamesController` now use the `withController` pattern. - The tests for `SampleGasPricesController` and `SamplePetnamesController` now set up a "unrestricted" messenger rather than a "root" messenger. - `@metamask/network-controller` types are now used directly instead of being copied (it was already a peer dependency). - `fetchGasPrices` in `SampleGasPricesController` is now called automatically when the globally selected chain changes, and also now takes a chain ID to match. - `getDefaultPetnamesControllerState` is now exported.

packages/sample-controllers/src/index.test.ts

mcmire · 2025-07-22T20:58:40Z

packages/sample-controllers/src/index.ts

 export {
-  getDefaultSampleGasPricesControllerState,
  SampleGasPricesController,
+  getDefaultSampleGasPricesControllerState,


This export was already present, but it was not ordered alphabetically.

mcmire · 2025-07-22T20:59:51Z

packages/sample-controllers/src/network-controller-types.ts

This file was added originally before sample-controllers was published to NPM as it wasn't a requirement that SampleGasPricesController be a working example, just illustrative. Since then, network-controller has been added as a peer dependency, so we can simply use the types from there.

packages/sample-controllers/src/sample-gas-prices-controller.ts

mcmire · 2025-07-22T21:02:15Z

packages/sample-controllers/src/sample-gas-prices-controller.ts

- *
- * @returns The default {@link SampleGasPricesController} state.
- */
-export function getDefaultSampleGasPricesControllerState(): SampleGasPricesControllerState {


Not sure why this was added to the "MESSENGER" section originally (probably just an oversight). Regardless this now lives under "STATE".

packages/sample-controllers/src/sample-gas-prices-controller.ts

packages/sample-controllers/src/sample-gas-prices-service/sample-abstract-gas-prices-service.ts

packages/sample-controllers/src/sample-gas-prices-service/sample-gas-prices-service.ts

Gudahtt · 2025-07-28T18:21:19Z

packages/sample-controllers/src/sample-gas-prices-service/sample-gas-prices-service.ts

+        `https://example.com/gas-prices/${chainId}.json`,
+      );
+      if (response.ok) {
+        // Type assertion: We have to assume the shape of the response data.


Nit (pre-existing issue): We can validate, we definitely don't need to assume the shape. Assuming the shape of the response without validation should be strongly discouraged. We've had plenty of examples even with internal APIs of them not returning the expected shape (especially with error responses, which sometimes can be returned even by HTTP 200 responses)

You're right, but we haven't been validating responses elsewhere as far as I know. What is the pattern that we want to promote here, should we use Superstruct to do so or do it manually? And what should happen if the validation fails, should we throw an error?

We don't have any prescriptive guidance right now on how to best validate, so that would be up to the team or contributor.

Either throwing an error for invalid inputs, or stripping the data down to the valid parts/normalizing it, may be appropriate depending on the circumstance. Personally I'd default to throwing an error if I wasn't sure how to handle something.

Okay, I've added some basic validation here: 8f9a6b9

packages/sample-controllers/src/sample-gas-prices-service/sample-gas-prices-service.ts

packages/sample-controllers/package.json

Gudahtt · 2025-07-28T18:52:22Z

packages/sample-controllers/src/sample-petnames-controller.test.ts

  });

-  describe('assignPetname', () => {
+  describe.each([


This seems a bit convoluted 🤔 Is it helpful to test the method in both ways? If we test just the method, certainly the tests would miss a problem with the action registration. But if we have tests for the action, it's unclear what value the tests for the method have. Especially if we decide to disallow calling methods directly at some point in the future.

I have noticed that it is easy when adding an action which directly maps to a method to forget to test the action. (I notice, in fact, that I forgot to do this for SampleGasPricesService.) This approach seemed like the best way to ensure that both the method and the action do exactly the same thing.

But if you think this is convoluted, and you think that at some point we will disallow calling methods at some point in the future, perhaps we should promote a pattern where the action is tested comprehensively and the method is tested more simply? (We could of course just test the action, but that seems inconsistent, as there are some cases where actions don't map to methods, and the methods are still technically callable, at least for now.)

I have updated the tests to primarily test the messenger actions here. Let me know if that fits more with what you were thinking or if you had something else in mind: b3f9796

It just seems like a lot for us to ask teams to test both redundantly, when there isn't really a reason for both to exist, you know. The way we wire up controllers, in theory nothing should be able to call any method directly anyway.

Certainly today there are methods that are not actions, but if this is an example of what we're recommending as best practices for new controllers, we need not let those existing inconsistencies restrict our choices here.

We haven't really agreed on preventing direct method calls though, that's just something I had in mind to propose in the future to simplify things. Maybe until we agree on that we should test both? I just didn't feel great about it, you know.

Yeah, I know what you mean. We could certainly ask people to stop testing methods directly that are being exposed via the messenger. I worry that this guidance would be out of place at the moment, though. As it is, we do use methods directly, both in the background API and in Mobile. So at the moment, it is useful, even if it's not ideal. But I see your point. I'll think about this some more.

I've updated the JSDocs to encourage people to use the controllers and service through the messenger, but left the method and its test in place for now since they have practical use: 0abf441. The tests for the methods are extremely simple on purpose, so it shouldn't be a huge burden if people were to copy those examples.

Once we have a plan for simplifying the background API as well as guidance for using the messenger from the UI, we should be able to completely drop methods along with their tests and promote the messenger as the one true way to use controllers and services.

Gudahtt

Mostly looks good! There is one significant problem with the service policy that must be fixed before merge, but most of my other comments are pre-existing issues or non-blocking discussions.

…graded

mcmire · 2025-08-21T22:07:58Z

packages/sample-controllers/src/index.ts

+  SampleGasPricesServiceEvents,
+  SampleGasPricesServiceMessenger,
+} from './sample-gas-prices-service/sample-gas-prices-service';
+export type { SampleGasPricesServiceFetchGasPricesAction } from './sample-gas-prices-service/sample-gas-prices-service-method-action-types';


These import paths are a bit annoying, now that I look at it. I'm curious whether flattening them is a good idea. Maybe I'll make another PR that organizes these files better.

packages/sample-controllers/src/sample-gas-prices-service/sample-gas-prices-service.test.ts

packages/sample-controllers/src/sample-gas-prices-controller.test.ts

packages/sample-controllers/src/sample-petnames-controller.test.ts

mcmire · 2025-08-21T22:12:28Z

The changes in #6335 will probably cause conflicts here, so I will wait until that's merged and then revisit this PR.

mcmire · 2025-08-22T21:49:27Z

...e-controllers/src/sample-gas-prices-service/sample-gas-prices-service-method-action-types.ts

@@ -0,0 +1,24 @@
+/**
+ * This file is auto generated by `scripts/generate-method-action-types.ts`.


One thing I noticed is that method action types for services are not generated automatically. I had to add this one myself. I will make a ticket to fix the script.

I've made a ticket for this here: #6407

mcmire · 2025-08-25T13:57:09Z

Ready for review again.

Gudahtt · 2025-08-27T19:47:32Z

packages/sample-controllers/src/sample-gas-prices-service/sample-gas-prices-service.test.ts

+      });
+    });
+
+    it.each([


Nice. This test is satisfyingly compact. Great to see response validation in the example service as well.

Gudahtt

LGTM!

mcmire · 2025-08-27T20:09:51Z

packages/sample-controllers/src/sample-gas-prices-service/sample-gas-prices-service.test.ts

+ *
+ * @returns The root messenger.
+ */
+function buildRootMessenger(): RootMessenger {


Dang, I just noticed that this should be getRootMessenger to align with controller tests. I will change this tomorrow.

This is now fixed: 7a97b62

cryptodev-2s

LGTM!

mcmire force-pushed the update-sample-controllers branch from 05b65fe to deac397 Compare July 22, 2025 19:27

mcmire marked this pull request as ready for review July 22, 2025 20:52

mcmire requested a review from a team as a code owner July 22, 2025 20:52

Merge branch 'main' into update-sample-controllers

3d76ba7

mcmire added the team-wallet-framework Deprecated: Please use `team-core-platform` instead. label Jul 22, 2025

This comment was marked as outdated.

Sign in to view

mcmire commented Jul 22, 2025

View reviewed changes

mcmire added 5 commits July 22, 2025 15:20

Fix typo

0962c3e

Fix error messages

e1dea00

Fix race condition

c0b6e96

Fix test

bb71b2d

Fix race condition for real

5e343da