Skip to content

fix(ramps-controller): refresh countries catalog on app startup#9261

Open
amitabh94 wants to merge 5 commits into
mainfrom
fix/tram-3682-countries-catalog-refresh-on-start
Open

fix(ramps-controller): refresh countries catalog on app startup#9261
amitabh94 wants to merge 5 commits into
mainfrom
fix/tram-3682-countries-catalog-refresh-on-start

Conversation

@amitabh94

@amitabh94 amitabh94 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Explanation

Stale persisted countries catalog data caused preset amounts (e.g. $100 / $250 / $500) to stay wrong after app restarts until the user changed region, even when the API had updated values.

This change is controller-only in @metamask/ramps-controller — no MetaMask Mobile or portfolio changes.

  • Mark the countries catalog as non-persisted (persist: false) so restarts do not reuse stale catalog data.
  • On init(), always refetch countries via getCountries() (including when userRegion is already set).
  • After each successful getCountries(), re-sync userRegion preset amounts from the fresh catalog via #syncUserRegionFromCountriesCatalog.
  • Tests cover init refetch, userRegion re-sync, and persistence metadata.

Consumers need a new @metamask/ramps-controller release; mobile can pick it up via the usual Core dependency bump.

References

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

Made with Cursor

@amitabh94 amitabh94 requested review from a team as code owners June 25, 2026 04:30
amitabh94 and others added 5 commits June 25, 2026 12:42
Stop persisting the countries catalog and force-refetch via init() so
preset amounts stay in sync after app restarts.

TRAM-3682

Co-authored-by: Cursor <cursoragent@cursor.com>
TRAM-3682

Co-authored-by: Cursor <cursoragent@cursor.com>
@wenfix wenfix force-pushed the fix/tram-3682-countries-catalog-refresh-on-start branch from dae992f to 82c35e8 Compare June 25, 2026 11:56

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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 82c35e8. Configure here.


if (forceRefresh || !hasCountries) {
await this.getCountries(options);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Init contradicts catalog sync grace

Medium Severity

After getCountries(), #syncUserRegionFromCountriesCatalog keeps a persisted userRegion when the refreshed catalog is empty or no longer contains that region. #runInit still always calls setUserRegion with the persisted code, which throws and runs #cleanupState, so startup can fail and wipe the saved region in cases the new tests treat as safe for getCountries alone.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 82c35e8. Configure here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@amitabh94, thoughts on the following?

Finding: init() can wipe the saved user region on app startup

Status: Flagged by Cursor Bugbot (Medium severity), confirmed by code review.

Summary

PR #9261 (part of the Ramps Hardening epic) changes the wallet so that, on every app
startup, it stops trusting the saved list of countries and re-downloads it fresh.
This fixes the original bug where Costa Rica users saw a stale default amount of 100
instead of 25,000.

However, it introduces a new edge case: if that startup download succeeds but comes
back empty — or comes back without the user's own country — the app will erase the user's
saved region entirely
(it gets set to "none"). The user would effectively look like a
brand-new user with no region, until the next successful refresh re-establishes it.

This is the opposite of what the hardening work is trying to achieve (keeping the user's
region and its correct preset amounts stable across restarts).

When does it actually happen?

It does not happen on a normal failed network call (a true failure is caught earlier
and the saved region is left alone). It happens only in this narrower window:

  • The startup countries request succeeds (HTTP 200), but
    • the response body is an empty list, or
    • the list no longer contains the user's region (e.g. the region was delisted, or a
      partial/garbled-but-valid response).

In those cases the saved region is wiped.

Before this PR, this could not happen at startup because the previously-saved country list
(which contained the user's region) was reused. Now the list is always discarded and
re-fetched, so the empty/incomplete-response case is reachable on every cold start.

Technical detail

In RampsController.ts:

  • #runInit() (the startup routine):

    1. await this.getCountries({ forceRefresh: true }) — re-downloads the catalog. Inside
      this, #syncUserRegionFromCountriesCatalog() gracefully keeps the existing region
      if the catalog is empty or doesn't contain the region (it just returns, no change).
    2. Then it unconditionally calls await this.setUserRegion(persistedRegionCode).
  • setUserRegion() does the opposite of the grace logic: if countries.data is empty,
    or findRegionFromCode() returns nothing, it calls #cleanupState() and throws.

  • #cleanupState()resetDependentResources(state, { clearUserRegionData: true })
    state.userRegion = null.

So the lenient guards added in #syncUserRegionFromCountriesCatalog() are immediately
undone by the setUserRegion() call in #runInit(). The net startup behavior in the
empty/region-missing case is: throw + null out userRegion.

The two unit tests added in the PR (leaves userRegion unchanged when the refreshed catalog is empty / ... region is absent ...) assert the grace behavior for getCountries() in
isolation
, which is accurate — but they do not reflect what init() does end-to-end, so
they can give a false sense of safety.

Options

  1. Fix it (recommended). Make init() consistent with the grace logic: when a region is
    already saved, don't let a transient/incomplete fresh catalog wipe it; keep the strict
    "throw if no region" behavior only for a brand-new user (the geolocation path, where no
    valid region exists yet). Sketch:

    async #runInit(options) {
      await this.getCountries({ ...options, forceRefresh: true });
    
      const persistedRegionCode = this.state.userRegion?.regionCode;
      const regionCode =
        persistedRegionCode ?? (await this.messenger.call('RampsService:getGeolocation'));
      if (!regionCode) throw new Error('Failed to fetch geolocation. ...');
    
      // getCountries already re-synced an existing region from the fresh catalog.
      // Don't let a transient/incomplete catalog wipe a previously-valid region.
      if (persistedRegionCode) return;
      await this.setUserRegion(regionCode, options);
    }

    Tradeoff: with this change, a forceRefresh init would no longer re-run
    resetDependentResources for an already-persisted region (today it does, because
    setUserRegion treats forceRefresh as a region change). Need product confirmation that
    that's acceptable.

  2. Accept as-is / out of scope. Decide the empty/region-missing-but-200 response is not
    a realistic case for this API, document the decision on the PR, and ship unchanged.

  3. Alternative hardening: keep calling setUserRegion, but change it so it does not null
    out a previously-valid region on a transient empty catalog (riskier — setUserRegion is
    also used for explicit user-driven region changes where throwing is intended).

Recommendation

Address it as Option 1 (small, contained change in #runInit plus an init-level test that
asserts the saved region survives an empty/region-missing startup refresh), pending
confirmation of the forceRefresh/resetDependentResources tradeoff noted above.

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.

2 participants