From 166bc0c427f31bc028d4e28d1b52d561fcfb4191 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 12 Dec 2023 18:52:59 -0330 Subject: [PATCH] Retry failed token price updates The `token-prices-service` will now retry token price updates if they fail. The retry strategy is designed to let the API recover if it is experiencing high traffic volumes. The service now requires state in order to hold onto the retry policy. It has been converted to a class for that reason. This required no changes to the abstract price service type or to the `TokenRatesController`. Closes #2084 --- packages/assets-controllers/package.json | 1 + packages/assets-controllers/src/index.ts | 2 +- .../token-prices-service/codefi-v2.test.ts | 438 +++++++++++++++++- .../src/token-prices-service/codefi-v2.ts | 67 ++- .../src/token-prices-service/index.test.ts | 2 +- .../src/token-prices-service/index.ts | 2 +- yarn.lock | 8 + 7 files changed, 495 insertions(+), 25 deletions(-) diff --git a/packages/assets-controllers/package.json b/packages/assets-controllers/package.json index e00d315c6ae..d11ce9d57a2 100644 --- a/packages/assets-controllers/package.json +++ b/packages/assets-controllers/package.json @@ -48,6 +48,7 @@ "@metamask/utils": "^8.2.0", "@types/uuid": "^8.3.0", "async-mutex": "^0.2.6", + "cockatiel": "3.1.1", "ethereumjs-util": "^7.0.10", "lodash": "^4.17.21", "multiformats": "^9.5.2", diff --git a/packages/assets-controllers/src/index.ts b/packages/assets-controllers/src/index.ts index 1977e0ee2db..22793a7edea 100644 --- a/packages/assets-controllers/src/index.ts +++ b/packages/assets-controllers/src/index.ts @@ -13,4 +13,4 @@ export { formatIconUrlWithProxy, getFormattedIpfsUrl, } from './assetsUtil'; -export { codefiTokenPricesServiceV2 } from './token-prices-service'; +export { CodefiTokenPricesServiceV2 } from './token-prices-service'; diff --git a/packages/assets-controllers/src/token-prices-service/codefi-v2.test.ts b/packages/assets-controllers/src/token-prices-service/codefi-v2.test.ts index 8819504f2c9..5f41bdda49a 100644 --- a/packages/assets-controllers/src/token-prices-service/codefi-v2.test.ts +++ b/packages/assets-controllers/src/token-prices-service/codefi-v2.test.ts @@ -1,12 +1,15 @@ import nock from 'nock'; +import { useFakeTimers } from 'sinon'; import { - codefiTokenPricesServiceV2, + CodefiTokenPricesServiceV2, SUPPORTED_CHAIN_IDS, SUPPORTED_CURRENCIES, } from './codefi-v2'; -describe('codefiTokenPricesServiceV2', () => { +const defaultMaxRetryDelay = 30_000; + +describe('new CodefiTokenPricesServiceV2()', () => { describe('fetchTokenPrices', () => { it('uses the /spot-prices endpoint of the Codefi Price API to gather prices for the given tokens', async () => { nock('https://price-api.metafi.codefi.network') @@ -28,7 +31,7 @@ describe('codefiTokenPricesServiceV2', () => { }); const pricedTokensByAddress = - await codefiTokenPricesServiceV2.fetchTokenPrices({ + await new CodefiTokenPricesServiceV2().fetchTokenPrices({ chainId: '0x1', tokenAddresses: ['0xAAA', '0xBBB', '0xCCC'], currency: 'ETH', @@ -70,7 +73,7 @@ describe('codefiTokenPricesServiceV2', () => { }); await expect( - codefiTokenPricesServiceV2.fetchTokenPrices({ + new CodefiTokenPricesServiceV2().fetchTokenPrices({ chainId: '0x1', tokenAddresses: ['0xAAA', '0xBBB', '0xCCC'], currency: 'ETH', @@ -96,13 +99,385 @@ describe('codefiTokenPricesServiceV2', () => { }); await expect( - codefiTokenPricesServiceV2.fetchTokenPrices({ + new CodefiTokenPricesServiceV2().fetchTokenPrices({ chainId: '0x1', tokenAddresses: ['0xAAA', '0xBBB', '0xCCC'], currency: 'ETH', }), ).rejects.toThrow('Could not find price for "0xAAA" in "ETH"'); }); + + it('throws if the request fails consistently', async () => { + nock('https://price-api.metafi.codefi.network') + .get('/v2/chains/1/spot-prices') + .query({ + tokenAddresses: '0xAAA,0xBBB,0xCCC', + vsCurrency: 'ETH', + }) + .replyWithError('Failed to fetch') + .persist(); + + await expect( + new CodefiTokenPricesServiceV2().fetchTokenPrices({ + chainId: '0x1', + tokenAddresses: ['0xAAA', '0xBBB', '0xCCC'], + currency: 'ETH', + }), + ).rejects.toThrow('Failed to fetch'); + }); + + it('throws if the initial request and all retries fail', async () => { + const retries = 3; + nock('https://price-api.metafi.codefi.network') + .get('/v2/chains/1/spot-prices') + .query({ + tokenAddresses: '0xAAA,0xBBB,0xCCC', + vsCurrency: 'ETH', + }) + .times(1 + retries) + .replyWithError('Failed to fetch'); + + await expect( + new CodefiTokenPricesServiceV2({ retries }).fetchTokenPrices({ + chainId: '0x1', + tokenAddresses: ['0xAAA', '0xBBB', '0xCCC'], + currency: 'ETH', + }), + ).rejects.toThrow('Failed to fetch'); + }); + + it('succeeds if the last retry succeeds', async () => { + const retries = 3; + // Initial interceptor for failing requests + nock('https://price-api.metafi.codefi.network') + .get('/v2/chains/1/spot-prices') + .query({ + tokenAddresses: '0xAAA,0xBBB,0xCCC', + vsCurrency: 'ETH', + }) + .times(retries) + .replyWithError('Failed to fetch'); + // Interceptor for successful request + nock('https://price-api.metafi.codefi.network') + .get('/v2/chains/1/spot-prices') + .query({ + tokenAddresses: '0xAAA,0xBBB,0xCCC', + vsCurrency: 'ETH', + }) + .reply(200, { + '0xaaa': { + eth: 148.17205755299946, + }, + '0xbbb': { + eth: 33689.98134554716, + }, + '0xccc': { + eth: 148.1344197578456, + }, + }); + + const pricedTokensByAddress = await new CodefiTokenPricesServiceV2({ + retries, + }).fetchTokenPrices({ + chainId: '0x1', + tokenAddresses: ['0xAAA', '0xBBB', '0xCCC'], + currency: 'ETH', + }); + + expect(pricedTokensByAddress).toStrictEqual({ + '0xAAA': { + tokenAddress: '0xAAA', + value: 148.17205755299946, + currency: 'ETH', + }, + '0xBBB': { + tokenAddress: '0xBBB', + value: 33689.98134554716, + currency: 'ETH', + }, + '0xCCC': { + tokenAddress: '0xCCC', + value: 148.1344197578456, + currency: 'ETH', + }, + }); + }); + + describe('after circuit break', () => { + let clock: sinon.SinonFakeTimers; + + beforeEach(() => { + clock = useFakeTimers({ now: Date.now() }); + }); + + afterEach(() => { + clock.restore(); + }); + + it('stops making fetch requests after too many consecutive failures', async () => { + const retries = 3; + // Max consencutive failures is set to match number of calls in three update attempts (including retries) + const maximumConsecutiveFailures = (1 + retries) * 3; + // Initial interceptor for failing requests + nock('https://price-api.metafi.codefi.network') + .get('/v2/chains/1/spot-prices') + .query({ + tokenAddresses: '0xAAA,0xBBB,0xCCC', + vsCurrency: 'ETH', + }) + .times(maximumConsecutiveFailures) + .replyWithError('Failed to fetch'); + // This interceptor should not be used + const successfullCallScope = nock( + 'https://price-api.metafi.codefi.network', + ) + .get('/v2/chains/1/spot-prices') + .query({ + tokenAddresses: '0xAAA,0xBBB,0xCCC', + vsCurrency: 'ETH', + }) + .reply(200, { + '0xaaa': { + eth: 148.17205755299946, + }, + '0xbbb': { + eth: 33689.98134554716, + }, + '0xccc': { + eth: 148.1344197578456, + }, + }); + const service = new CodefiTokenPricesServiceV2({ + retries, + maximumConsecutiveFailures, + // Ensure break duration is well over the max delay for a single request, so that the + // break doesn't end during a retry attempt + circuitBreakDuration: defaultMaxRetryDelay * 10, + }); + const fetchTokenPrices = () => + service.fetchTokenPrices({ + chainId: '0x1', + tokenAddresses: ['0xAAA', '0xBBB', '0xCCC'], + currency: 'ETH', + }); + // Initial three calls to exhaust maximum allowed failures + // eslint-disable-next-line @typescript-eslint/no-unused-vars + for (const _retryAttempt of Array(retries).keys()) { + // eslint-disable-next-line no-loop-func + await expect(() => + fetchTokenPricesWithFakeTimers({ + clock, + fetchTokenPrices, + retries, + }), + ).rejects.toThrow('Failed to fetch'); + } + + await expect(() => + fetchTokenPricesWithFakeTimers({ + clock, + fetchTokenPrices, + retries, + }), + ).rejects.toThrow( + 'Execution prevented because the circuit breaker is open', + ); + expect(successfullCallScope.isDone()).toBe(false); + }); + + it('keeps circuit closed if first request fails when half-open', async () => { + const retries = 3; + // Max consencutive failures is set to match number of calls in three update attempts (including retries) + const maximumConsecutiveFailures = (1 + retries) * 3; + // Ensure break duration is well over the max delay for a single request, so that the + // break doesn't end during a retry attempt + const circuitBreakDuration = defaultMaxRetryDelay * 10; + // Initial interceptor for failing requests + nock('https://price-api.metafi.codefi.network') + .get('/v2/chains/1/spot-prices') + .query({ + tokenAddresses: '0xAAA,0xBBB,0xCCC', + vsCurrency: 'ETH', + }) + // The +1 is for the additional request when the circuit is half-open + .times(maximumConsecutiveFailures + 1) + .replyWithError('Failed to fetch'); + // This interceptor should not be used + const successfullCallScope = nock( + 'https://price-api.metafi.codefi.network', + ) + .get('/v2/chains/1/spot-prices') + .query({ + tokenAddresses: '0xAAA,0xBBB,0xCCC', + vsCurrency: 'ETH', + }) + .reply(200, { + '0xaaa': { + eth: 148.17205755299946, + }, + '0xbbb': { + eth: 33689.98134554716, + }, + '0xccc': { + eth: 148.1344197578456, + }, + }); + const service = new CodefiTokenPricesServiceV2({ + retries, + maximumConsecutiveFailures, + circuitBreakDuration, + }); + const fetchTokenPrices = () => + service.fetchTokenPrices({ + chainId: '0x1', + tokenAddresses: ['0xAAA', '0xBBB', '0xCCC'], + currency: 'ETH', + }); + // Initial three calls to exhaust maximum allowed failures + // eslint-disable-next-line @typescript-eslint/no-unused-vars + for (const _retryAttempt of Array(retries).keys()) { + // eslint-disable-next-line no-loop-func + await expect(() => + fetchTokenPricesWithFakeTimers({ + clock, + fetchTokenPrices, + retries, + }), + ).rejects.toThrow('Failed to fetch'); + } + // Confirm that circuit has broken + await expect(() => + fetchTokenPricesWithFakeTimers({ + clock, + fetchTokenPrices, + retries, + }), + ).rejects.toThrow( + 'Execution prevented because the circuit breaker is open', + ); + // Wait for circuit to move to half-open + await clock.tickAsync(circuitBreakDuration); + + // The circuit should remain open after the first request fails + // The fetch error is replaced by the circuit break error due to the retries + await expect(() => + fetchTokenPricesWithFakeTimers({ + clock, + fetchTokenPrices, + retries, + }), + ).rejects.toThrow( + 'Execution prevented because the circuit breaker is open', + ); + + // Confirm that the circuit is still open + await expect(() => + fetchTokenPricesWithFakeTimers({ + clock, + fetchTokenPrices, + retries, + }), + ).rejects.toThrow( + 'Execution prevented because the circuit breaker is open', + ); + expect(successfullCallScope.isDone()).toBe(false); + }); + + it('recovers after circuit break', async () => { + const retries = 3; + // Max consencutive failures is set to match number of calls in three update attempts (including retries) + const maximumConsecutiveFailures = (1 + retries) * 3; + // Ensure break duration is well over the max delay for a single request, so that the + // break doesn't end during a retry attempt + const circuitBreakDuration = defaultMaxRetryDelay * 10; + // Initial interceptor for failing requests + nock('https://price-api.metafi.codefi.network') + .get('/v2/chains/1/spot-prices') + .query({ + tokenAddresses: '0xAAA,0xBBB,0xCCC', + vsCurrency: 'ETH', + }) + .times(maximumConsecutiveFailures) + .replyWithError('Failed to fetch'); + // Later interceptor for successfull request after recovery + nock('https://price-api.metafi.codefi.network') + .get('/v2/chains/1/spot-prices') + .query({ + tokenAddresses: '0xAAA,0xBBB,0xCCC', + vsCurrency: 'ETH', + }) + .reply(200, { + '0xaaa': { + eth: 148.17205755299946, + }, + '0xbbb': { + eth: 33689.98134554716, + }, + '0xccc': { + eth: 148.1344197578456, + }, + }); + const service = new CodefiTokenPricesServiceV2({ + retries, + maximumConsecutiveFailures, + circuitBreakDuration, + }); + const fetchTokenPrices = () => + service.fetchTokenPrices({ + chainId: '0x1', + tokenAddresses: ['0xAAA', '0xBBB', '0xCCC'], + currency: 'ETH', + }); + // Initial three calls to exhaust maximum allowed failures + // eslint-disable-next-line @typescript-eslint/no-unused-vars + for (const _retryAttempt of Array(retries).keys()) { + // eslint-disable-next-line no-loop-func + await expect(() => + fetchTokenPricesWithFakeTimers({ + clock, + fetchTokenPrices, + retries, + }), + ).rejects.toThrow('Failed to fetch'); + } + // Confirm that circuit has broken + await expect(() => + fetchTokenPricesWithFakeTimers({ + clock, + fetchTokenPrices, + retries, + }), + ).rejects.toThrow( + 'Execution prevented because the circuit breaker is open', + ); + // Wait for circuit to move to half-open + await clock.tickAsync(circuitBreakDuration); + + const pricedTokensByAddress = await fetchTokenPricesWithFakeTimers({ + clock, + fetchTokenPrices, + retries, + }); + + expect(pricedTokensByAddress).toStrictEqual({ + '0xAAA': { + tokenAddress: '0xAAA', + value: 148.17205755299946, + currency: 'ETH', + }, + '0xBBB': { + tokenAddress: '0xBBB', + value: 33689.98134554716, + currency: 'ETH', + }, + '0xCCC': { + tokenAddress: '0xCCC', + value: 148.1344197578456, + currency: 'ETH', + }, + }); + }); + }); }); describe('validateChainIdSupported', () => { @@ -110,14 +485,14 @@ describe('codefiTokenPricesServiceV2', () => { 'returns true if the given chain ID is %s', (chainId) => { expect( - codefiTokenPricesServiceV2.validateChainIdSupported(chainId), + new CodefiTokenPricesServiceV2().validateChainIdSupported(chainId), ).toBe(true); }, ); it('returns false if the given chain ID is not one of the supported chain IDs', () => { expect( - codefiTokenPricesServiceV2.validateChainIdSupported( + new CodefiTokenPricesServiceV2().validateChainIdSupported( '0x999999999999999', ), ).toBe(false); @@ -129,7 +504,7 @@ describe('codefiTokenPricesServiceV2', () => { 'returns true if the given currency is %s', (currency) => { expect( - codefiTokenPricesServiceV2.validateCurrencySupported(currency), + new CodefiTokenPricesServiceV2().validateCurrencySupported(currency), ).toBe(true); }, ); @@ -138,15 +513,54 @@ describe('codefiTokenPricesServiceV2', () => { 'returns true if the given currency is %s', (currency) => { expect( - codefiTokenPricesServiceV2.validateCurrencySupported(currency), + new CodefiTokenPricesServiceV2().validateCurrencySupported(currency), ).toBe(true); }, ); it('returns false if the given currency is not one of the supported currencies', () => { - expect(codefiTokenPricesServiceV2.validateCurrencySupported('LOL')).toBe( - false, - ); + expect( + new CodefiTokenPricesServiceV2().validateCurrencySupported('LOL'), + ).toBe(false); }); }); }); + +/** + * Calls the 'fetchTokenPrices' function while advancing the clock, allowing + * the function to resolve. + * + * Fetching token rates is challenging in an environment with fake timers + * because we're using a library that automatically retries failed requests, + * which uses `setTimeout` internally. We have to advance the clock after the + * update call starts but before awaiting the result, otherwise it never + * resolves. + * + * @param args - Arguments + * @param args.clock - The fake timers clock to advance. + * @param args.fetchTokenPrices - The "fetchTokenPrices" function to call. + * @param args.retries - The number of retries the fetch call is configured to make. + */ +async function fetchTokenPricesWithFakeTimers({ + clock, + fetchTokenPrices, + retries, +}: { + clock: sinon.SinonFakeTimers; + fetchTokenPrices: () => Promise; + retries: number; +}) { + const pendingUpdate = fetchTokenPrices(); + pendingUpdate.catch(() => { + // suppress Unhandled Promise error + }); + + // Advance timer enough to exceed max possible retry delay for initial call, and all + // subsequent retries + // eslint-disable-next-line @typescript-eslint/no-unused-vars + for (const _retryAttempt of Array(retries + 1).keys()) { + await clock.tickAsync(defaultMaxRetryDelay); + } + + return await pendingUpdate; +} diff --git a/packages/assets-controllers/src/token-prices-service/codefi-v2.ts b/packages/assets-controllers/src/token-prices-service/codefi-v2.ts index f1407ba2234..477122a9e8b 100644 --- a/packages/assets-controllers/src/token-prices-service/codefi-v2.ts +++ b/packages/assets-controllers/src/token-prices-service/codefi-v2.ts @@ -1,6 +1,15 @@ import { handleFetch } from '@metamask/controller-utils'; import type { Hex } from '@metamask/utils'; import { hexToNumber } from '@metamask/utils'; +import { + circuitBreaker, + ConsecutiveBreaker, + ExponentialBackoff, + handleAll, + type IPolicy, + retry, + wrap, +} from 'cockatiel'; import type { AbstractTokenPricesService, @@ -238,15 +247,53 @@ type SupportedChainId = (typeof SUPPORTED_CHAIN_IDS)[number]; */ const BASE_URL = 'https://price-api.metafi.codefi.network/v2'; +const DEFAULT_TOKEN_PRICE_RETRIES = 3; +// Each update attempt will result (1 + retries) calls if the server is down +const DEFAULT_TOKEN_PRICE_MAX_CONSECUTIVE_FAILURES = + (1 + DEFAULT_TOKEN_PRICE_RETRIES) * 3; + /** * This version of the token prices service uses V2 of the Codefi Price API to * fetch token prices. */ -export const codefiTokenPricesServiceV2: AbstractTokenPricesService< - SupportedChainId, - Hex, - SupportedCurrency -> = { +export class CodefiTokenPricesServiceV2 + implements + AbstractTokenPricesService +{ + #tokenPricePolicy: IPolicy; + + /** + * Construct a Codefi Token Price Service. + * + * @param options - Constructor options + * @param options.retries - Number of retry attempts for each token price update. + * @param options.maximumConsecutiveFailures - The maximum number of consecutive failures + * allowed before breaking the circuit and pausing further updates. + * @param options.circuitBreakDuration - The amount of time to wait when the circuit breaks + * from too many consecutive failures. + */ + constructor({ + retries = DEFAULT_TOKEN_PRICE_RETRIES, + maximumConsecutiveFailures = DEFAULT_TOKEN_PRICE_MAX_CONSECUTIVE_FAILURES, + circuitBreakDuration = 30 * 60 * 1000, + }: { + retries?: number; + maximumConsecutiveFailures?: number; + circuitBreakDuration?: number; + } = {}) { + // Construct a policy that will retry each update, and halt further updates + // for a certain period after too many consecutive failures. + const retryPolicy = retry(handleAll, { + maxAttempts: retries, + backoff: new ExponentialBackoff(), + }); + const circuitBreakerPolicy = circuitBreaker(handleAll, { + halfOpenAfter: circuitBreakDuration, + breaker: new ConsecutiveBreaker(maximumConsecutiveFailures), + }); + this.#tokenPricePolicy = wrap(retryPolicy, circuitBreakerPolicy); + } + /** * Retrieves prices in the given currency for the tokens identified by the * given addresses which are expected to live on the given chain. @@ -275,7 +322,7 @@ export const codefiTokenPricesServiceV2: AbstractTokenPricesService< const pricesByCurrencyByTokenAddress: SpotPricesEndpointData< Lowercase, Lowercase - > = await handleFetch(url); + > = await this.#tokenPricePolicy.execute(() => handleFetch(url)); return tokenAddresses.reduce( ( @@ -312,7 +359,7 @@ export const codefiTokenPricesServiceV2: AbstractTokenPricesService< }, {}, ) as TokenPricesByTokenAddress; - }, + } /** * Type guard for whether the API can return token prices for the given chain @@ -324,7 +371,7 @@ export const codefiTokenPricesServiceV2: AbstractTokenPricesService< validateChainIdSupported(chainId: unknown): chainId is SupportedChainId { const supportedChainIds: readonly string[] = SUPPORTED_CHAIN_IDS; return typeof chainId === 'string' && supportedChainIds.includes(chainId); - }, + } /** * Type guard for whether the API can return token prices in the given @@ -340,5 +387,5 @@ export const codefiTokenPricesServiceV2: AbstractTokenPricesService< typeof currency === 'string' && supportedCurrencies.includes(currency.toLowerCase()) ); - }, -}; + } +} diff --git a/packages/assets-controllers/src/token-prices-service/index.test.ts b/packages/assets-controllers/src/token-prices-service/index.test.ts index 90b95c628cf..f70aaf2d00c 100644 --- a/packages/assets-controllers/src/token-prices-service/index.test.ts +++ b/packages/assets-controllers/src/token-prices-service/index.test.ts @@ -4,7 +4,7 @@ describe('token-prices-service', () => { it('has expected exports', () => { expect(Object.keys(allExports)).toMatchInlineSnapshot(` Array [ - "codefiTokenPricesServiceV2", + "CodefiTokenPricesServiceV2", ] `); }); diff --git a/packages/assets-controllers/src/token-prices-service/index.ts b/packages/assets-controllers/src/token-prices-service/index.ts index edb6ff1d1f9..0c0cbd56c84 100644 --- a/packages/assets-controllers/src/token-prices-service/index.ts +++ b/packages/assets-controllers/src/token-prices-service/index.ts @@ -1,2 +1,2 @@ export type { AbstractTokenPricesService } from './abstract-token-prices-service'; -export { codefiTokenPricesServiceV2 } from './codefi-v2'; +export { CodefiTokenPricesServiceV2 } from './codefi-v2'; diff --git a/yarn.lock b/yarn.lock index 00dab9fccc9..da5034445b5 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1757,6 +1757,7 @@ __metadata: "@types/node": ^16.18.54 "@types/uuid": ^8.3.0 async-mutex: ^0.2.6 + cockatiel: 3.1.1 deepmerge: ^4.2.2 ethereumjs-util: ^7.0.10 jest: ^27.5.1 @@ -4763,6 +4764,13 @@ __metadata: languageName: node linkType: hard +"cockatiel@npm:3.1.1": + version: 3.1.1 + resolution: "cockatiel@npm:3.1.1" + checksum: c394fa5dc5a0f21a9ff9f007f16320a162000191c570fa277b527a72505a954aae5f2e93b0de0a558f5e3340fed37c014c9fe72d43adfee4aa09d976bdefe745 + languageName: node + linkType: hard + "collect-v8-coverage@npm:^1.0.0": version: 1.0.2 resolution: "collect-v8-coverage@npm:1.0.2"