Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
42 changes: 42 additions & 0 deletions packages/shield-controller/src/ShieldController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from '@metamask/transaction-controller';

import { ShieldController } from './ShieldController';
import { TX_META_SIMULATION_DATA_MOCKS } from '../tests/data';
import { createMockBackend, MOCK_COVERAGE_ID } from '../tests/mocks/backend';
import { createMockMessenger } from '../tests/mocks/messenger';
import {
Expand Down Expand Up @@ -169,6 +170,47 @@ describe('ShieldController', () => {
});
});

TX_META_SIMULATION_DATA_MOCKS.forEach(
({ description, previousSimulationData, newSimulationData }) => {
it(`should check coverage when ${description}`, async () => {
const { baseMessenger, backend } = setup();
const previousTxMeta = {
...generateMockTxMeta(),
simulationData: previousSimulationData,
};
const coverageResultReceived =
setupCoverageResultReceived(baseMessenger);

// Add transaction.
baseMessenger.publish(
'TransactionController:stateChange',
{ transactions: [previousTxMeta] } as TransactionControllerState,
undefined as never,
);
expect(await coverageResultReceived).toBeUndefined();
expect(backend.checkCoverage).toHaveBeenCalledWith({
txMeta: previousTxMeta,
});

// Simulate transaction.
const txMeta2 = { ...previousTxMeta };
txMeta2.simulationData = newSimulationData;
const coverageResultReceived2 =
setupCoverageResultReceived(baseMessenger);
baseMessenger.publish(
'TransactionController:stateChange',
{ transactions: [txMeta2] } as TransactionControllerState,
undefined as never,
);
expect(await coverageResultReceived2).toBeUndefined();
expect(backend.checkCoverage).toHaveBeenCalledWith({
coverageId: MOCK_COVERAGE_ID,
txMeta: txMeta2,
});
});
},
);

it('throws an error when the coverage ID has changed', async () => {
const { controller, backend } = setup();
backend.checkCoverage.mockResolvedValueOnce({
Expand Down
63 changes: 62 additions & 1 deletion packages/shield-controller/src/ShieldController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,14 +268,18 @@ export class ShieldController extends BaseController<
);
for (const transaction of transactions) {
const previousTransaction = previousTransactionsById.get(transaction.id);
const simulationDataChanged = this.#compareTransactionSimulationData(
previousTransaction?.simulationData,
transaction.simulationData,
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Transaction Simulation Data Comparison Reversed

The call to #compareTransactionSimulationData passes previousTransaction?.simulationData as the current simulationData and transaction.simulationData as the previous previousSimulationData. This reverses the comparison logic, leading to incorrect change detection for transaction simulation data and potentially triggering unnecessary coverage checks.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot must have had a bad day. That's not the case, as far as I can see.


// Check coverage if the transaction is new or if the simulation data has
// changed.
if (
!previousTransaction ||
// Checking reference equality is sufficient because this object is
// replaced if the simulation data has changed.
previousTransaction.simulationData !== transaction.simulationData
simulationDataChanged
) {
this.checkCoverage(transaction).catch(
// istanbul ignore next
Expand Down Expand Up @@ -443,4 +447,61 @@ export class ShieldController extends BaseController<
#getLatestCoverageId(itemId: string): string | undefined {
return this.state.coverageResults[itemId]?.results[0]?.coverageId;
}

/**
* Compares the simulation data of a transaction.
*
* @param simulationData - The simulation data of the transaction.
* @param previousSimulationData - The previous simulation data of the transaction.
* @returns Whether the simulation data has changed.
*/
#compareTransactionSimulationData(
simulationData?: TransactionMeta['simulationData'],
previousSimulationData?: TransactionMeta['simulationData'],
) {
if (!simulationData && !previousSimulationData) {
return false;
}

// check the simulation error
if (
simulationData?.error?.code !== previousSimulationData?.error?.code ||
simulationData?.error?.message !== previousSimulationData?.error?.message
) {
return true;
}

// check the native balance change
if (
simulationData?.nativeBalanceChange?.difference !==
previousSimulationData?.nativeBalanceChange?.difference ||
simulationData?.nativeBalanceChange?.newBalance !==
previousSimulationData?.nativeBalanceChange?.newBalance ||
simulationData?.nativeBalanceChange?.previousBalance !==
previousSimulationData?.nativeBalanceChange?.previousBalance ||
simulationData?.nativeBalanceChange?.isDecrease !==
previousSimulationData?.nativeBalanceChange?.isDecrease
) {
return true;
}

// check the token balance changes
if (
simulationData?.tokenBalanceChanges.length !==
previousSimulationData?.tokenBalanceChanges.length ||
simulationData?.tokenBalanceChanges.some(
(tokenBalanceChange, index) =>
tokenBalanceChange.difference !==
previousSimulationData?.tokenBalanceChanges[index].difference,
)
) {
return true;
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Undefined Token Balances Cause TypeError

The tokenBalanceChanges comparison in #compareTransactionSimulationData can cause a TypeError. If either simulationData?.tokenBalanceChanges or previousSimulationData?.tokenBalanceChanges is undefined, the code attempts to call .some() on an undefined value. This happens because the length comparison doesn't fully guard against undefined values before the .some() call.

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokenBalanceChanges is a required filed in SimulationData.

// check the isUpdatedAfterSecurityCheck
return (
simulationData?.isUpdatedAfterSecurityCheck !==
previousSimulationData?.isUpdatedAfterSecurityCheck
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Transaction Simulation Data Comparison Fails

The #compareTransactionSimulationData method doesn't correctly detect changes when one simulationData parameter is undefined and the other is defined (e.g., an empty object). This can lead to coverage checks not being triggered when the simulation data has effectively changed.

Fix in Cursor Fix in Web

}
Loading
Loading