MetaMask / MetaMask/metamask-mobile

SamplePetnamesController template lags the controller-integration pattern it is meant to teach

Open
#32,729 1 comment 0 reactions 0 assignees View on GitHub
INVALID-ISSUE-TEMPLATE Sev3 ta-needs-engineer-escalation ta-triaged team-mobile-platform type-tech-debt
Dominant language
TypeScript
Stars
3k
Forks
1.7k
Avg merge
1d 14h
Merged PRs (30d)
669

Description

## What is this about?

`SamplePetnamesController` (`app/features/SampleFeature/controllers/`) is the reference template every new controller integration copies (see `app/core/Engine/README.md` / the controller-integration checklist). Today it drifts from the current best-practice pattern in 4 verified ways, so it teaches an outdated pattern to anyone using it as a starting point.

## Technical Details

1. **No co-located tests** — the controllers folder contains only `sample-petnames-controller-init.ts` and `sample-petnames-controller-messenger.ts`. Every real controller ships both an init test and a messenger test.
- Add `sample-petnames-controller-init.test.ts` (using `buildMessengerClientInitRequestMock`, mirroring `app/core/Engine/controllers/config-registry-controller-init.test.ts`).
- Add `sample-petnames-controller-messenger.test.ts` (smoke test, e.g. `toBeInstanceOf(Messenger)`).

2. **Selector fallback drift** — `selectSamplePetnamesControllerState` (`app/features/SampleFeature/selectors/samplePetNameController/index.ts`) returns `undefined` instead of the package default state.
- Fall back to `getDefaultPetnamesControllerState()` (exact export name from `@metamask/sample-controllers`), matching the convention used by `ConfigRegistryController` (`getDefaultConfigRegistryControllerState()`).
- Ripple effects: update `selectors/samplePetNameController/index.test.ts` (its "returns undefined when controller is not initialized" cases) and drop the `| undefined` from downstream selector signatures.

3. **Older messenger factory typing** — `getSamplePetnamesControllerMessenger` takes an untyped `RootExtendedMessenger` parameter.
- Narrow it as `getMoneyAccountUpgradeControllerMessenger` does: `RootMessenger, MessengerEvents>`, so delegated names are type-checked exactly.

4. **Stale template docs** — `app/features/SampleFeature/README.md` directs developers to place controllers in `/app/core/controllers/` (this path does not exist; the actual location is `app/core/Engine/controllers/`) and links `../../docs/architecture.md` (this file does not exist; the actual file is `docs/readme/architecture.md`).
- Correct both paths.

## Acceptance Criteria

- Items 1-4 above are implemented.
- `yarn lint:tsc` passes.
- New and updated tests pass (`yarn jest app/features/SampleFeature`).

## References

- Optional follow-up, out of scope for this issue: the template never demonstrates delegation, init messengers, or services, since the package messenger has `AllowedActions = never` / `AllowedEvents = never`. Integrating the sibling `SampleGasPricesController` + `SampleGasPricesService` from `@metamask/sample-controllers` would cover delegation, services, and the stateless-service delta, but it is net-new integration work (types.ts/Engine.ts/constants.ts/fixture), so it belongs in its own issue/PR.

Contributor guide

Open the contributing guide

Research direction

Start with the SampleFeature controller files, selectors/samplePetNameController/index.ts and its test, and app/features/SampleFeature/README.md; compare their patterns with ConfigRegistryController and the referenced messenger factory. Run yarn jest app/features/SampleFeature and yarn lint:tsc. Done means all four template gaps are addressed and the listed checks pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
developer-experience, documentation, testing-qa
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.