diff --git a/packages/ramps-controller/CHANGELOG.md b/packages/ramps-controller/CHANGELOG.md index 7590b66c166..d06b48fb62a 100644 --- a/packages/ramps-controller/CHANGELOG.md +++ b/packages/ramps-controller/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Refactor: Consolidate reset logic with a shared resetResource helper and fix abort handling for dependent resources ([#7818](https://github.com/MetaMask/core/pull/7818)) - **BREAKING:** Restructure `RampsControllerState` to use nested `ResourceState` objects for each resource with `data`, `selected`, `isLoading`, and `error` ([#7779](https://github.com/MetaMask/core/pull/7779)) ## [5.1.0] diff --git a/packages/ramps-controller/src/RampsController.test.ts b/packages/ramps-controller/src/RampsController.test.ts index 76ed13c196a..9c76678e6ad 100644 --- a/packages/ramps-controller/src/RampsController.test.ts +++ b/packages/ramps-controller/src/RampsController.test.ts @@ -10,11 +10,13 @@ import * as path from 'path'; import type { RampsControllerMessenger, + RampsControllerState, ResourceState, UserRegion, } from './RampsController'; import { RampsController, + getDefaultRampsControllerState, RAMPS_CONTROLLER_REQUIRED_SERVICE_ACTIONS, } from './RampsController'; import type { @@ -470,6 +472,61 @@ describe('RampsController', () => { ); }); }); + + it('returns providers for region when state has providers (fetches and returns result)', async () => { + await withController( + { + options: { + state: { + userRegion: createMockUserRegion('us-ca'), + providers: createResourceState(mockProviders, null), + }, + }, + }, + async ({ controller, rootMessenger }) => { + let serviceCalled = false; + rootMessenger.registerActionHandler( + 'RampsService:getProviders', + async () => { + serviceCalled = true; + return { providers: mockProviders }; + }, + ); + + const result = await controller.getProviders('us-ca'); + + expect(serviceCalled).toBe(true); + expect(result.providers).toStrictEqual(mockProviders); + }, + ); + }); + + it('calls service when getProviders is called with filter options even if state has providers', async () => { + await withController( + { + options: { + state: { + userRegion: createMockUserRegion('us-ca'), + providers: createResourceState(mockProviders, null), + }, + }, + }, + async ({ controller, rootMessenger }) => { + let serviceCalled = false; + rootMessenger.registerActionHandler( + 'RampsService:getProviders', + async () => { + serviceCalled = true; + return { providers: mockProviders }; + }, + ); + + await controller.getProviders('us-ca', { provider: 'moonpay' }); + + expect(serviceCalled).toBe(true); + }, + ); + }); }); describe('metadata', () => { @@ -1144,6 +1201,42 @@ describe('RampsController', () => { expect(controller.state.countries.data).toStrictEqual(mockCountries); }); }); + + it('stores empty array when getCountries returns non-array (defensive)', async () => { + await withController(async ({ controller, rootMessenger }) => { + rootMessenger.registerActionHandler( + 'RampsService:getCountries', + async () => 'not an array' as unknown as Country[], + ); + + const countries = await controller.getCountries(); + + expect(countries).toBe('not an array'); + expect(controller.state.countries.data).toStrictEqual([]); + }); + }); + + it('does not throw when updating resource field and resource is null (defensive)', async () => { + const stateWithNullCountries = { + ...getDefaultRampsControllerState(), + countries: null, + } as unknown as RampsControllerState; + + await withController( + { + options: { + state: stateWithNullCountries, + }, + }, + async ({ controller, rootMessenger }) => { + rootMessenger.registerActionHandler( + 'RampsService:getCountries', + async () => mockCountries, + ); + await expect(controller.getCountries()).rejects.toThrow(); + }, + ); + }); }); describe('init', () => { @@ -1369,6 +1462,50 @@ describe('RampsController', () => { ); }); }); + + it('calls getTokens and getProviders when hydrating even if state has data', async () => { + const existingProviders: Provider[] = [ + { + id: '/providers/test', + name: 'Test Provider', + environmentType: 'STAGING', + description: 'Test', + hqAddress: '123 Test St', + links: [], + logos: { light: '', dark: '', height: 24, width: 77 }, + }, + ]; + await withController( + { + options: { + state: { + userRegion: createMockUserRegion('us-ca'), + providers: createResourceState(existingProviders, null), + }, + }, + }, + async ({ controller, rootMessenger }) => { + let providersCalled = false; + rootMessenger.registerActionHandler( + 'RampsService:getTokens', + async () => ({ topTokens: [], allTokens: [] }), + ); + rootMessenger.registerActionHandler( + 'RampsService:getProviders', + async () => { + providersCalled = true; + return { providers: [] }; + }, + ); + + controller.hydrateState(); + + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(providersCalled).toBe(true); + }, + ); + }); }); describe('setUserRegion', () => { @@ -2069,6 +2206,8 @@ describe('RampsController', () => { expect(controller.state.providers.selected).toBeNull(); expect(controller.state.paymentMethods.data).toStrictEqual([]); expect(controller.state.paymentMethods.selected).toBeNull(); + expect(controller.state.paymentMethods.isLoading).toBe(false); + expect(controller.state.paymentMethods.error).toBeNull(); }, ); }); @@ -2263,6 +2402,8 @@ describe('RampsController', () => { expect(controller.state.tokens.selected).toBeNull(); expect(controller.state.paymentMethods.data).toStrictEqual([]); expect(controller.state.paymentMethods.selected).toBeNull(); + expect(controller.state.paymentMethods.isLoading).toBe(false); + expect(controller.state.paymentMethods.error).toBeNull(); }, ); }); @@ -2637,6 +2778,61 @@ describe('RampsController', () => { }); }); + it('returns tokens for region when state has tokens (fetches and returns result)', async () => { + await withController( + { + options: { + state: { + userRegion: createMockUserRegion('us-ca'), + tokens: createResourceState(mockTokens, null), + }, + }, + }, + async ({ controller, rootMessenger }) => { + let serviceCalled = false; + rootMessenger.registerActionHandler( + 'RampsService:getTokens', + async () => { + serviceCalled = true; + return mockTokens; + }, + ); + + const result = await controller.getTokens('us-ca', 'buy'); + + expect(serviceCalled).toBe(true); + expect(result).toStrictEqual(mockTokens); + }, + ); + }); + + it('calls service when getTokens is called with provider filter even if state has tokens', async () => { + await withController( + { + options: { + state: { + userRegion: createMockUserRegion('us-ca'), + tokens: createResourceState(mockTokens, null), + }, + }, + }, + async ({ controller, rootMessenger }) => { + let serviceCalled = false; + rootMessenger.registerActionHandler( + 'RampsService:getTokens', + async () => { + serviceCalled = true; + return mockTokens; + }, + ); + + await controller.getTokens('us-ca', 'buy', { provider: 'moonpay' }); + + expect(serviceCalled).toBe(true); + }, + ); + }); + it('prefers provided region over userRegion in state', async () => { await withController( { @@ -3402,10 +3598,10 @@ describe('RampsController', () => { ); controller.setSelectedToken(tokenB.assetId); - await new Promise((resolve) => setTimeout(resolve, 10)); resolveTokenARequest({ payments: paymentMethodsForTokenA }); await tokenAPaymentMethodsPromise; + await new Promise((resolve) => setTimeout(resolve, 10)); expect(controller.state.tokens.selected).toStrictEqual(tokenB); expect(controller.state.paymentMethods.data).toStrictEqual( @@ -3510,10 +3706,10 @@ describe('RampsController', () => { ); controller.setSelectedProvider(providerB.id); - await new Promise((resolve) => setTimeout(resolve, 10)); resolveProviderARequest({ payments: paymentMethodsForProviderA }); await providerAPaymentMethodsPromise; + await new Promise((resolve) => setTimeout(resolve, 10)); expect(controller.state.providers.selected).toStrictEqual(providerB); expect(controller.state.paymentMethods.data).toStrictEqual( diff --git a/packages/ramps-controller/src/RampsController.ts b/packages/ramps-controller/src/RampsController.ts index f72c6b2df7d..2403d5980a0 100644 --- a/packages/ramps-controller/src/RampsController.ts +++ b/packages/ramps-controller/src/RampsController.ts @@ -261,6 +261,28 @@ export function getDefaultRampsControllerState(): RampsControllerState { }; } +const DEPENDENT_RESOURCE_KEYS = [ + 'providers', + 'tokens', + 'paymentMethods', + 'quotes', +] as const; + +type DependentResourceKey = (typeof DEPENDENT_RESOURCE_KEYS)[number]; + +function resetResource( + state: RampsControllerState, + resourceType: DependentResourceKey, + defaultResource?: RampsControllerState[DependentResourceKey], +): void { + const def = defaultResource ?? getDefaultRampsControllerState()[resourceType]; + const resource = state[resourceType]; + resource.data = def.data; + resource.selected = def.selected; + resource.isLoading = def.isLoading; + resource.error = def.error; +} + /** * Resets region-dependent resources (userRegion, providers, tokens, paymentMethods, quotes). * Mutates state in place; use from within controller update() for atomic updates. @@ -276,21 +298,10 @@ function resetDependentResources( if (options?.clearUserRegionData) { state.userRegion = null; } - state.providers.selected = null; - state.providers.data = []; - state.providers.isLoading = false; - state.providers.error = null; - state.tokens.selected = null; - state.tokens.data = null; - state.tokens.isLoading = false; - state.tokens.error = null; - state.paymentMethods.data = []; - state.paymentMethods.selected = null; - state.paymentMethods.isLoading = false; - state.paymentMethods.error = null; - state.quotes.data = null; - state.quotes.isLoading = false; - state.quotes.error = null; + const defaultState = getDefaultRampsControllerState(); + for (const key of DEPENDENT_RESOURCE_KEYS) { + resetResource(state, key, defaultState[key]); + } } // === MESSENGER === @@ -512,30 +523,45 @@ export class RampsController extends BaseController< } /** - * Executes a request with caching and deduplication. + * Executes a request with caching, deduplication, and at most one in-flight + * request per resource type. * - * If a request with the same cache key is already in flight, returns the - * existing promise. If valid cached data exists, returns it without making - * a new request. + * 1. **Same cache key in flight** – If a request with this cache key is + * already pending, returns that promise (deduplication; no second request). * - * @param cacheKey - Unique identifier for this request. - * @param fetcher - Function that performs the actual fetch. Receives an AbortSignal. - * @param options - Options for cache behavior. - * @returns The result of the request. + * 2. **Cache hit** – If valid, non-expired data exists in state.requests for + * this key and forceRefresh is not set, returns that data without fetching. + * + * 3. **New request** – If options.resourceType is set, aborts any other + * in-flight request for that resource so only one request per resource + * runs at a time. Sets resource loading true, runs the fetcher, then on + * success or error updates request state and resource error; in finally, + * clears resource loading only if this request was not aborted. + * + * @param cacheKey - Unique identifier for this request (e.g. from createCacheKey). + * @param fetcher - Async function that performs the fetch. Receives an AbortSignal + * that is aborted when this request is superseded by another for the same resource. + * @param options - Optional forceRefresh, ttl, and resourceType for loading/error state. + * @returns The result of the request (from cache, joined promise, or fetcher). */ async executeRequest( cacheKey: string, fetcher: (signal: AbortSignal) => Promise, options?: ExecuteRequestOptions, ): Promise { + // Get TTL for verifying cache expiration const ttl = options?.ttl ?? this.#requestCacheTTL; - // Check for existing pending request - join it instead of making a duplicate + // DEDUPLICATION: + // Check if a request is already in flight for this cache key + // If so, return the original promise for that request const pending = this.#pendingRequests.get(cacheKey); if (pending) { return pending.promise as Promise; } + // CACHE HIT: + // If cache is not expired, return the cached data if (!options?.forceRefresh) { const cached = this.state.requests[cacheKey]; if (cached && !isCacheExpired(cached, ttl)) { @@ -543,7 +569,8 @@ export class RampsController extends BaseController< } } - // Create abort controller for this request + // Create a new abort controller for this request + // Record the time the request was started const abortController = new AbortController(); const lastFetchedAt = Date.now(); const { resourceType } = options ?? {}; @@ -566,7 +593,6 @@ export class RampsController extends BaseController< try { const data = await fetcher(abortController.signal); - // Don't update state if aborted if (abortController.signal.aborted) { throw new Error('Request was aborted'); } @@ -577,30 +603,23 @@ export class RampsController extends BaseController< ); if (resourceType) { - // We need the extra logic because there are two situations where we’re allowed to clear the error: - // No callback → always clear - // Callback present → clear only when isResultCurrent() returns true. const isCurrent = !options?.isResultCurrent || options.isResultCurrent(); if (isCurrent) { this.#setResourceError(resourceType, null); } } - return data; } catch (error) { - // Don't update state if aborted if (abortController.signal.aborted) { throw error; } const errorMessage = (error as Error)?.message ?? 'Unknown error'; - this.#updateRequestState( cacheKey, createErrorState(errorMessage, lastFetchedAt), ); - if (resourceType) { const isCurrent = !options?.isResultCurrent || options.isResultCurrent(); @@ -608,12 +627,12 @@ export class RampsController extends BaseController< this.#setResourceError(resourceType, errorMessage); } } - throw error; } finally { - // Only delete if this is still our entry (not replaced by a new request) - const currentPending = this.#pendingRequests.get(cacheKey); - if (currentPending?.abortController === abortController) { + if ( + this.#pendingRequests.get(cacheKey)?.abortController === + abortController + ) { this.#pendingRequests.delete(cacheKey); } @@ -631,8 +650,11 @@ export class RampsController extends BaseController< } })(); - // Store pending request for deduplication - this.#pendingRequests.set(cacheKey, { promise, abortController }); + this.#pendingRequests.set(cacheKey, { + promise, + abortController, + resourceType, + }); return promise; } @@ -702,11 +724,18 @@ export class RampsController extends BaseController< field: 'isLoading' | 'error', value: boolean | string | null, ): void { + const current = this.state[resourceType] as + | Record + | undefined + | null; + + if (!current || typeof current !== 'object') { + return; + } + + const next = { ...current, [field]: value }; this.update((state) => { - const resource = state[resourceType]; - if (resource) { - (resource as Record)[field] = value; - } + (state as Record)[resourceType] = next; }); } @@ -879,8 +908,10 @@ export class RampsController extends BaseController< if (providerId === null) { this.update((state) => { state.providers.selected = null; - state.paymentMethods.data = []; - state.paymentMethods.selected = null; + resetResource( + state as unknown as RampsControllerState, + 'paymentMethods', + ); }); return; } @@ -908,8 +939,7 @@ export class RampsController extends BaseController< this.update((state) => { state.providers.selected = provider; - state.paymentMethods.data = []; - state.paymentMethods.selected = null; + resetResource(state as unknown as RampsControllerState, 'paymentMethods'); }); this.#fireAndForget( @@ -974,7 +1004,7 @@ export class RampsController extends BaseController< ); this.update((state) => { - state.countries.data = countries; + state.countries.data = Array.isArray(countries) ? [...countries] : []; }); return countries; @@ -1056,8 +1086,10 @@ export class RampsController extends BaseController< if (!assetId) { this.update((state) => { state.tokens.selected = null; - state.paymentMethods.data = []; - state.paymentMethods.selected = null; + resetResource( + state as unknown as RampsControllerState, + 'paymentMethods', + ); }); return; } @@ -1088,8 +1120,7 @@ export class RampsController extends BaseController< this.update((state) => { state.tokens.selected = token; - state.paymentMethods.data = []; - state.paymentMethods.selected = null; + resetResource(state as unknown as RampsControllerState, 'paymentMethods'); }); this.#fireAndForget( diff --git a/packages/ramps-controller/src/RequestCache.ts b/packages/ramps-controller/src/RequestCache.ts index 08b1c1bcaf7..6f92ff2ee93 100644 --- a/packages/ramps-controller/src/RequestCache.ts +++ b/packages/ramps-controller/src/RequestCache.ts @@ -160,4 +160,6 @@ export type ExecuteRequestOptions = { export type PendingRequest = { promise: Promise; abortController: AbortController; + /** When set, used to abort other in-flight requests for this resource when a new one starts. */ + resourceType?: ResourceType; };