Update BaseDataService to accommodate mutations, not just queries by mcmire · Pull Request #9324 · MetaMask/core

mcmire · 2026-06-30T22:03:06Z

Explanation

Currently, BaseDataService has a fetchQuery method which is best used for making read-only requests ("queries" in TanStack Query parlance), but does not work as well for requests that change state on the server side ("mutations"). For instance, it makes sense for queries to be cached, but not so much for mutations.

This commit adds a separate method, executeMutation, which accommodates mutations better. Its implementation is different from fetchQuery as it uses the mutation cache instead of the query cache.

References

https://consensyssoftware.atlassian.net/browse/WPC-1118

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
New protected API for state-changing HTTP calls; incorrect use could duplicate writes if callers retry at a higher layer, but behavior is scoped to base-data-service with tests documenting no automatic mutation retries.

Overview
Adds executeMutation on BaseDataService so subclasses can run server-side writes without treating them like cached reads. It builds mutations through TanStack Query’s mutation cache (not the query cache), requires mutationKey and mutationFn, and disallows per-call retry / retryDelay like fetchQuery.

Mutations are wrapped only in the circuit breaker (circuitBreakerPolicy.execute), not the full service policy execute path used for queries, so failed mutations are not retried automatically. They also do not trigger messenger :cacheUpdated events, since only the query cache is subscribed.

The example ExampleDataService gains addFollower (social API PUT) plus tests and nock mocks covering success, no-retry-on-failure, and no cache events.

^{Reviewed by Cursor Bugbot for commit 5ce00ba. Bugbot is set up for automated code reviews on this repo. Configure here.}

Currently, `BaseDataService` has a `fetchQuery` method which is best used for making read-only requests ("queries" in TanStack Query parlance), but does not work as well for requests that change state on the server side ("mutations"). For instance, queries are cacheable, but mutations are not. This commit adds a separate method, `executeMutation`, which accommodates mutations better. Its implementation is different from `fetchQuery` as it uses the mutation cache instead of the query cache.

mcmire · 2026-06-30T22:06:36Z

+      'mutationKey' | 'mutationFn'
+    >,
+  ): Promise<TData> {
+    const mutationCache = this.#queryClient.getMutationCache();


QueryClient doesn't have a executeMutation method itself. I looked inside of the class, however, and saw that there was a getMutationCache method and followed where that led. This is not how useMutation works, which uses MutationObserver, but I don't know if that really matters. I figured it made more sense to mimic how fetchQuery works. But I'm not very familiar with TanStack Query, so if this is not the way we should be doing things, I'm happy to change this.

mcmire · 2026-06-30T22:07:03Z

    );
  }

+  async addFollower(followerId: string): Promise<AddFollowerResponse> {


Adapted from SocialService:

core/packages/social-controllers/src/SocialService.ts

Line 515 in 552b51e

async follow(options: FollowOptions): Promise<FollowResponse> {

cursor

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 603d673. Configure here.}

cursor · 2026-06-30T22:15:44Z

+      mutationFn: (context) =>
+        this.#policy.execute(() => options.mutationFn(context)),
+    });
+    return await mutation.execute();


Mutation variables never forwarded

Medium Severity

executeMutation always calls mutation.execute() with no arguments, so TanStack mutationFn never receives TVariables. Callers that pass inputs via variables (the usual mutation pattern) instead of closing over them will run with missing or undefined data.

^{Reviewed by Cursor Bugbot for commit 603d673. Configure here.}

We are using an older version of TanStack Query where Mutation.execute doesn't take any arguments. Variables are passed to the Mutation constructor.

FrederikBolding · 2026-07-01T13:36:15Z

+    const mutation = mutationCache.build(this.#queryClient, {
+      ...options,
+      mutationFn: (context) =>
+        this.#policy.execute(() => options.mutationFn(context)),


Hmm, are we safe to apply the policy as-is to mutations?

Just wondering if there could be a problem with accidentally doing double the mutation with retries 🤔

Hmm, good point. We're still making an API request, so I feel like we would still want to have some kind of retry logic. But maybe it needs to be a little different than the logic used for GET requests.

I wonder if the default retry policy for createServicePolicy is too liberal. Right now, if the function you pass to the policy throws any error, then the function will get run again.

In RpcService we only retry connection errors, JSON parse errors, HTTP server errors (5xx), timeout errors, and "connection reset" errors. I wonder if BaseDataService should configure createServicePolicy such that it does the same thing?

Then I would be less worried here, because at that point, either we never make the request, or we do make the request but the server returns a 5xx. And in that case I feel like we ought to assume that the server is well-behaved, i.e. won't attempt to write to a database if it runs into an error. (If the server is not well-behaved, it shouldn't be our fault — the engineer writing the data service should know that and account for it.)

What do you think about that idea?

Improving the defaults to be more "targeted" to BaseDataService makes sense to me. Though I still would be a bit worried that developers configure the service policy mainly for GET requests and don't realize how it may impact a PUT 🤔

I wouldn't expect that most developers would configure the service policy, I would expect them to go with the defaults. But that's a good point. Should we have two kinds of service policies, one for queries and another for mutations? Then if a developer does configure a service policy, it should be more obvious what it's used for, and maybe it will cause them to think about it more critically. Or maybe this is solvable via documentation: we can add an "advanced" section to the tutorial/guide on data services that talks about the service policy, and we can remind the reader to consider non-GET requests when configuring it.

Unsure if a separate service policy or modifications to the default service policy would be preferred. We could consider a default isServiceFailure function that changes depending on the type of request for example? But maybe a separate policy for maximum flexibility is preferred 🤔

Thinking about this some more, I've see realized that it makes more sense to have one policy instead of two. All requests to an API should share one circuit and one counter to keep track of whether the circuit should break. Queries and mutations shouldn't be treated specially. I'll see if I figure out that path tomorrow.

Perhaps it is enough to find a way to disable retrying for mutations?

I guess that approach could work to get this PR merged, and then we could further refine it in another PR if we need to.

FrederikBolding · 2026-07-09T11:34:52Z

+   * Additionally `retry` and `retryDelay` are not available, retries can be customized using the `servicePolicyOptions`.
+   * @returns The mutation results.
+   */
+  protected async executeMutation<


The mutation cache is separate from the query cache, should we sync it with the UI as well?

E.g. https://github.com/MetaMask/core/blob/main/packages/base-data-service/src/BaseDataService.ts#L145-L152

Ooh good point. Okay I'll make this change.

mcmire · 2026-07-09T21:01:55Z

Moving this PR back to draft. There's still more work to do here in order to properly support mutations.

mcmire force-pushed the add-execute-mutation branch from 7e277f0 to 603d673 Compare June 30, 2026 22:07

mcmire commented Jun 30, 2026

View reviewed changes

mcmire marked this pull request as ready for review June 30, 2026 22:14

mcmire requested a review from a team as a code owner June 30, 2026 22:14

mcmire temporarily deployed to default-branch June 30, 2026 22:14 — with GitHub Actions Inactive

cursor Bot reviewed Jun 30, 2026

View reviewed changes

Make test more accurate

e88e14a

FrederikBolding reviewed Jul 1, 2026

View reviewed changes

FrederikBolding reviewed Jul 9, 2026

View reviewed changes

mcmire added 3 commits July 9, 2026 12:48

Merge branch 'main' into add-execute-mutation

ab2386d

Disable retry policy when executing a mutation

748b6e9

Fix tests

5ce00ba

mcmire marked this pull request as draft July 9, 2026 21:01

mcmire mentioned this pull request Jul 10, 2026

Constrain MessengerAdapter to match given data services #9475

Draft

4 tasks

Uh oh!

Uh oh!

Conversation

mcmire commented Jun 30, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Checklist

Uh oh!

mcmire Jun 30, 2026 • 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!

mcmire Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cursor Bot Jun 30, 2026

Choose a reason for hiding this comment

Mutation variables never forwarded

Uh oh!

mcmire Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

FrederikBolding Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

mcmire Jul 1, 2026 • 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!

FrederikBolding Jul 2, 2026 • 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!

mcmire Jul 2, 2026 • 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!

FrederikBolding Jul 6, 2026 • 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!

mcmire Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

FrederikBolding Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

mcmire Jul 9, 2026 • 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!

FrederikBolding Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

mcmire Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

mcmire commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mcmire commented Jun 30, 2026 •

edited by cursor Bot

Loading

mcmire Jun 30, 2026 •

edited

Loading

mcmire Jul 1, 2026 •

edited

Loading

FrederikBolding Jul 2, 2026 •

edited

Loading

mcmire Jul 2, 2026 •

edited

Loading

FrederikBolding Jul 6, 2026 •

edited

Loading

mcmire Jul 9, 2026 •

edited

Loading