Skip to content

feat: trace Bridge transactions and quote fetching #5768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/bridge-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Sentry traces for `BridgeQuotesFetched` and `SwapQuotesFetched` events ([#5780](https://github.com/MetaMask/core/pull/5780))
- Export `isCrossChain` utility ([#5780](https://github.com/MetaMask/core/pull/5780))

### Changed

- **BREAKING:** Remove `BridgeToken` export ([#5768](https://github.com/MetaMask/core/pull/5768))
- **BREAKING** Rename `QuoteResponse.bridgePriceData` to `priceData` ([#5784](https://github.com/MetaMask/core/pull/5784))
- `traceFn` added to BridgeController constructor to enable clients to pass in a custom sentry trace handler ([#5768](https://github.com/MetaMask/core/pull/5768))

## [22.0.0]

Expand Down
29 changes: 27 additions & 2 deletions packages/bridge-controller/src/bridge-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { BigNumber } from '@ethersproject/bignumber';
import { Contract } from '@ethersproject/contracts';
import { Web3Provider } from '@ethersproject/providers';
import type { StateMetadata } from '@metamask/base-controller';
import type { ChainId } from '@metamask/controller-utils';
import type { ChainId, TraceCallback } from '@metamask/controller-utils';
import { SolScope } from '@metamask/keyring-api';
import { abiERC20 } from '@metamask/metamask-eth-abis';
import type { NetworkClientId } from '@metamask/network-controller';
Expand All @@ -20,6 +20,7 @@ import {
REFRESH_INTERVAL_MS,
} from './constants/bridge';
import { CHAIN_IDS } from './constants/chains';
import { TraceName } from './constants/traces';
import { selectIsAssetExchangeRateInState } from './selectors';
import type { QuoteRequest } from './types';
import {
Expand All @@ -37,6 +38,7 @@ import { getAssetIdsForToken, toExchangeRates } from './utils/assets';
import { hasSufficientBalance } from './utils/balance';
import {
getDefaultBridgeControllerState,
isCrossChain,
isSolanaChainId,
sumHexes,
} from './utils/bridge';
Expand Down Expand Up @@ -151,6 +153,8 @@ export class BridgeController extends StaticIntervalPollingController<BridgePoll
properties: CrossChainSwapsEventProperties<T>,
) => void;

readonly #trace: TraceCallback;

readonly #config: {
customBridgeApiBaseUrl?: string;
};
Expand All @@ -163,6 +167,7 @@ export class BridgeController extends StaticIntervalPollingController<BridgePoll
fetchFn,
config,
trackMetaMetricsFn,
traceFn,
}: {
messenger: BridgeControllerMessenger;
state?: Partial<BridgeControllerState>;
Expand All @@ -182,6 +187,7 @@ export class BridgeController extends StaticIntervalPollingController<BridgePoll
eventName: T,
properties: CrossChainSwapsEventProperties<T>,
) => void;
traceFn?: TraceCallback;
}) {
super({
name: BRIDGE_CONTROLLER_NAME,
Expand All @@ -201,6 +207,7 @@ export class BridgeController extends StaticIntervalPollingController<BridgePoll
this.#fetchFn = fetchFn;
this.#trackMetaMetricsFn = trackMetaMetricsFn;
this.#config = config ?? {};
this.#trace = traceFn ?? (((_request, fn) => fn?.()) as TraceCallback);

// Register action handlers
this.messagingSystem.registerActionHandler(
Expand Down Expand Up @@ -453,7 +460,7 @@ export class BridgeController extends StaticIntervalPollingController<BridgePoll
state.quoteFetchError = DEFAULT_BRIDGE_CONTROLLER_STATE.quoteFetchError;
});

try {
const fetchQuotes = async () => {
const quotes = await fetchBridgeQuotes(
updatedQuoteRequest,
// AbortController is always defined by this line, because we assign it a few lines above,
Expand All @@ -473,6 +480,24 @@ export class BridgeController extends StaticIntervalPollingController<BridgePoll
state.quotes = quotesWithL1GasFees ?? quotesWithSolanaFees ?? quotes;
state.quotesLoadingStatus = RequestStatus.FETCHED;
});
};

try {
await this.#trace(
{
name: isCrossChain(
updatedQuoteRequest.srcChainId,
updatedQuoteRequest.destChainId,
)
? TraceName.BridgeQuotesFetched
: TraceName.SwapQuotesFetched,
data: {
srcChainId: formatChainIdToCaip(updatedQuoteRequest.srcChainId),
destChainId: formatChainIdToCaip(updatedQuoteRequest.destChainId),
},
},
fetchQuotes,
);
} catch (error) {
const isAbortError = (error as Error).name === 'AbortError';
const isAbortedDueToReset = error === RESET_STATE_ABORT_MESSAGE;
Expand Down
4 changes: 4 additions & 0 deletions packages/bridge-controller/src/constants/traces.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export enum TraceName {
BridgeQuotesFetched = 'Bridge Quotes Fetched',
SwapQuotesFetched = 'Swap Quotes Fetched',
}
2 changes: 1 addition & 1 deletion packages/bridge-controller/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ export type {
L1GasFees,
SolanaFees,
QuoteMetadata,
BridgeToken,
GasMultiplierByChainId,
FeatureFlagResponse,
BridgeAsset,
Expand Down Expand Up @@ -101,6 +100,7 @@ export {
isSolanaChainId,
getNativeAssetForChainId,
getDefaultBridgeControllerState,
isCrossChain,
} from './utils/bridge';

export { isValidQuoteRequest, formatEtaInMinutes } from './utils/quote';
Expand Down
18 changes: 18 additions & 0 deletions packages/bridge-controller/src/utils/bridge.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { Hex } from '@metamask/utils';
import {
getEthUsdtResetData,
getNativeAssetForChainId,
isCrossChain,
isEthUsdt,
isSolanaChainId,
isSwapsDefaultTokenAddress,
Expand Down Expand Up @@ -202,4 +203,21 @@ describe('Bridge utils', () => {
expect(decimalResult).toStrictEqual(stringifiedDecimalResult);
});
});

describe('isCrossChain', () => {
it('should return false when there is no destChainId', () => {
const result = isCrossChain('0x1');
expect(result).toBe(false);
});

it('should return false when srcChainId is invalid', () => {
const result = isCrossChain('a', '0x1');
expect(result).toBe(false);
});

it('should return false when destChainId is invalid', () => {
const result = isCrossChain('0x1', 'a');
expect(result).toBe(false);
});
});
});
27 changes: 26 additions & 1 deletion packages/bridge-controller/src/utils/bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ import {
SYMBOL_TO_SLIP44_MAP,
type SupportedSwapsNativeCurrencySymbols,
} from '../constants/tokens';
import type { BridgeAsset, BridgeControllerState } from '../types';
import type {
BridgeAsset,
BridgeControllerState,
GenericQuoteRequest,
} from '../types';
import { ChainId } from '../types';

export const getDefaultBridgeControllerState = (): BridgeControllerState => {
Expand Down Expand Up @@ -175,3 +179,24 @@ export const isSolanaChainId = (
}
return chainId.toString() === ChainId.SOLANA.toString();
};

/**
* Checks whether the transaction is a cross-chain transaction by comparing the source and destination chainIds
*
* @param srcChainId - The source chainId
* @param destChainId - The destination chainId
* @returns Whether the transaction is a cross-chain transaction
*/
export const isCrossChain = (
srcChainId: GenericQuoteRequest['srcChainId'],
destChainId?: GenericQuoteRequest['destChainId'],
) => {
try {
if (!destChainId) {
return false;
}
return formatChainIdToCaip(srcChainId) !== formatChainIdToCaip(destChainId);
} catch {
return false;
}
};
14 changes: 3 additions & 11 deletions packages/bridge-controller/src/utils/metrics/properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { AccountsControllerState } from '../../../../accounts-controller/sr
import { DEFAULT_BRIDGE_CONTROLLER_STATE } from '../../constants/bridge';
import type { BridgeControllerState, QuoteResponse, TxData } from '../../types';
import { type GenericQuoteRequest, type QuoteRequest } from '../../types';
import { getNativeAssetForChainId } from '../bridge';
import { getNativeAssetForChainId, isCrossChain } from '../bridge';
import {
formatAddressToAssetId,
formatChainIdToCaip,
Expand Down Expand Up @@ -49,11 +49,7 @@ export const getActionType = (
srcChainId?: GenericQuoteRequest['srcChainId'],
destChainId?: GenericQuoteRequest['destChainId'],
) => {
if (
srcChainId &&
formatChainIdToCaip(srcChainId) ===
formatChainIdToCaip(destChainId ?? srcChainId)
) {
if (srcChainId && !isCrossChain(srcChainId, destChainId ?? srcChainId)) {
return MetricsActionType.SWAPBRIDGE_V1;
}
return MetricsActionType.CROSSCHAIN_V1;
Expand All @@ -69,11 +65,7 @@ export const getSwapType = (
srcChainId?: GenericQuoteRequest['srcChainId'],
destChainId?: GenericQuoteRequest['destChainId'],
) => {
if (
srcChainId &&
formatChainIdToCaip(srcChainId) ===
formatChainIdToCaip(destChainId ?? srcChainId)
) {
if (srcChainId && !isCrossChain(srcChainId, destChainId ?? srcChainId)) {
return MetricsSwapType.SINGLE;
}
return MetricsSwapType.CROSSCHAIN;
Expand Down
5 changes: 5 additions & 0 deletions packages/bridge-status-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Sentry traces for Swap and Bridge `TransactionApprovalCompleted` and `TransactionCompleted` events ([#5780](https://github.com/MetaMask/core/pull/5780))

### Changed

- `traceFn` added to BridgeStatusController constructor to enable clients to pass in a custom sentry trace handler ([#5768](https://github.com/MetaMask/core/pull/5768))
- Replace `bridgePriceData` with `priceData` from QuoteResponse object ([#5784](https://github.com/MetaMask/core/pull/5784))

## [19.0.0]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,31 @@ Array [
]
`;

exports[`BridgeStatusController submitTx: EVM should delay after submitting linea approval 4`] = `
Array [
Array [
Object {
"data": Object {
"srcChainId": "eip155:59144",
"stxEnabled": false,
},
"name": "Bridge Transaction Approval Completed",
},
[Function],
],
Array [
Object {
"data": Object {
"srcChainId": "eip155:59144",
"stxEnabled": false,
},
"name": "Bridge Transaction Completed",
},
[Function],
],
]
`;

exports[`BridgeStatusController submitTx: EVM should handle smart accounts (4337) 1`] = `
Object {
"chainId": "0xa4b1",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ const addTransactionFn = jest.fn();
const estimateGasFeeFn = jest.fn();
const addUserOperationFromTransactionFn = jest.fn();

const getController = (call: jest.Mock) => {
const getController = (call: jest.Mock, traceFn?: jest.Mock) => {
const controller = new BridgeStatusController({
messenger: {
call,
Expand All @@ -536,6 +536,7 @@ const getController = (call: jest.Mock) => {
addTransactionFn,
estimateGasFeeFn,
addUserOperationFromTransactionFn,
traceFn,
});

jest.spyOn(controller, 'startPolling').mockImplementation(jest.fn());
Expand Down Expand Up @@ -1837,12 +1838,17 @@ describe('BridgeStatusController', () => {
const handleLineaDelaySpy = jest
.spyOn(transactionUtils, 'handleLineaDelay')
.mockResolvedValueOnce();
const mockTraceFn = jest
.fn()
.mockImplementation((_p, callback) => callback());

setupApprovalMocks();
setupBridgeMocks();

const { controller, startPollingForBridgeTxStatusSpy } =
getController(mockMessengerCall);
const { controller, startPollingForBridgeTxStatusSpy } = getController(
mockMessengerCall,
mockTraceFn,
);

const lineaQuoteResponse = {
...mockEvmQuoteResponse,
Expand All @@ -1853,6 +1859,7 @@ describe('BridgeStatusController', () => {
const result = await controller.submitTx(lineaQuoteResponse, false);
controller.stopAllPolling();

expect(mockTraceFn).toHaveBeenCalledTimes(2);
expect(handleLineaDelaySpy).toHaveBeenCalledTimes(1);
expect(result).toMatchSnapshot();
expect(startPollingForBridgeTxStatusSpy).toHaveBeenCalledTimes(1);
Expand All @@ -1866,6 +1873,7 @@ describe('BridgeStatusController', () => {
1234567890,
);
expect(mockMessengerCall.mock.calls).toMatchSnapshot();
expect(mockTraceFn.mock.calls).toMatchSnapshot();
});
});
});
Loading
Loading