Skip to content

Migrate SampleGasPriceService to BaseDataService#8343

Open
mcmire wants to merge 5 commits intomainfrom
update-sample-gas-price-service
Open

Migrate SampleGasPriceService to BaseDataService#8343
mcmire wants to merge 5 commits intomainfrom
update-sample-gas-price-service

Conversation

@mcmire
Copy link
Copy Markdown
Contributor

@mcmire mcmire commented Mar 30, 2026

Explanation

With the release of @metamask/base-data-service, the SampleGasPricesService class needs to be updated so that new data services follow our best practices.

References

(No ticket, but stumbled upon this while reviewing other PRs such as the AuthenticatedUserStorage service class: #8260)

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
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Introduces breaking API changes (service inheritance and removed onRetry/onBreak/onDegraded) and changes request behavior to be query-cached/deduplicated, which could affect consumers and error/retry semantics.

Overview
Migrates SampleGasPricesService to extend @metamask/base-data-service’s BaseDataService, switching fetchGasPrices to use fetchQuery (TanStack Query) so API calls are cached and deduplicated and adding optional queryClientConfig for cache/client tuning.

Adds new messenger surface area for cache management/observability (:invalidateQueries action plus cacheUpdated events), exports the new types from src/index.ts, and updates tests accordingly while removing the previous onRetry/onBreak/onDegraded hooks. Dependency and TS project references are updated to include @metamask/base-data-service, @tanstack/query-core, and @metamask/superstruct (used for response validation).

Written by Cursor Bugbot for commit 3a60272. This will update automatically on new commits. Configure here.

With the release of `@metamask/base-data-service`, the
`SampleGasPricesService` class needs to be updated so that new data
services that other engineers create follow the standards we want them
to follow.
@mcmire mcmire force-pushed the update-sample-gas-price-service branch from 186aee8 to 896f3cc Compare March 30, 2026 22:05
* @returns The root messenger.
*/
function getRootMessenger(): RootMessenger {
function createRootMessenger(): RootMessenger {
Copy link
Copy Markdown
Contributor Author

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*.

* {@link CockatielEvent}.
* @see {@link createServicePolicy}
*/
onRetry(listener: Parameters<ServicePolicy['onRetry']>[0]): IDisposable {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We probably should add these methods (or something like them) to BaseDataService, but regardless, I don't want to ask that engineers add them to their data service classes unless they really need to.

) {
return { low, average, high };
}
if (!is(jsonResponse, GasPricesResponseStruct)) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We might as well switch to using Superstruct for response validation. Most teams are shifting to using this anyway.


### 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

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.

},
);

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

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.

@mcmire mcmire marked this pull request as ready for review March 31, 2026 00:08
@mcmire mcmire requested a review from a team as a code owner March 31, 2026 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant