Skip to content

Commit

Permalink
chore: limit bridge quote refreshes
Browse files Browse the repository at this point in the history
  • Loading branch information
@micaelae
micaelae committed Nov 9, 2024
1 parent 7888b60 commit 1a77d8b
Show file tree
Hide file tree
Showing 15 changed files with 306 additions and 50 deletions.
1 change: 1 addition & 0 deletions app/scripts/constants/sentry-state.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ export const SENTRY_BACKGROUND_STATE = {
quotes: [],
quotesLastFetched: true,
quotesLoadingStatus: true,
quotesRefreshCount: true,
},
},
CronjobController: {
Expand Down
152 changes: 142 additions & 10 deletions app/scripts/controllers/bridge/bridge-controller.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { flushPromises } from '../../../../test/lib/timer-helpers';
// TODO: Remove restricted import
// eslint-disable-next-line import/no-restricted-paths
import * as bridgeUtil from '../../../../ui/pages/bridge/bridge.util';
import * as balanceUtils from '../../../../shared/modules/bridge-utils/balance';
import BridgeController from './bridge-controller';
import { BridgeControllerMessenger } from './types';
import { DEFAULT_BRIDGE_CONTROLLER_STATE } from './constants';
Expand Down Expand Up @@ -37,7 +38,7 @@ describe('BridgeController', function () {
.reply(200, {
'extension-config': {
refreshRate: 3,
maxRefreshCount: 1,
maxRefreshCount: 3,
},
'extension-support': true,
'src-network-allowlist': [10, 534352],
Expand Down Expand Up @@ -78,7 +79,7 @@ describe('BridgeController', function () {
destNetworkAllowlist: [CHAIN_IDS.POLYGON, CHAIN_IDS.ARBITRUM],
srcNetworkAllowlist: [CHAIN_IDS.OPTIMISM, CHAIN_IDS.SCROLL],
extensionConfig: {
maxRefreshCount: 1,
maxRefreshCount: 3,
refreshRate: 3,
},
};
Expand Down Expand Up @@ -236,7 +237,13 @@ describe('BridgeController', function () {
bridgeController,
'startPollingByNetworkClientId',
);
messengerMock.call.mockReturnValue({ address: '0x123' } as never);
const hasSufficientBalanceSpy = jest
.spyOn(balanceUtils, 'hasSufficientBalance')
.mockResolvedValue(true);
messengerMock.call.mockReturnValue({
address: '0x123',
provider: jest.fn(),
} as never);

const fetchBridgeQuotesSpy = jest
.spyOn(bridgeUtil, 'fetchBridgeQuotes')
Expand Down Expand Up @@ -280,9 +287,10 @@ describe('BridgeController', function () {

expect(stopAllPollingSpy).toHaveBeenCalledTimes(1);
expect(startPollingByNetworkClientIdSpy).toHaveBeenCalledTimes(1);
expect(hasSufficientBalanceSpy).toHaveBeenCalledTimes(1);
expect(startPollingByNetworkClientIdSpy).toHaveBeenCalledWith('0x1', {
...quoteRequest,
insufficientBal: true,
insufficientBal: false,
});

expect(bridgeController.state.bridgeState).toStrictEqual(
Expand All @@ -302,7 +310,7 @@ describe('BridgeController', function () {
expect(fetchBridgeQuotesSpy).toHaveBeenCalledWith(
{
...quoteRequest,
insufficientBal: true,
insufficientBal: false,
},
expect.any(AbortSignal),
);
Expand All @@ -312,7 +320,7 @@ describe('BridgeController', function () {

expect(bridgeController.state.bridgeState).toEqual(
expect.objectContaining({
quoteRequest: { ...quoteRequest, insufficientBal: true },
quoteRequest: { ...quoteRequest, insufficientBal: false },
quotes: [],
quotesLoadingStatus: 0,
}),
Expand All @@ -323,7 +331,7 @@ describe('BridgeController', function () {
await flushPromises();
expect(bridgeController.state.bridgeState).toEqual(
expect.objectContaining({
quoteRequest: { ...quoteRequest, insufficientBal: true },
quoteRequest: { ...quoteRequest, insufficientBal: false },
quotes: [1, 2, 3],
quotesLoadingStatus: 1,
}),
Expand All @@ -335,14 +343,15 @@ describe('BridgeController', function () {
// After 2nd fetch
jest.advanceTimersByTime(50000);
await flushPromises();
expect(fetchBridgeQuotesSpy).toHaveBeenCalledTimes(2);
expect(bridgeController.state.bridgeState).toEqual(
expect.objectContaining({
quoteRequest: { ...quoteRequest, insufficientBal: true },
quoteRequest: { ...quoteRequest, insufficientBal: false },
quotes: [5, 6, 7],
quotesLoadingStatus: 1,
quotesRefreshCount: 2,
}),
);
expect(fetchBridgeQuotesSpy).toHaveBeenCalledTimes(2);
const secondFetchTime =
bridgeController.state.bridgeState.quotesLastFetched;
expect(secondFetchTime).toBeGreaterThan(firstFetchTime);
Expand All @@ -353,14 +362,137 @@ describe('BridgeController', function () {
expect(fetchBridgeQuotesSpy).toHaveBeenCalledTimes(3);
expect(bridgeController.state.bridgeState).toEqual(
expect.objectContaining({
quoteRequest: { ...quoteRequest, insufficientBal: true },
quoteRequest: { ...quoteRequest, insufficientBal: false },
quotes: [5, 6, 7],
quotesLoadingStatus: 2,
quotesRefreshCount: 3,
}),
);
expect(bridgeController.state.bridgeState.quotesLastFetched).toStrictEqual(
secondFetchTime,
);

expect(hasSufficientBalanceSpy).toHaveBeenCalledTimes(1);
});

it('updateBridgeQuoteRequestParams should only poll once if insufficientBal=true', async function () {
jest.useFakeTimers();
const stopAllPollingSpy = jest.spyOn(bridgeController, 'stopAllPolling');
const startPollingByNetworkClientIdSpy = jest.spyOn(
bridgeController,
'startPollingByNetworkClientId',
);
const hasSufficientBalanceSpy = jest
.spyOn(balanceUtils, 'hasSufficientBalance')
.mockResolvedValue(false);
messengerMock.call.mockReturnValue({
address: '0x123',
provider: jest.fn(),
} as never);

const fetchBridgeQuotesSpy = jest
.spyOn(bridgeUtil, 'fetchBridgeQuotes')
.mockImplementationOnce(async () => {
return await new Promise((resolve) => {
return setTimeout(() => {
resolve([1, 2, 3] as never);
}, 5000);
});
});

fetchBridgeQuotesSpy.mockImplementation(async () => {
return await new Promise((resolve) => {
return setTimeout(() => {
resolve([5, 6, 7] as never);
}, 10000);
});
});

const quoteParams = {
srcChainId: 1,
destChainId: 10,
srcTokenAddress: '0x0000000000000000000000000000000000000000',
destTokenAddress: '0x123',
srcTokenAmount: '1000000000000000000',
};
const quoteRequest = {
...quoteParams,
slippage: 0.5,
walletAddress: '0x123',
};
await bridgeController.updateBridgeQuoteRequestParams(quoteParams);

expect(stopAllPollingSpy).toHaveBeenCalledTimes(1);
expect(startPollingByNetworkClientIdSpy).toHaveBeenCalledTimes(1);
expect(hasSufficientBalanceSpy).toHaveBeenCalledTimes(1);
expect(startPollingByNetworkClientIdSpy).toHaveBeenCalledWith('0x1', {
...quoteRequest,
insufficientBal: true,
});

expect(bridgeController.state.bridgeState).toStrictEqual(
expect.objectContaining({
quoteRequest: { ...quoteRequest, walletAddress: undefined },
quotes: DEFAULT_BRIDGE_CONTROLLER_STATE.quotes,
quotesLastFetched: DEFAULT_BRIDGE_CONTROLLER_STATE.quotesLastFetched,
quotesLoadingStatus:
DEFAULT_BRIDGE_CONTROLLER_STATE.quotesLoadingStatus,
}),
);

// Loading state
jest.advanceTimersByTime(1000);
await flushPromises();
expect(fetchBridgeQuotesSpy).toHaveBeenCalledTimes(1);
expect(fetchBridgeQuotesSpy).toHaveBeenCalledWith(
{
...quoteRequest,
insufficientBal: true,
},
expect.any(AbortSignal),
);
expect(bridgeController.state.bridgeState.quotesLastFetched).toStrictEqual(
undefined,
);

expect(bridgeController.state.bridgeState).toEqual(
expect.objectContaining({
quoteRequest: { ...quoteRequest, insufficientBal: true },
quotes: [],
quotesLoadingStatus: 0,
}),
);

// After first fetch
jest.advanceTimersByTime(10000);
await flushPromises();
expect(bridgeController.state.bridgeState).toEqual(
expect.objectContaining({
quoteRequest: { ...quoteRequest, insufficientBal: true },
quotes: [1, 2, 3],
quotesLoadingStatus: 1,
quotesRefreshCount: 1,
}),
);
const firstFetchTime =
bridgeController.state.bridgeState.quotesLastFetched ?? 0;
expect(firstFetchTime).toBeGreaterThan(0);

// After 2nd fetch
jest.advanceTimersByTime(50000);
await flushPromises();
expect(fetchBridgeQuotesSpy).toHaveBeenCalledTimes(1);
expect(bridgeController.state.bridgeState).toEqual(
expect.objectContaining({
quoteRequest: { ...quoteRequest, insufficientBal: true },
quotes: [1, 2, 3],
quotesLoadingStatus: 1,
quotesRefreshCount: 1,
}),
);
const secondFetchTime =
bridgeController.state.bridgeState.quotesLastFetched;
expect(secondFetchTime).toStrictEqual(firstFetchTime);
});

it('updateBridgeQuoteRequestParams should not trigger quote polling if request is invalid', function () {
Expand Down
15 changes: 15 additions & 0 deletions app/scripts/controllers/bridge/bridge-controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export default class BridgeController extends StaticIntervalPollingController<
quotesLastFetched: DEFAULT_BRIDGE_CONTROLLER_STATE.quotesLastFetched,
quotesLoadingStatus:
DEFAULT_BRIDGE_CONTROLLER_STATE.quotesLoadingStatus,
quotesRefreshCount: DEFAULT_BRIDGE_CONTROLLER_STATE.quotesRefreshCount,
};
});

Expand Down Expand Up @@ -195,18 +196,31 @@ export default class BridgeController extends StaticIntervalPollingController<
quoteRequest: request,
};
});
const { maxRefreshCount } =
bridgeState.bridgeFeatureFlags[BridgeFeatureFlagsKey.EXTENSION_CONFIG];
const newQuotesRefreshCount = bridgeState.quotesRefreshCount + 1;

try {
const quotes = await fetchBridgeQuotes(
request,
this.#abortController.signal,
);

// Stop polling if the maximum number of refreshes has been reached
if (
(request.insufficientBal && newQuotesRefreshCount >= 1) ||
(!request.insufficientBal && newQuotesRefreshCount >= maxRefreshCount)
) {
this.stopAllPolling();
}

this.update((_state) => {
_state.bridgeState = {
..._state.bridgeState,
quotes,
quotesLastFetched: Date.now(),
quotesLoadingStatus: RequestStatus.FETCHED,
quotesRefreshCount: newQuotesRefreshCount,
};
});
} catch (error) {
Expand All @@ -220,6 +234,7 @@ export default class BridgeController extends StaticIntervalPollingController<
_state.bridgeState = {
...bridgeState,
quotesLoadingStatus: RequestStatus.ERROR,
quotesRefreshCount: newQuotesRefreshCount,
};
});
console.log('Failed to fetch bridge quotes', error);
Expand Down
1 change: 1 addition & 0 deletions app/scripts/controllers/bridge/constants.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,5 @@ export const DEFAULT_BRIDGE_CONTROLLER_STATE: BridgeControllerState = {
quotes: [],
quotesLastFetched: undefined,
quotesLoadingStatus: undefined,
quotesRefreshCount: 0,
};
1 change: 1 addition & 0 deletions app/scripts/controllers/bridge/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export type BridgeControllerState = {
quotes: QuoteResponse[];
quotesLastFetched?: number;
quotesLoadingStatus?: RequestStatus;
quotesRefreshCount: number;
};

export enum BridgeUserAction {
Expand Down
1 change: 1 addition & 0 deletions test/e2e/tests/metrics/errors.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,7 @@ describe('Sentry errors', function () {
},
quotesLastFetched: true,
quotesLoadingStatus: true,
quotesRefreshCount: true,
},
currentPopupId: false, // Initialized as undefined
// Part of transaction controller store, but missing from the initial
Expand Down
1 change: 1 addition & 0 deletions test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
"srcTokenAddress": "0x0000000000000000000000000000000000000000"
},
"quotes": {},
"quotesRefreshCount": 0,
"srcTokens": {},
"srcTopAssets": {}
}
Expand Down
1 change: 1 addition & 0 deletions test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-ui-state.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@
"srcTokenAddress": "0x0000000000000000000000000000000000000000"
},
"quotes": {},
"quotesRefreshCount": 0,
"srcTokens": {},
"srcTopAssets": {}
},
Expand Down
17 changes: 0 additions & 17 deletions test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-background-state.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,6 @@
},
"snapsInstallPrivacyWarningShown": true
},
"BridgeController": {
"bridgeState": {
"bridgeFeatureFlags": {
"extensionSupport": "boolean",
"srcNetworkAllowlist": {
"0": "string",
"1": "string",
"2": "string"
},
"destNetworkAllowlist": {
"0": "string",
"1": "string",
"2": "string"
}
}
}
},
"CurrencyController": {
"currentCurrency": "usd",
"currencyRates": {
Expand Down
Loading

0 comments on commit 1a77d8b

Please sign in to comment.