diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 1d41465cfa..0e0cf97028 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -2239,7 +2239,7 @@ }, "packages/transaction-pay-controller/src/utils/transaction.ts": { "no-restricted-syntax": { - "count": 2 + "count": 6 } }, "packages/user-operation-controller/src/UserOperationController.test.ts": { diff --git a/packages/transaction-pay-controller/CHANGELOG.md b/packages/transaction-pay-controller/CHANGELOG.md index c9bea4df38..e1e1a72ea1 100644 --- a/packages/transaction-pay-controller/CHANGELOG.md +++ b/packages/transaction-pay-controller/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** Re-parse required tokens when asset state changes ([#8714](https://github.com/MetaMask/core/pull/8714)) + - Adds `AssetsControllerStateChangeEvent`, `CurrencyRateStateChange`, `TokenRatesControllerStateChangeEvent`, and `TokensControllerStateChangeEvent` to `AllowedEvents`. + - Consumers must grant these events when creating the controller messenger. - Bump `@metamask/gas-fee-controller` from `^26.1.1` to `^26.2.0` ([#8722](https://github.com/MetaMask/core/pull/8722)) - Bump `@metamask/transaction-controller` from `^65.1.0` to `^65.2.0` ([#8722](https://github.com/MetaMask/core/pull/8722)) diff --git a/packages/transaction-pay-controller/src/TransactionPayController.test.ts b/packages/transaction-pay-controller/src/TransactionPayController.test.ts index 1e18803732..e857a98390 100644 --- a/packages/transaction-pay-controller/src/TransactionPayController.test.ts +++ b/packages/transaction-pay-controller/src/TransactionPayController.test.ts @@ -17,7 +17,11 @@ import type { import { getStrategyOrder } from './utils/feature-flags'; import { updateQuotes } from './utils/quotes'; import { updateSourceAmounts } from './utils/source-amounts'; -import { getTransaction, pollTransactionChanges } from './utils/transaction'; +import { + getTransaction, + subscribeAssetChanges, + subscribeTransactionChanges, +} from './utils/transaction'; jest.mock('./actions/update-fiat-payment'); jest.mock('./actions/update-payment-token'); @@ -41,7 +45,10 @@ describe('TransactionPayController', () => { const getTransactionMock = jest.mocked(getTransaction); const updateSourceAmountsMock = jest.mocked(updateSourceAmounts); const updateQuotesMock = jest.mocked(updateQuotes); - const pollTransactionChangesMock = jest.mocked(pollTransactionChanges); + const subscribeTransactionChangesMock = jest.mocked( + subscribeTransactionChanges, + ); + const subscribeAssetChangesMock = jest.mocked(subscribeAssetChanges); const getStrategyOrderMock = jest.mocked(getStrategyOrder); let messenger: TransactionPayControllerMessenger; let getKeyringControllerStateMock: jest.Mock; @@ -80,6 +87,21 @@ describe('TransactionPayController', () => { updateQuotesMock.mockResolvedValue(true); }); + describe('constructor', () => { + it('subscribes to rate changes for in-flight retry', () => { + const controller = createController(); + + expect(subscribeAssetChangesMock).toHaveBeenCalledWith( + messenger, + expect.any(Function), + expect.any(Function), + ); + + const getControllerState = subscribeAssetChangesMock.mock.calls[0][1]; + expect(getControllerState()).toBe(controller.state); + }); + }); + describe('updatePaymentToken', () => { it('calls util', () => { createController().updatePaymentToken({ @@ -674,7 +696,7 @@ describe('TransactionPayController', () => { ).toBeDefined(); const removeTransactionDataCallback = - pollTransactionChangesMock.mock.calls[0][2]; + subscribeTransactionChangesMock.mock.calls[0][2]; removeTransactionDataCallback(TRANSACTION_ID_MOCK); diff --git a/packages/transaction-pay-controller/src/TransactionPayController.ts b/packages/transaction-pay-controller/src/TransactionPayController.ts index 61cb6b4790..19d4fdfb6f 100644 --- a/packages/transaction-pay-controller/src/TransactionPayController.ts +++ b/packages/transaction-pay-controller/src/TransactionPayController.ts @@ -26,7 +26,11 @@ import type { import { getStrategyOrder } from './utils/feature-flags'; import { updateQuotes } from './utils/quotes'; import { updateSourceAmounts } from './utils/source-amounts'; -import { getTransaction, pollTransactionChanges } from './utils/transaction'; +import { + getTransaction, + subscribeAssetChanges, + subscribeTransactionChanges, +} from './utils/transaction'; const MESSENGER_EXPOSED_METHODS = [ 'getDelegationTransaction', @@ -87,12 +91,18 @@ export class TransactionPayController extends BaseController< MESSENGER_EXPOSED_METHODS, ); - pollTransactionChanges( + subscribeTransactionChanges( messenger, this.#updateTransactionData.bind(this), this.#removeTransactionData.bind(this), ); + subscribeAssetChanges( + messenger, + () => this.state, + this.#updateTransactionData.bind(this), + ); + // eslint-disable-next-line no-new new QuoteRefresher({ getStrategies: this.#getStrategiesWithFallback.bind(this), diff --git a/packages/transaction-pay-controller/src/types.ts b/packages/transaction-pay-controller/src/types.ts index 7c5064498e..d3ecc63e47 100644 --- a/packages/transaction-pay-controller/src/types.ts +++ b/packages/transaction-pay-controller/src/types.ts @@ -1,10 +1,18 @@ -import type { AssetsControllerGetStateForTransactionPayAction } from '@metamask/assets-controller'; +import type { + AssetsControllerGetStateForTransactionPayAction, + AssetsControllerStateChangeEvent, +} from '@metamask/assets-controller'; import type { CurrencyRateControllerGetStateAction, + CurrencyRateStateChange, TokenBalancesControllerGetStateAction, } from '@metamask/assets-controllers'; import type { TokenRatesControllerGetStateAction } from '@metamask/assets-controllers'; -import type { TokensControllerGetStateAction } from '@metamask/assets-controllers'; +import type { TokenRatesControllerStateChangeEvent } from '@metamask/assets-controllers'; +import type { + TokensControllerGetStateAction, + TokensControllerStateChangeEvent, +} from '@metamask/assets-controllers'; import type { AccountTrackerControllerGetStateAction } from '@metamask/assets-controllers'; import type { ControllerStateChangeEvent } from '@metamask/base-controller'; import type { ControllerGetStateAction } from '@metamask/base-controller'; @@ -82,7 +90,11 @@ export type AllowedActions = | TransactionControllerUpdateTransactionAction; export type AllowedEvents = + | AssetsControllerStateChangeEvent | BridgeStatusControllerStateChangeEvent + | CurrencyRateStateChange + | TokenRatesControllerStateChangeEvent + | TokensControllerStateChangeEvent | TransactionControllerStateChangeEvent | TransactionControllerUnapprovedTransactionAddedEvent; diff --git a/packages/transaction-pay-controller/src/utils/required-tokens.ts b/packages/transaction-pay-controller/src/utils/required-tokens.ts index 93367c647b..9103a6c1ce 100644 --- a/packages/transaction-pay-controller/src/utils/required-tokens.ts +++ b/packages/transaction-pay-controller/src/utils/required-tokens.ts @@ -3,7 +3,9 @@ import { toHex } from '@metamask/controller-utils'; import { abiERC20 } from '@metamask/metamask-eth-abis'; import type { TransactionMeta } from '@metamask/transaction-controller'; import type { Hex } from '@metamask/utils'; +import { createModuleLogger } from '@metamask/utils'; +import { projectLogger } from '../logger'; import type { TransactionPayControllerMessenger, TransactionPayRequiredToken, @@ -15,6 +17,8 @@ import { getTokenInfo, } from './token'; +const log = createModuleLogger(projectLogger, 'required-tokens'); + const FOUR_BYTE_TOKEN_TRANSFER = '0xa9059cbb'; /** @@ -61,19 +65,28 @@ function getTokenTransferToken( const { data, to } = getTokenTransferData(transaction) ?? {}; if (!to || !data) { + log('No token transfer detected', { + transactionId: transaction.id, + }); return undefined; } let transferAmount: Hex | undefined; + let decodeError: unknown; try { const result = new Interface(abiERC20).decodeFunctionData('transfer', data); transferAmount = toHex(result._value); - } catch { - // Intentionally empty + } catch (error) { + decodeError = error; } if (transferAmount === undefined) { + log('Failed to decode transfer calldata', { + transactionId: transaction.id, + to, + error: decodeError, + }); return undefined; } @@ -105,6 +118,16 @@ function buildRequiredToken( const tokenBalance = getTokenBalance(messenger, from, chainId, tokenAddress); if (tokenDecimals === undefined || !symbol || fiatRates === undefined) { + log('Missing token data', { + transactionId: transaction.id, + chainId, + tokenAddress, + missing: { + decimals: tokenDecimals === undefined, + symbol: !symbol, + fiatRates: fiatRates === undefined, + }, + }); return undefined; } diff --git a/packages/transaction-pay-controller/src/utils/transaction.test.ts b/packages/transaction-pay-controller/src/utils/transaction.test.ts index c24fbc5c62..c409c89670 100644 --- a/packages/transaction-pay-controller/src/utils/transaction.test.ts +++ b/packages/transaction-pay-controller/src/utils/transaction.test.ts @@ -8,18 +8,25 @@ import type { Hex } from '@metamask/utils'; import { noop } from 'lodash'; import { getMessengerMock } from '../tests/messenger-mock'; -import type { TransactionData, TransactionPayRequiredToken } from '../types'; +import type { + TransactionData, + TransactionPayControllerState, + TransactionPayRequiredToken, +} from '../types'; +import { getAssetsUnifyStateFeature } from './feature-flags'; import { parseRequiredTokens } from './required-tokens'; import { FINALIZED_STATUSES, collectTransactionIds, getTransaction, isPredictWithdrawTransaction, - pollTransactionChanges, + subscribeAssetChanges, + subscribeTransactionChanges, updateTransaction, waitForTransactionConfirmed, } from './transaction'; +jest.mock('./feature-flags'); jest.mock('./required-tokens'); const TRANSACTION_ID_MOCK = '123-456'; @@ -44,6 +51,9 @@ const TRANSCTION_TOKEN_REQUIRED_MOCK = { describe('Transaction Utils', () => { const parseRequiredTokensMock = jest.mocked(parseRequiredTokens); + const getAssetsUnifyStateFeatureMock = jest.mocked( + getAssetsUnifyStateFeature, + ); const { messenger, getTransactionControllerStateMock, @@ -57,6 +67,8 @@ describe('Transaction Utils', () => { getTransactionControllerStateMock.mockReturnValue({ transactions: [] as TransactionMeta[], } as TransactionControllerState); + + getAssetsUnifyStateFeatureMock.mockReturnValue(false); }); describe('getTransaction', () => { @@ -79,13 +91,13 @@ describe('Transaction Utils', () => { }); }); - describe('pollTransactionChanges', () => { + describe('subscribeTransactionChanges', () => { it('updates state for new transactions', () => { const updateTransactionDataMock = jest.fn(); parseRequiredTokensMock.mockReturnValue([TRANSCTION_TOKEN_REQUIRED_MOCK]); - pollTransactionChanges(messenger, updateTransactionDataMock, noop); + subscribeTransactionChanges(messenger, updateTransactionDataMock, noop); publish( 'TransactionController:stateChange', @@ -110,7 +122,7 @@ describe('Transaction Utils', () => { parseRequiredTokensMock.mockReturnValue([TRANSCTION_TOKEN_REQUIRED_MOCK]); - pollTransactionChanges(messenger, updateTransactionDataMock, noop); + subscribeTransactionChanges(messenger, updateTransactionDataMock, noop); publish( 'TransactionController:stateChange', @@ -138,7 +150,7 @@ describe('Transaction Utils', () => { (status) => { const removeTransactionDataMock = jest.fn(); - pollTransactionChanges(messenger, noop, removeTransactionDataMock); + subscribeTransactionChanges(messenger, noop, removeTransactionDataMock); publish( 'TransactionController:stateChange', @@ -165,7 +177,7 @@ describe('Transaction Utils', () => { it('removes state if transaction is deleted', () => { const removeTransactionDataMock = jest.fn(); - pollTransactionChanges(messenger, noop, removeTransactionDataMock); + subscribeTransactionChanges(messenger, noop, removeTransactionDataMock); publish( 'TransactionController:stateChange', @@ -189,6 +201,204 @@ describe('Transaction Utils', () => { }); }); + describe('subscribeAssetChanges', () => { + function buildState( + data: Partial & { + tokens: TransactionPayRequiredToken[]; + } = { tokens: [] }, + ): TransactionPayControllerState { + return { + transactionData: { + [TRANSACTION_ID_MOCK]: { + isLoading: false, + ...data, + } as TransactionData, + }, + }; + } + + let isolatedMessenger: ReturnType['messenger']; + let isolatedPublish: ReturnType['publish']; + let isolatedGetTransactionControllerStateMock: ReturnType< + typeof getMessengerMock + >['getTransactionControllerStateMock']; + + beforeEach(() => { + const fresh = getMessengerMock(); + isolatedMessenger = fresh.messenger; + isolatedPublish = fresh.publish; + isolatedGetTransactionControllerStateMock = + fresh.getTransactionControllerStateMock; + isolatedGetTransactionControllerStateMock.mockReturnValue({ + transactions: [] as TransactionMeta[], + } as TransactionControllerState); + }); + + it('re-parses required tokens for transactions with empty tokens when token rates change', () => { + const updateTransactionDataMock = jest.fn(); + + parseRequiredTokensMock.mockReturnValue([TRANSCTION_TOKEN_REQUIRED_MOCK]); + isolatedGetTransactionControllerStateMock.mockReturnValue({ + transactions: [TRANSACTION_META_MOCK], + } as TransactionControllerState); + + subscribeAssetChanges( + isolatedMessenger, + () => buildState({ tokens: [] }), + updateTransactionDataMock, + ); + + isolatedPublish('TokenRatesController:stateChange', {} as never, []); + + expect(updateTransactionDataMock).toHaveBeenCalledTimes(1); + const transactionData = {} as TransactionData; + updateTransactionDataMock.mock.calls[0][1](transactionData); + expect(transactionData.tokens).toStrictEqual([ + TRANSCTION_TOKEN_REQUIRED_MOCK, + ]); + }); + + it('re-parses required tokens when currency rates change', () => { + const updateTransactionDataMock = jest.fn(); + + parseRequiredTokensMock.mockReturnValue([TRANSCTION_TOKEN_REQUIRED_MOCK]); + isolatedGetTransactionControllerStateMock.mockReturnValue({ + transactions: [TRANSACTION_META_MOCK], + } as TransactionControllerState); + + subscribeAssetChanges( + isolatedMessenger, + () => buildState({ tokens: [] }), + updateTransactionDataMock, + ); + + isolatedPublish('CurrencyRateController:stateChange', {} as never, []); + + expect(updateTransactionDataMock).toHaveBeenCalledTimes(1); + }); + + it('re-parses required tokens when token state changes', () => { + const updateTransactionDataMock = jest.fn(); + + parseRequiredTokensMock.mockReturnValue([TRANSCTION_TOKEN_REQUIRED_MOCK]); + isolatedGetTransactionControllerStateMock.mockReturnValue({ + transactions: [TRANSACTION_META_MOCK], + } as TransactionControllerState); + + subscribeAssetChanges( + isolatedMessenger, + () => buildState({ tokens: [] }), + updateTransactionDataMock, + ); + + isolatedPublish('TokensController:stateChange', {} as never, []); + + expect(updateTransactionDataMock).toHaveBeenCalledTimes(1); + }); + + it('subscribes to AssetsController state changes when unify-state feature is enabled', () => { + const updateTransactionDataMock = jest.fn(); + + getAssetsUnifyStateFeatureMock.mockReturnValue(true); + parseRequiredTokensMock.mockReturnValue([TRANSCTION_TOKEN_REQUIRED_MOCK]); + isolatedGetTransactionControllerStateMock.mockReturnValue({ + transactions: [TRANSACTION_META_MOCK], + } as TransactionControllerState); + + subscribeAssetChanges( + isolatedMessenger, + () => buildState({ tokens: [] }), + updateTransactionDataMock, + ); + + isolatedPublish('AssetsController:stateChange', {} as never, []); + + expect(updateTransactionDataMock).toHaveBeenCalledTimes(1); + }); + + it('does not subscribe to per-source events when unify-state feature is enabled', () => { + const updateTransactionDataMock = jest.fn(); + + getAssetsUnifyStateFeatureMock.mockReturnValue(true); + isolatedGetTransactionControllerStateMock.mockReturnValue({ + transactions: [TRANSACTION_META_MOCK], + } as TransactionControllerState); + + subscribeAssetChanges( + isolatedMessenger, + () => buildState({ tokens: [] }), + updateTransactionDataMock, + ); + + isolatedPublish('TokensController:stateChange', {} as never, []); + isolatedPublish('TokenRatesController:stateChange', {} as never, []); + isolatedPublish('CurrencyRateController:stateChange', {} as never, []); + + expect(updateTransactionDataMock).not.toHaveBeenCalled(); + }); + + it('skips transactions whose tokens are already populated', () => { + const updateTransactionDataMock = jest.fn(); + + isolatedGetTransactionControllerStateMock.mockReturnValue({ + transactions: [TRANSACTION_META_MOCK], + } as TransactionControllerState); + + subscribeAssetChanges( + isolatedMessenger, + () => + buildState({ + tokens: [TRANSCTION_TOKEN_REQUIRED_MOCK], + }), + updateTransactionDataMock, + ); + + isolatedPublish('TokenRatesController:stateChange', {} as never, []); + + expect(updateTransactionDataMock).not.toHaveBeenCalled(); + expect(parseRequiredTokensMock).not.toHaveBeenCalled(); + }); + + it.each(FINALIZED_STATUSES)( + 'skips transactions whose status is %s', + (status) => { + const updateTransactionDataMock = jest.fn(); + + isolatedGetTransactionControllerStateMock.mockReturnValue({ + transactions: [{ ...TRANSACTION_META_MOCK, status }], + } as TransactionControllerState); + + subscribeAssetChanges( + isolatedMessenger, + () => buildState({ tokens: [] }), + updateTransactionDataMock, + ); + + isolatedPublish('TokenRatesController:stateChange', {} as never, []); + + expect(updateTransactionDataMock).not.toHaveBeenCalled(); + }, + ); + + it('skips transactions that no longer exist in TransactionController state', () => { + const updateTransactionDataMock = jest.fn(); + + isolatedGetTransactionControllerStateMock.mockReturnValue({ + transactions: [] as TransactionMeta[], + } as TransactionControllerState); + + subscribeAssetChanges( + isolatedMessenger, + () => buildState({ tokens: [] }), + updateTransactionDataMock, + ); + + isolatedPublish('TokenRatesController:stateChange', {} as never, []); + + expect(updateTransactionDataMock).not.toHaveBeenCalled(); + }); + }); + describe('updateTransaction', () => { it('updates transaction', () => { getTransactionControllerStateMock.mockReturnValue({ diff --git a/packages/transaction-pay-controller/src/utils/transaction.ts b/packages/transaction-pay-controller/src/utils/transaction.ts index ee75e3a182..c97d4004ec 100644 --- a/packages/transaction-pay-controller/src/utils/transaction.ts +++ b/packages/transaction-pay-controller/src/utils/transaction.ts @@ -5,13 +5,16 @@ import { import type { TransactionMeta } from '@metamask/transaction-controller'; import type { Hex } from '@metamask/utils'; import { createModuleLogger } from '@metamask/utils'; +import type { Patch } from 'immer'; import { cloneDeep } from 'lodash'; import { projectLogger } from '../logger'; import type { TransactionPayControllerMessenger, + TransactionPayControllerState, UpdateTransactionDataCallback, } from '../types'; +import { getAssetsUnifyStateFeature } from './feature-flags'; import { parseRequiredTokens } from './required-tokens'; const log = createModuleLogger(projectLogger, 'transaction'); @@ -43,13 +46,13 @@ export function getTransaction( } /** - * Poll for transaction changes and update the transaction data accordingly. + * Subscribe to transaction changes and update the transaction data accordingly. * * @param messenger - Controller messenger. * @param updateTransactionData - Callback to update transaction data. * @param removeTransactionData - Callback to remove transaction data. */ -export function pollTransactionChanges( +export function subscribeTransactionChanges( messenger: TransactionPayControllerMessenger, updateTransactionData: UpdateTransactionDataCallback, removeTransactionData: (transactionId: string) => void, @@ -103,6 +106,63 @@ export function pollTransactionChanges( ); } +/** + * Subscribe to asset state changes and re-parse required tokens for + * in-flight transactions whose tokens have not yet resolved. + * + * @param messenger - Controller messenger. + * @param getControllerState - Callback returning the current controller state. + * @param updateTransactionData - Callback to update transaction data. + */ +export function subscribeAssetChanges( + messenger: TransactionPayControllerMessenger, + getControllerState: () => TransactionPayControllerState, + updateTransactionData: UpdateTransactionDataCallback, +): void { + const buildHandler = + (source: string) => + (_state: unknown, patches: Patch[] | undefined): void => { + const { transactionData } = getControllerState(); + + for (const [transactionId, data] of Object.entries(transactionData)) { + if (data.tokens.length > 0) { + continue; + } + + const transaction = getTransaction(transactionId, messenger); + + if (!transaction || FINALIZED_STATUSES.includes(transaction.status)) { + continue; + } + + log('Asset data changed', { transactionId, source, patches }); + + onTransactionChange(transaction, messenger, updateTransactionData); + } + }; + + if (getAssetsUnifyStateFeature(messenger)) { + messenger.subscribe( + 'AssetsController:stateChange', + buildHandler('AssetsController'), + ); + return; + } + + messenger.subscribe( + 'TokensController:stateChange', + buildHandler('TokensController'), + ); + messenger.subscribe( + 'TokenRatesController:stateChange', + buildHandler('TokenRatesController'), + ); + messenger.subscribe( + 'CurrencyRateController:stateChange', + buildHandler('CurrencyRateController'), + ); +} + /** * Wait for a transaction to be confirmed or fail. *