Skip to content

Migrate SampleGasPriceService to BaseDataService #8343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mcmire wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Migrate SampleGasPriceService to BaseDataService #8343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
896f3cc
Migrate SampleGasPriceService to BaseDataService
mcmire Mar 30, 2026
52639fb
Update changelog
mcmire Mar 30, 2026
bb9a7bc
Use lowercase for the service name
mcmire Mar 30, 2026
9c341e1
Achieve coverage
mcmire Mar 30, 2026
3a60272
Merge branch 'main' into update-sample-gas-price-service
mcmire Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions packages/sample-controllers/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Add actions and events for accessing and interacting with the new query cache for `SampleGasPricesService` ([#8343](https://github.com/MetaMask/core/pull/8343))
- New actions and events are:
- `SampleGasPricesServiceInvalidateQueriesAction`
- `SampleGasPricesServiceCacheUpdatedEvent`
- `SampleGasPricesServiceGranularCacheUpdatedEvent`
- Additionally, the actions are available within `SampleGasPricesServiceActions` and the events are available within `SampleGasPricesServiceEvents`
- Add optional `queryClientConfig` constructor argument which can be used to configure the underlying TanStack Query client ([#8343](https://github.com/MetaMask/core/pull/8343))
- Add `destroy` method to `BaseDataService` ([#8343](https://github.com/MetaMask/core/pull/8343))

### Changed

- **BREAKING:** `SampleGasPricesService` now inherits from `BaseDataService` from `@metamask/base-data-service` ([#8343](https://github.com/MetaMask/core/pull/8343))
Copy link
Copy Markdown
Contributor Author

@mcmire mcmire Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This very well may not be breaking. I'll have to take a closer look at this, but I wanted to call it out in case it was.

- Update `SampleGasPricesService.fetchGasPrices` (and messenger action) so requests to API will be cached and/or deduplicated ([#8343](https://github.com/MetaMask/core/pull/8343))
- Add new dependencies ([#8343](https://github.com/MetaMask/core/pull/8343))
- Add `@metamask/base-data-service` ^0.1.1
- Add `@tanstack/query-core` ^4.43.0
- Add `@metamask/superstruct` ^3.2.1

### Removed

- **BREAKING:** Remove `onRetry`, `onBreak`, and `onDegraded` ([#8343](https://github.com/MetaMask/core/pull/8343))
- You are free to implement these methods in your "real" service class if you need them, but we no longer require you to do so.

## [4.0.4]

### Changed
Expand Down
5 changes: 4 additions & 1 deletion packages/sample-controllers/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,12 @@
},
"dependencies": {
"@metamask/base-controller": "^9.0.1",
"@metamask/base-data-service": "^0.1.1",
"@metamask/messenger": "^1.0.0",
"@metamask/network-controller": "^30.0.1",
"@metamask/utils": "^11.9.0"
"@metamask/superstruct": "^3.1.0",
"@metamask/utils": "^11.9.0",
"@tanstack/query-core": "^4.43.0"
},
"devDependencies": {
"@metamask/auto-changelog": "^3.4.4",
Expand Down
3 changes: 3 additions & 0 deletions packages/sample-controllers/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
export type {
SampleGasPricesServiceInvalidateQueriesAction,
SampleGasPricesServiceActions,
SampleGasPricesServiceCacheUpdatedEvent,
SampleGasPricesServiceGranularCacheUpdatedEvent,
SampleGasPricesServiceEvents,
SampleGasPricesServiceMessenger,
} from './sample-gas-prices-service/sample-gas-prices-service';
Expand Down
229 changes: 22 additions & 207 deletions packages/sample-controllers/src/sample-gas-prices-service/sample-gas-prices-service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { HttpError } from '@metamask/controller-utils';
import { DEFAULT_MAX_RETRIES } from '@metamask/controller-utils';
import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger';
import type {
MockAnyNamespace,
Expand All @@ -11,14 +11,6 @@ import type { SampleGasPricesServiceMessenger } from './sample-gas-prices-servic
import { SampleGasPricesService } from './sample-gas-prices-service';

describe('SampleGasPricesService', () => {
beforeEach(() => {
jest.useFakeTimers({ doNotFake: ['nextTick', 'queueMicrotask'] });
});

afterEach(() => {
jest.useRealTimers();
});

describe('SampleGasPricesService:fetchGasPrices', () => {
it('returns the low, average, and high gas prices from the API', async () => {
nock('https://api.example.com')
Expand All @@ -31,7 +23,7 @@ describe('SampleGasPricesService', () => {
high: 15,
},
});
const { rootMessenger } = getService();
const { rootMessenger } = createService();

const gasPricesResponse = await rootMessenger.call(
'SampleGasPricesService:fetchGasPrices',
Expand All @@ -45,6 +37,19 @@ describe('SampleGasPricesService', () => {
});
});

it('throws if the API returns a non-200 status', async () => {
nock('https://api.example.com')
.get('/gas-prices')
.query({ chainId: 'eip155:1' })
.times(DEFAULT_MAX_RETRIES + 1)
.reply(500);
const { rootMessenger } = createService();

await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow("Gas prices API failed with status '500'");
});

it.each([
'not an object',
{ missing: 'data' },
Expand All @@ -62,202 +67,13 @@ describe('SampleGasPricesService', () => {
.get('/gas-prices')
.query({ chainId: 'eip155:1' })
.reply(200, JSON.stringify(response));
const { rootMessenger } = getService();
const { rootMessenger } = createService();

await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow('Malformed response received from gas prices API');
},
);

it('calls onDegraded listeners if the request takes longer than 5 seconds to resolve', async () => {
Copy link
Copy Markdown
Contributor Author

@mcmire mcmire Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed these methods from the class so these tests are no longer necessary.

nock('https://api.example.com')
.get('/gas-prices')
.query({ chainId: 'eip155:1' })
.reply(200, () => {
jest.advanceTimersByTime(6000);
return {
data: {
low: 5,
average: 10,
high: 15,
},
};
});
const { service, rootMessenger } = getService();
const onDegradedListener = jest.fn();
service.onDegraded(onDegradedListener);

await rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1');

expect(onDegradedListener).toHaveBeenCalled();
});

it('allows the degradedThreshold to be changed', async () => {
nock('https://api.example.com')
.get('/gas-prices')
.query({ chainId: 'eip155:1' })
.reply(200, () => {
jest.advanceTimersByTime(1000);
return {
data: {
low: 5,
average: 10,
high: 15,
},
};
});
const { service, rootMessenger } = getService({
options: {
policyOptions: { degradedThreshold: 500 },
},
});
const onDegradedListener = jest.fn();
service.onDegraded(onDegradedListener);

await rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1');

expect(onDegradedListener).toHaveBeenCalled();
});

it('attempts a request that responds with non-200 up to 4 times, throwing if it never succeeds', async () => {
nock('https://api.example.com')
.get('/gas-prices')
.query({ chainId: 'eip155:1' })
.times(4)
.reply(500);
const { service, rootMessenger } = getService();
service.onRetry(() => {
jest.advanceTimersToNextTimerAsync().catch(console.error);
});

await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow(
"Fetching 'https://api.example.com/gas-prices?chainId=eip155%3A1' failed with status '500'",
);
});

it('calls onDegraded listeners when the maximum number of retries is exceeded', async () => {
nock('https://api.example.com')
.get('/gas-prices')
.query({ chainId: 'eip155:1' })
.times(4)
.reply(500);
const { service, rootMessenger } = getService();
service.onRetry(() => {
jest.advanceTimersToNextTimerAsync().catch(console.error);
});
const onDegradedListener = jest.fn();
service.onDegraded(onDegradedListener);

await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow(
"Fetching 'https://api.example.com/gas-prices?chainId=eip155%3A1' failed with status '500'",
);
expect(onDegradedListener).toHaveBeenCalled();
});

it('intercepts requests and throws a circuit break error after the 4th failed attempt, running onBreak listeners', async () => {
nock('https://api.example.com')
.get('/gas-prices')
.query({ chainId: 'eip155:1' })
.times(12)
.reply(500);
const { service, rootMessenger } = getService();
service.onRetry(() => {
jest.advanceTimersToNextTimerAsync().catch(console.error);
});
const onBreakListener = jest.fn();
service.onBreak(onBreakListener);

// Should make 4 requests
await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow(
"Fetching 'https://api.example.com/gas-prices?chainId=eip155%3A1' failed with status '500'",
);
// Should make 4 requests
await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow(
"Fetching 'https://api.example.com/gas-prices?chainId=eip155%3A1' failed with status '500'",
);
// Should make 4 requests
await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow(
"Fetching 'https://api.example.com/gas-prices?chainId=eip155%3A1' failed with status '500'",
);
// Should not make an additional request (we only mocked 12 requests
// above)
await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow(
'Execution prevented because the circuit breaker is open',
);
expect(onBreakListener).toHaveBeenCalledWith({
error: new HttpError(
500,
"Fetching 'https://api.example.com/gas-prices?chainId=eip155%3A1' failed with status '500'",
),
});
});

it('resumes requests after the circuit break duration passes, returning the API response if the request ultimately succeeds', async () => {
const circuitBreakDuration = 5_000;
nock('https://api.example.com')
.get('/gas-prices')
.query({ chainId: 'eip155:1' })
.times(12)
.reply(500)
.get('/gas-prices')
.query({ chainId: 'eip155:1' })
.reply(200, {
data: {
low: 5,
average: 10,
high: 15,
},
});
const { service, rootMessenger } = getService({
options: {
policyOptions: { circuitBreakDuration },
},
});
service.onRetry(() => {
jest.advanceTimersToNextTimerAsync().catch(console.error);
});

await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow(
"Fetching 'https://api.example.com/gas-prices?chainId=eip155%3A1' failed with status '500'",
);
await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow(
"Fetching 'https://api.example.com/gas-prices?chainId=eip155%3A1' failed with status '500'",
);
await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow(
"Fetching 'https://api.example.com/gas-prices?chainId=eip155%3A1' failed with status '500'",
);
await expect(
rootMessenger.call('SampleGasPricesService:fetchGasPrices', '0x1'),
).rejects.toThrow(
'Execution prevented because the circuit breaker is open',
);
await jest.advanceTimersByTimeAsync(circuitBreakDuration);
const gasPricesResponse = await service.fetchGasPrices('0x1');
expect(gasPricesResponse).toStrictEqual({
low: 5,
average: 10,
high: 15,
});
});
});

describe('fetchGasPrices', () => {
Expand All @@ -272,7 +88,7 @@ describe('SampleGasPricesService', () => {
high: 15,
},
});
const { service } = getService();
const { service } = createService();

const gasPricesResponse = await service.fetchGasPrices('0x1');

Expand Down Expand Up @@ -301,7 +117,7 @@ type RootMessenger = Messenger<
*
* @returns The root messenger.
*/
function getRootMessenger(): RootMessenger {
function createRootMessenger(): RootMessenger {
Copy link
Copy Markdown
Contributor Author

@mcmire mcmire Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get* did not seem like an appropriate prefix for a factory function. The choice of prefix for factory functions was brought up in the past (by Erik I believe) and if I remember correctly we settled on create*.

return new Messenger({ namespace: MOCK_ANY_NAMESPACE });
}

Expand All @@ -312,7 +128,7 @@ function getRootMessenger(): RootMessenger {
* events required by the controller's messenger.
* @returns The service-specific messenger.
*/
function getMessenger(
function createServiceMessenger(
rootMessenger: RootMessenger,
): SampleGasPricesServiceMessenger {
return new Messenger({
Expand All @@ -330,7 +146,7 @@ function getMessenger(
* `messenger`).
* @returns The new service, root messenger, and service messenger.
*/
function getService({
function createService({
options = {},
}: {
options?: Partial<ConstructorParameters<typeof SampleGasPricesService>[0]>;
Expand All @@ -339,10 +155,9 @@ function getService({
rootMessenger: RootMessenger;
messenger: SampleGasPricesServiceMessenger;
} {
const rootMessenger = getRootMessenger();
const messenger = getMessenger(rootMessenger);
const rootMessenger = createRootMessenger();
const messenger = createServiceMessenger(rootMessenger);
const service = new SampleGasPricesService({
fetch,
messenger,
...options,
});
Expand Down
Loading
Loading