Repository navigation
fix(money): Money API balance freshness + minBlock RPC fallback - #10422
Closed
ffmcgee725 wants to merge 6 commits into
Closed
ffmcgee725 wants to merge 6 commits into
ffmcgee725 wants to merge 6 commits into
Conversation
Member
Author
|
@metamaskbot publish-preview |
ffmcgee725
marked this pull request as ready for review
September 24, 2026 10:37
ffmcgee725
temporarily deployed
to
default-branch
September 24, 2026 10:37 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 b228946. Configure here.
| // An in-flight read started before the caller's reference point would | ||
| // otherwise be shared with this one under the same query key. | ||
| await this.cancelQueries({ queryKey }); | ||
| } |
There was a problem hiding this comment.
Fresh RPC cancel fails sibling fetches
Medium Severity
A fresh getMoneyAccountBalance cancels any in-flight read on the same query key, and the rejected caller surfaces CancelledError. fetchBalanceWithFallback treats that as a real source failure, so it falls back to the Money API or throws MoneyAccountBalanceFetchError. Concurrent post-tx refreshes, or a refresh overlapping a steady-state poll, can therefore return a stale API balance or fail entirely even though another RPC read is succeeding.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b228946. Configure here.
Member
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Jwhiles
reviewed
Sep 24, 2026
| | RemoteFeatureFlagControllerGetStateAction | ||
| | MoneyAccountApiDataServiceFetchPositionsAction; | ||
| | MoneyAccountApiDataServiceFetchPositionsAction | ||
| | MoneyAccountApiDataServiceInvalidateQueriesAction; |
Contributor
There was a problem hiding this comment.
Does this technically make it a breaking change? Since the messenger might need to be altered?
Member
Author
There was a problem hiding this comment.
Very good point actually, let me address this
Jwhiles
reviewed
Sep 24, 2026
| if (options.fresh) { | ||
| // An in-flight read started before the caller's reference point would | ||
| // otherwise be shared with this one under the same query key. | ||
| await this.cancelQueries({ queryKey }); |
Contributor
There was a problem hiding this comment.
Cancelling queries in tanStack will cause other callers to get the previously cached values. So if we make two 'fresh' reads at the same time, the first one will end up getting the old value.
I'm not sure to what extent that's a problem - but it could lead to some confusing situations.
Member
Author
There was a problem hiding this comment.
I'm okay with it here since the intended caller is a single post-tx refresh (overlapping dual-fresh isn’t something we expect), and it’s still better than sharing a pre-tx in-flight promise.
Happy to harden if you think it's worth currently (e.g. revert: false, or bypass fetchQuery on the RPC path like we do for positions), or later if it starts biting us.
Contributor
There was a problem hiding this comment.
Okay, I'm happy to go ahead. As you say we can update this later.
Jwhiles
reviewed
Sep 24, 2026
| await this.invalidateQueries({ queryKey: [...queryKey] }); | ||
| // Bypass fetchQuery: a minBlock-rejected body must not become the | ||
| // steady-state cache entry with a fresh dataUpdatedAt. | ||
| return await this.#fetchPositionsFromNetwork(normalizedAddress, true); |
Contributor
There was a problem hiding this comment.
It looks like when we go down this path we don't cache the result - could we find a way to insert it into the cache?
Member
Author
There was a problem hiding this comment.
Money Account API handles caching in this path 👍🏾
Jwhiles
previously approved these changes
Sep 24, 2026
ffmcgee725
enabled auto-merge
September 24, 2026 14:42
ffmcgee725
disabled auto-merge
September 24, 2026 14:42
Matt561
self-requested a review
September 24, 2026 14:54
Member
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Matt561
reviewed
Sep 24, 2026
Matt561
left a comment
Matt561
left a comment
Contributor
There was a problem hiding this comment.
Added a couple of comments but don't want to delay merging if you disagree 👍
| await this.invalidateQueries({ queryKey: [...queryKey] }); | ||
| // Bypass fetchQuery: a minBlock-rejected body must not become the | ||
| // steady-state cache entry with a fresh dataUpdatedAt. | ||
| return await this.#fetchPositionsFromNetwork(normalizedAddress, true); |
Contributor
There was a problem hiding this comment.
This path appears to bypass the retry and circuit breaker policy from the BaseDataService. Meaning, open breaker cannot stop requests, transient errors get no retries, and health events become inaccurate.
Is it feasible here to still use fetchQuery with pre-cache freshness validation? We could add a validateResponse callback to fetchQuery in the base class that is executed after #policy.execute, processQueryResponse but before returning the response to TanStack.
| ); | ||
| async fetchPositions( | ||
| address: string, | ||
| options?: FetchPositionsOptions, |
Contributor
There was a problem hiding this comment.
Building on top of #10422 (comment) - Does it make sense to move minBlock into FetchPositionOptions so that this module that owns the cache also owns the cache-acceptance decision?
Member
Author
There was a problem hiding this comment.
Having the cache owner own acceptance would also let us drop the invalidateQueries messenger permission. Going to keep minBlock on the balance facade for this PR and follow up with moving it into FetchPositionsOptions + validateResponse since that involves more cross package tweaks.
ffmcgee725
force-pushed
the
jc/MUSD-1425
branch
from
September 24, 2026 15:14
1efee31 to
acac4ba
Compare
Member
Author
|
@metamaskbot publish-preview |
mcmire
reviewed
Sep 24, 2026
| * @param filters - Optional filter for selecting specific queries. | ||
| * @returns Nothing. | ||
| */ | ||
| protected async cancelQueries( |
Collaborator
There was a problem hiding this comment.
Would you mind adding making these changes in a separate PR so they're more visible? (but also see notes for MoneyAccountApiDataService below)
Comment on lines
+258
to
+261
| options?: FetchPositionsOptions, | ||
| ): Promise<PositionResponse> { | ||
| const normalizedAddress = address.toLowerCase(); | ||
| const fresh = options?.fresh === true; |
Collaborator
There was a problem hiding this comment.
Nit: Better to destructure options above and set a default for fresh at the same time? e.g.
Suggested change
| options?: FetchPositionsOptions, | |
| ): Promise<PositionResponse> { | |
| const normalizedAddress = address.toLowerCase(); | |
| const fresh = options?.fresh === true; | |
| { fetch = true }: FetchPositionsOptions, | |
| ): Promise<PositionResponse> { | |
| const normalizedAddress = address.toLowerCase(); |
Comment on lines
+267
to
283
| if (fresh) { | ||
| // Cancel any in-flight non-fresh fetch so it cannot win the shared | ||
| // queryKey and re-seed a response that omitted Cache-Control: no-cache. | ||
| await this.cancelQueries({ queryKey: [...queryKey] }); | ||
| await this.invalidateQueries({ queryKey: [...queryKey] }); | ||
| // Bypass fetchQuery: a minBlock-rejected body must not become the | ||
| // steady-state cache entry with a fresh dataUpdatedAt. Still run through | ||
| // the service policy so transient failures retry and an open breaker | ||
| // can stop the request. | ||
| return await this.executeWithPolicy(async () => { | ||
| return await this.#fetchPositionsFromNetwork(normalizedAddress, true); | ||
| }); | ||
| } | ||
|
|
||
| return this.fetchQuery({ | ||
| queryKey: [`${this.name}:fetchPositions`, address.toLowerCase()], | ||
| queryKey: [...queryKey], | ||
| staleTime: DEFAULT_STALE_TIME_MS, |
Collaborator
There was a problem hiding this comment.
I wonder if there is another way to accomplish this? If you were to set staleTime to 0 when fresh is true would that work to naturally invalidate the query without explicitly needing to call invalidateQueries or even bypass the query client?
Suggested change
| if (fresh) { | |
| // Cancel any in-flight non-fresh fetch so it cannot win the shared | |
| // queryKey and re-seed a response that omitted Cache-Control: no-cache. | |
| await this.cancelQueries({ queryKey: [...queryKey] }); | |
| await this.invalidateQueries({ queryKey: [...queryKey] }); | |
| // Bypass fetchQuery: a minBlock-rejected body must not become the | |
| // steady-state cache entry with a fresh dataUpdatedAt. Still run through | |
| // the service policy so transient failures retry and an open breaker | |
| // can stop the request. | |
| return await this.executeWithPolicy(async () => { | |
| return await this.#fetchPositionsFromNetwork(normalizedAddress, true); | |
| }); | |
| } | |
| return this.fetchQuery({ | |
| queryKey: [`${this.name}:fetchPositions`, address.toLowerCase()], | |
| queryKey: [...queryKey], | |
| staleTime: DEFAULT_STALE_TIME_MS, | |
| if (fresh) { | |
| // Cancel any in-flight non-fresh fetch so it cannot win the shared | |
| // queryKey and re-seed a response that omitted Cache-Control: no-cache. | |
| await this.cancelQueries({ queryKey: [...queryKey] }); | |
| } | |
| return this.fetchQuery({ | |
| queryKey: [...queryKey], | |
| staleTime: fresh ? 0 : DEFAULT_STALE_TIME_MS, |
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
@metamask/money-account-api-data-service: optionalfetchPositions(address, { fresh })cancels in-flight reads, invalidates the client cache, and fetches withCache-Control: no-cachewithout writing through TanStack — so a rejected freshness body cannot seed the next steady-state poll. Asks the Money API to skip its Nest response cache when the server honors the header. Additive optionalmusd_balance_updated_aton the positions balance summary.@metamask/money-account-balance-service:fetchBalanceWithFallbacksurfaces API freshness (asOfBlock,asOfTimestamp,dataFreshness,indexerLagSeconds,musdBalanceUpdatedAt) and accepts additive{ minBlock, fresh }. When the API indexer watermark is behindminBlock, throwsMoneyAccountBalanceStaleError, invalidates the positions query, and falls back to pending-tag RPC without reporting a defect.minBlockimplies a cache-bypassing positions read.@metamask/base-data-service: protectedcancelQueriesfor forced-refresh races.MoneyAccountApiDataService:invalidateQueriesto the balance service.Test plan
money-account-api-data-serviceunit tests (fresh header + no steady-state cache seed)money-account-balance-serviceunit tests (freshness fields, minBlock → RPC, invalidate on stale, api-only stale,musdBalanceUpdatedAtmapping){ fresh: true, minBlock: receipt.blockNumber }fromuseRefreshMoneyBalanceOnTxConfirmNote
Medium Risk
Changes canonical balance fetch paths, TanStack cache invalidation, and post-tx fallback behavior; mistakes could show stale balances or over-fetch, though options are additive and heavily unit-tested.
Overview
Adds post-transaction balance refresh plumbing so Money API reads can bypass client/server caches without poisoning steady-state TanStack entries, and so lagging indexer watermarks can fall back to RPC once.
@metamask/base-data-serviceexposes protectedcancelQueriesandexecuteWithPolicyso subclasses can abort in-flight cached reads and still get retries/circuit-breaker on one-off network calls that must not write through TanStack.@metamask/money-account-api-data-serviceextendsfetchPositionswith optional{ fresh: true }: cancel + invalidate the positions query, sendCache-Control: no-cache, fetch via policy without seeding the query cache, and accept additivemusd_balance_updated_aton the balance summary.@metamask/money-account-balance-serviceextendsfetchBalanceWithFallbackwithFetchBalanceWithFallbackOptions(fresh,minBlock). API-sourced results now include freshness metadata (asOfBlock,asOfTimestamp, etc.). Whenas_of_block<minBlock, it throwsMoneyAccountBalanceStaleError(not reported as a defect), invalidates the positions cache via messengerMoneyAccountApiDataService:invalidateQueries, and falls back to RPC when policy allows;minBlockimplies a fresh positions read.Reviewed by Cursor Bugbot for commit cf53ee5. Bugbot is set up for automated code reviews on this repo. Configure here.