diff --git a/packages/transaction-controller/jest.config.js b/packages/transaction-controller/jest.config.js index 80740de238..649d199ade 100644 --- a/packages/transaction-controller/jest.config.js +++ b/packages/transaction-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 94.02, - functions: 97.49, - lines: 98.36, - statements: 98.37, + branches: 93.87, + functions: 97.51, + lines: 98.38, + statements: 98.39, }, }, diff --git a/packages/transaction-controller/package.json b/packages/transaction-controller/package.json index c0692bab1a..3915270d4e 100644 --- a/packages/transaction-controller/package.json +++ b/packages/transaction-controller/package.json @@ -61,7 +61,6 @@ "@metamask/rpc-errors": "^7.0.0", "@metamask/utils": "^9.1.0", "async-mutex": "^0.5.0", - "bignumber.js": "^9.1.2", "bn.js": "^5.2.1", "eth-method-registry": "^4.0.0", "fast-json-patch": "^3.1.1", diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 4496a35f01..b43fd4457f 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -49,7 +49,7 @@ import { add0x } from '@metamask/utils'; import { Mutex } from 'async-mutex'; import { MethodRegistry } from 'eth-method-registry'; import { EventEmitter } from 'events'; -import { cloneDeep, mapValues, merge, pickBy, sortBy, isEqual } from 'lodash'; +import { cloneDeep, mapValues, merge, pickBy, sortBy } from 'lodash'; import { v1 as random } from 'uuid'; import { DefaultGasFeeFlow } from './gas-flows/DefaultGasFeeFlow'; @@ -106,7 +106,7 @@ import { getNextNonce, } from './utils/nonce'; import type { ResimulateResponse } from './utils/resimulate'; -import { shouldResimulate } from './utils/resimulate'; +import { hasSimulationDataChanged, shouldResimulate } from './utils/resimulate'; import { getTransactionParamsWithIncreasedGasFee } from './utils/retry'; import { getSimulationData } from './utils/simulation'; import { @@ -3667,7 +3667,7 @@ export class TransactionController extends BaseController< if ( blockTime && prevSimulationData && - !isEqual(simulationData, prevSimulationData) + hasSimulationDataChanged(prevSimulationData, simulationData) ) { simulationData = { ...simulationData, diff --git a/packages/transaction-controller/src/utils/resimulate.test.ts b/packages/transaction-controller/src/utils/resimulate.test.ts index 26edc4f5c6..00aad5b94e 100644 --- a/packages/transaction-controller/src/utils/resimulate.test.ts +++ b/packages/transaction-controller/src/utils/resimulate.test.ts @@ -1,19 +1,23 @@ /* eslint-disable @typescript-eslint/naming-convention */ +import { BN } from 'bn.js'; + import { CHAIN_IDS } from '../constants'; import type { SecurityAlertResponse, SimulationData, + SimulationTokenBalanceChange, TransactionMeta, } from '../types'; -import { TransactionStatus } from '../types'; +import { SimulationTokenStandard, TransactionStatus } from '../types'; import { BLOCK_TIME_ADDITIONAL_SECONDS, BLOCKAID_RESULT_TYPE_MALICIOUS, + hasSimulationDataChanged, RESIMULATE_PARAMS, shouldResimulate, - VALUE_NATIVE_BALANCE_PERCENT_THRESHOLD, + VALUE_COMPARISON_PERCENT_THRESHOLD, } from './resimulate'; -import { isPercentageDifferenceWithinThreshold } from './utils'; +import { getPercentageChange } from './utils'; jest.mock('./utils'); @@ -25,6 +29,15 @@ const SECURITY_ALERT_RESPONSE_MOCK: SecurityAlertResponse = { result_type: 'TestResultType', }; +const TOKEN_BALANCE_CHANGE_MOCK: SimulationTokenBalanceChange = { + address: '0x1', + standard: SimulationTokenStandard.erc20, + difference: '0x1', + previousBalance: '0x1', + newBalance: '0x2', + isDecrease: false, +}; + const SIMULATION_DATA_MOCK: SimulationData = { nativeBalanceChange: { difference: '0x1', @@ -35,6 +48,16 @@ const SIMULATION_DATA_MOCK: SimulationData = { tokenBalanceChanges: [], }; +const SIMULATION_DATA_2_MOCK: SimulationData = { + nativeBalanceChange: { + difference: '0x1', + previousBalance: '0x2', + newBalance: '0x3', + isDecrease: false, + }, + tokenBalanceChanges: [], +}; + const TRANSACTION_META_MOCK: TransactionMeta = { chainId: CHAIN_IDS.MAINNET, id: '123-456', @@ -50,13 +73,13 @@ const TRANSACTION_META_MOCK: TransactionMeta = { }; describe('Resimulate Utils', () => { - const isPercentageDifferenceWithinThresholdMock = jest.mocked( - isPercentageDifferenceWithinThreshold, - ); + const getPercentageChangeMock = jest.mocked(getPercentageChange); beforeEach(() => { jest.resetAllMocks(); jest.spyOn(Date, 'now').mockReturnValue(CURRENT_TIME_MOCK); + + getPercentageChangeMock.mockReturnValue(0); }); describe('shouldResimulate', () => { @@ -149,7 +172,9 @@ describe('Resimulate Utils', () => { describe('Value & Native Balance', () => { it('resimulates if value does not match native balance difference from simulation', () => { - isPercentageDifferenceWithinThresholdMock.mockReturnValue(false); + getPercentageChangeMock.mockReturnValueOnce( + VALUE_COMPARISON_PERCENT_THRESHOLD + 1, + ); const result = shouldResimulate(TRANSACTION_META_MOCK, { ...TRANSACTION_META_MOCK, @@ -160,7 +185,9 @@ describe('Resimulate Utils', () => { }); it('includes block time if value does not match native balance difference from simulation', () => { - isPercentageDifferenceWithinThresholdMock.mockReturnValue(false); + getPercentageChangeMock.mockReturnValueOnce( + VALUE_COMPARISON_PERCENT_THRESHOLD + 1, + ); const result = shouldResimulate(TRANSACTION_META_MOCK, { ...TRANSACTION_META_MOCK, @@ -173,7 +200,7 @@ describe('Resimulate Utils', () => { }); it('does not resimulate if simulation data changed but value and native balance match', () => { - isPercentageDifferenceWithinThresholdMock.mockReturnValue(true); + getPercentageChangeMock.mockReturnValueOnce(0); const result = shouldResimulate(TRANSACTION_META_MOCK, { ...TRANSACTION_META_MOCK, @@ -187,8 +214,6 @@ describe('Resimulate Utils', () => { }); it('does not resimulate if simulation data changed but value and native balance not specified', () => { - isPercentageDifferenceWithinThresholdMock.mockReturnValue(true); - const result = shouldResimulate( { ...TRANSACTION_META_MOCK, @@ -210,13 +235,10 @@ describe('Resimulate Utils', () => { }, ); - expect(isPercentageDifferenceWithinThresholdMock).toHaveBeenCalledTimes( - 1, - ); - expect(isPercentageDifferenceWithinThresholdMock).toHaveBeenCalledWith( - '0x0', - '0x0', - VALUE_NATIVE_BALANCE_PERCENT_THRESHOLD, + expect(getPercentageChangeMock).toHaveBeenCalledTimes(1); + expect(getPercentageChangeMock).toHaveBeenCalledWith( + new BN(0), + new BN(0), ); expect(result).toStrictEqual({ @@ -226,4 +248,100 @@ describe('Resimulate Utils', () => { }); }); }); + + describe('hasSimulationDataChanged', () => { + it('returns false if simulation data unchanged', () => { + const result = hasSimulationDataChanged( + SIMULATION_DATA_MOCK, + SIMULATION_DATA_MOCK, + ); + + expect(result).toBe(false); + }); + + it('returns true if native balance changed', () => { + getPercentageChangeMock.mockReturnValueOnce( + VALUE_COMPARISON_PERCENT_THRESHOLD + 1, + ); + + const result = hasSimulationDataChanged( + SIMULATION_DATA_MOCK, + SIMULATION_DATA_2_MOCK, + ); + + expect(result).toBe(true); + }); + + it('returns true if token balance count does not match', () => { + getPercentageChangeMock.mockReturnValueOnce(0); + + const result = hasSimulationDataChanged(SIMULATION_DATA_MOCK, { + ...SIMULATION_DATA_MOCK, + tokenBalanceChanges: [TOKEN_BALANCE_CHANGE_MOCK], + }); + + expect(result).toBe(true); + }); + + it('returns true if token balance does not match', () => { + getPercentageChangeMock + .mockReturnValueOnce(0) + .mockReturnValueOnce(VALUE_COMPARISON_PERCENT_THRESHOLD + 1); + + const result = hasSimulationDataChanged( + { + ...SIMULATION_DATA_MOCK, + tokenBalanceChanges: [TOKEN_BALANCE_CHANGE_MOCK], + }, + { + ...SIMULATION_DATA_MOCK, + tokenBalanceChanges: [ + { ...TOKEN_BALANCE_CHANGE_MOCK, difference: '0x2' }, + ], + }, + ); + + expect(result).toBe(true); + }); + + it('returns false if token balance changed but within threshold', () => { + getPercentageChangeMock + .mockReturnValueOnce(0) + .mockReturnValueOnce(VALUE_COMPARISON_PERCENT_THRESHOLD); + + const result = hasSimulationDataChanged( + { + ...SIMULATION_DATA_MOCK, + tokenBalanceChanges: [TOKEN_BALANCE_CHANGE_MOCK], + }, + { + ...SIMULATION_DATA_MOCK, + tokenBalanceChanges: [ + { ...TOKEN_BALANCE_CHANGE_MOCK, difference: '0x2' }, + ], + }, + ); + + expect(result).toBe(false); + }); + + it('returns true if new token balance not found', () => { + getPercentageChangeMock.mockReturnValueOnce(0).mockReturnValueOnce(0); + + const result = hasSimulationDataChanged( + { + ...SIMULATION_DATA_MOCK, + tokenBalanceChanges: [TOKEN_BALANCE_CHANGE_MOCK], + }, + { + ...SIMULATION_DATA_MOCK, + tokenBalanceChanges: [ + { ...TOKEN_BALANCE_CHANGE_MOCK, address: '0x2' }, + ], + }, + ); + + expect(result).toBe(true); + }); + }); }); diff --git a/packages/transaction-controller/src/utils/resimulate.ts b/packages/transaction-controller/src/utils/resimulate.ts index 05ae69b209..b339356c7a 100644 --- a/packages/transaction-controller/src/utils/resimulate.ts +++ b/packages/transaction-controller/src/utils/resimulate.ts @@ -1,16 +1,22 @@ import type { Hex } from '@metamask/utils'; -import { createModuleLogger } from '@metamask/utils'; +import { createModuleLogger, remove0x } from '@metamask/utils'; +import { BN } from 'bn.js'; import { isEqual } from 'lodash'; import { projectLogger } from '../logger'; -import type { TransactionMeta, TransactionParams } from '../types'; -import { isPercentageDifferenceWithinThreshold } from './utils'; +import type { + SimulationBalanceChange, + SimulationData, + TransactionMeta, + TransactionParams, +} from '../types'; +import { getPercentageChange } from './utils'; const log = createModuleLogger(projectLogger, 'resimulate'); export const RESIMULATE_PARAMS = ['to', 'value', 'data'] as const; export const BLOCKAID_RESULT_TYPE_MALICIOUS = 'Malicious'; -export const VALUE_NATIVE_BALANCE_PERCENT_THRESHOLD = 5; +export const VALUE_COMPARISON_PERCENT_THRESHOLD = 5; export const BLOCK_TIME_ADDITIONAL_SECONDS = 60; export type ResimulateResponse = { @@ -71,6 +77,68 @@ export function shouldResimulate( }; } +/** + * Determine if the simulation data has changed. + * @param originalSimulationData - The original simulation data. + * @param newSimulationData - The new simulation data. + * @returns Whether the simulation data has changed. + */ +export function hasSimulationDataChanged( + originalSimulationData: SimulationData, + newSimulationData: SimulationData, +): boolean { + if (isEqual(originalSimulationData, newSimulationData)) { + return false; + } + + if ( + isBalanceChangeUpdated( + originalSimulationData?.nativeBalanceChange, + newSimulationData?.nativeBalanceChange, + ) + ) { + log('Simulation data native balance changed'); + return true; + } + + if ( + originalSimulationData.tokenBalanceChanges.length !== + newSimulationData.tokenBalanceChanges.length + ) { + return true; + } + + for (const originalTokenBalanceChange of originalSimulationData.tokenBalanceChanges) { + const newTokenBalanceChange = newSimulationData.tokenBalanceChanges.find( + ({ address, id }) => + address === originalTokenBalanceChange.address && + id === originalTokenBalanceChange.id, + ); + + if (!newTokenBalanceChange) { + log('Missing new token balance', { + address: originalTokenBalanceChange.address, + id: originalTokenBalanceChange.id, + }); + + return true; + } + + if ( + isBalanceChangeUpdated(originalTokenBalanceChange, newTokenBalanceChange) + ) { + log('Simulation data token balance changed', { + originalTokenBalanceChange, + newTokenBalanceChange, + }); + + return true; + } + } + + return false; +} + /** * Determine if the transaction parameters have been updated. * @param originalTransactionMeta - The original transaction metadata. @@ -162,9 +230,59 @@ function hasValueAndNativeBalanceMismatch( const newNativeBalanceDifference = newSimulationData?.nativeBalanceChange?.difference ?? '0x0'; - return !isPercentageDifferenceWithinThreshold( - newNativeBalanceDifference, + return !percentageChangeWithinThreshold( newValue as Hex, - VALUE_NATIVE_BALANCE_PERCENT_THRESHOLD, + newNativeBalanceDifference, + false, + newSimulationData?.nativeBalanceChange?.isDecrease === false, + ); +} + +/** + * Determine if a balance change has been updated. + * @param originalBalanceChange - The original balance change. + * @param newBalanceChange - The new balance change. + * @returns Whether the balance change has been updated. + */ +function isBalanceChangeUpdated( + originalBalanceChange?: SimulationBalanceChange, + newBalanceChange?: SimulationBalanceChange, +): boolean { + return !percentageChangeWithinThreshold( + originalBalanceChange?.difference ?? '0x0', + newBalanceChange?.difference ?? '0x0', + originalBalanceChange?.isDecrease === false, + newBalanceChange?.isDecrease === false, + ); +} + +/** + * Determine if the percentage change between two values is within a threshold. + * @param originalValue - The original value. + * @param newValue - The new value. + * @param originalNegative - Whether the original value is negative. + * @param newNegative - Whether the new value is negative. + * @returns Whether the percentage change between the two values is within a threshold. + */ +function percentageChangeWithinThreshold( + originalValue: Hex, + newValue: Hex, + originalNegative?: boolean, + newNegative?: boolean, +): boolean { + let originalValueBN = new BN(remove0x(originalValue), 'hex'); + let newValueBN = new BN(remove0x(newValue), 'hex'); + + if (originalNegative) { + originalValueBN = originalValueBN.neg(); + } + + if (newNegative) { + newValueBN = newValueBN.neg(); + } + + return ( + getPercentageChange(originalValueBN, newValueBN) <= + VALUE_COMPARISON_PERCENT_THRESHOLD ); } diff --git a/packages/transaction-controller/src/utils/utils.test.ts b/packages/transaction-controller/src/utils/utils.test.ts index 21ecf4c502..a49b883dec 100644 --- a/packages/transaction-controller/src/utils/utils.test.ts +++ b/packages/transaction-controller/src/utils/utils.test.ts @@ -1,3 +1,5 @@ +import { BN } from 'bn.js'; + import type { FeeMarketEIP1559Values, GasPriceValue, @@ -215,85 +217,46 @@ describe('utils', () => { }); }); - describe('isPercentageDifferenceWithinThreshold', () => { - it('should return true when both values are zero', () => { - const value1 = '0x0'; - const value2 = '0x0'; - const threshold = 10; - expect( - util.isPercentageDifferenceWithinThreshold(value1, value2, threshold), - ).toBe(true); + describe('getPercentageChange', () => { + it('supports original and new value as zero', () => { + expect(util.getPercentageChange(new BN(0), new BN(0))).toBe(0); }); - it('should return false when one value is zero and the other is not', () => { - const value1 = '0x0'; - const value2 = '0x1'; - const threshold = 10; - expect( - util.isPercentageDifferenceWithinThreshold(value1, value2, threshold), - ).toBe(false); + it('supports original value as zero and new value not', () => { + expect(util.getPercentageChange(new BN(0), new BN(1))).toBe(100); }); - it('should return true when percentage difference is less than threshold', () => { - const value1 = '0xa'; // 10 in decimal - const value2 = '0xb'; // 11 in decimal - const threshold = 15; // Difference is ~9.09% - expect( - util.isPercentageDifferenceWithinThreshold(value1, value2, threshold), - ).toBe(true); + it('supports new value greater than original value', () => { + expect(util.getPercentageChange(new BN(10), new BN(11))).toBe(10); }); - it('should return true when percentage difference is exactly the threshold', () => { - const value1 = '0xa'; // 10 - const value2 = '0xc'; // 12 - const threshold = 20; // Difference is 20% - expect( - util.isPercentageDifferenceWithinThreshold(value1, value2, threshold), - ).toBe(true); + it('supports new value less than original value', () => { + expect(util.getPercentageChange(new BN(11), new BN(10))).toBe(9); }); - it('should return false when percentage difference is greater than threshold', () => { - const value1 = '0xa'; // 10 - const value2 = '0x20'; // 32 - const threshold = 20; // Difference is 120% + it('supports large numbers', () => { expect( - util.isPercentageDifferenceWithinThreshold(value1, value2, threshold), - ).toBe(false); + util.getPercentageChange( + new BN( + '100000000000000000000000000000000000000000000000000000000000000000000000000000000', + ), + new BN( + '200000000000000000000000000000000000000000000000000000000000000000000000000000000', + ), + ), + ).toBe(100); }); - it('should handle large numbers correctly', () => { - const value1 = - '0x1fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'; // Large number - const value2 = - '0x2fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'; // Approximately 100% difference - const threshold = 100; - expect( - util.isPercentageDifferenceWithinThreshold(value1, value2, threshold), - ).toBe(true); + it('supports identical original and new value', () => { + expect(util.getPercentageChange(new BN(1), new BN(1))).toBe(0); }); - it('should return false when average is zero but values are not both zero', () => { - const value1 = '0x0'; - const value2 = '0x1'; - const threshold = 0; - expect( - util.isPercentageDifferenceWithinThreshold(value1, value2, threshold), - ).toBe(false); + it('supports negative original value', () => { + expect(util.getPercentageChange(new BN(-1), new BN(2))).toBe(300); }); - it('should handle threshold of 0 correctly', () => { - const value1 = '0xa'; // 10 - const value2 = '0xa'; // 10 - const threshold = 0; - expect( - util.isPercentageDifferenceWithinThreshold(value1, value2, threshold), - ).toBe(true); - - const value3 = '0xa'; // 10 - const value4 = '0xb'; // 11 - expect( - util.isPercentageDifferenceWithinThreshold(value3, value4, threshold), - ).toBe(false); + it('supports negative new value', () => { + expect(util.getPercentageChange(new BN(2), new BN(-1))).toBe(150); }); }); }); diff --git a/packages/transaction-controller/src/utils/utils.ts b/packages/transaction-controller/src/utils/utils.ts index 32ca8df753..360f5260dc 100644 --- a/packages/transaction-controller/src/utils/utils.ts +++ b/packages/transaction-controller/src/utils/utils.ts @@ -3,8 +3,8 @@ import { getKnownPropertyNames, isStrictHexString, } from '@metamask/utils'; -import type { Hex, Json } from '@metamask/utils'; -import BigNumber from 'bignumber.js'; +import type { Json } from '@metamask/utils'; +import BN from 'bn.js'; import { TransactionStatus } from '../types'; import type { @@ -186,29 +186,27 @@ export function padHexToEvenLength(hex: string) { } /** - * Calculate the percentage difference between two hex values and determine if it is within a threshold. + * Calculate the absolute percentage change between two values. * - * @param value1 - The first hex value. - * @param value2 - The second hex value. - * @param threshold - The percentage threshold for considering the values equal. - * @returns Whether the percentage difference is within the threshold. + * @param originalValue - The first value. + * @param newValue - The second value. + * @returns The percentage change from the first value to the second value. + * If the original value is zero and the new value is not, returns 100. */ -export function isPercentageDifferenceWithinThreshold( - value1: Hex, - value2: Hex, - threshold: number, -): boolean { - const bnValue1 = new BigNumber(value1); - const bnValue2 = new BigNumber(value2); - - if (bnValue1.isZero() && bnValue2.isZero()) { - return true; - } +export function getPercentageChange(originalValue: BN, newValue: BN): number { + const precisionFactor = new BN(10).pow(new BN(18)); + const originalValuePrecision = originalValue.mul(precisionFactor); + const newValuePrecision = newValue.mul(precisionFactor); - const difference = bnValue1.minus(bnValue2).abs(); - const average = bnValue1.plus(bnValue2).dividedBy(2); + const difference = newValuePrecision.sub(originalValuePrecision); - const percentageDifference = difference.dividedBy(average).multipliedBy(100); + if (difference.isZero()) { + return 0; + } + + if (originalValuePrecision.isZero() && !newValuePrecision.isZero()) { + return 100; + } - return percentageDifference.isLessThanOrEqualTo(threshold); + return difference.muln(100).div(originalValuePrecision).abs().toNumber(); } diff --git a/yarn.lock b/yarn.lock index 4482cf79f1..b72af722e3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3704,7 +3704,6 @@ __metadata: "@types/jest": "npm:^27.4.1" "@types/node": "npm:^16.18.54" async-mutex: "npm:^0.5.0" - bignumber.js: "npm:^9.1.2" bn.js: "npm:^5.2.1" deepmerge: "npm:^4.2.2" eth-method-registry: "npm:^4.0.0"