Skip to content

Commit

Permalink
fix: incorrect token balance changes for simulations of multiple toke…
Browse files Browse the repository at this point in the history 
…ns that include an NFT mint (#4290)

## Explanation
When simulating a transaction resulting in an NFT mint, we skip the
prior owner check for the NFT since the token does not exist yet.
However, for subsequent tokens, we were using the incorrect index for
the prior balance transaction.

The fix is to keep a separate prevBalanceIndex, which we only increment
if a prior balance transaction was included in the simulation.

## References

* Related to MetaMask/metamask-extension#24586

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/transaction-controller`

- **FIXED**: Incorrect token balance changes for simulations of multiple
tokens that include an NFT mint.

## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

Co-authored-by: legobeat <[email protected]>
  • Loading branch information
@dbrans @legobeat
dbrans and legobeat authored May 21, 2024
1 parent 3be5fcc commit 06b8c5f
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 30 deletions.
97 changes: 68 additions & 29 deletions packages/transaction-controller/src/utils/simulation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ jest.mock('./simulation-api');

const USER_ADDRESS_MOCK = '0x123';
const OTHER_ADDRESS_MOCK = '0x456';
const CONTRACT_ADDRESS_MOCK = '0x789';
const CONTRACT_ADDRESS_1_MOCK = '0x789';
const CONTRACT_ADDRESS_2_MOCK = '0xDEF';
const BALANCE_1_MOCK = '0x0';
const BALANCE_2_MOCK = '0x1';
const DIFFERENCE_MOCK = '0x1';
Expand All @@ -38,23 +39,23 @@ const REQUEST_MOCK: GetSimulationDataRequest = {

const PARSED_ERC20_TRANSFER_EVENT_MOCK = {
name: 'Transfer',
contractAddress: CONTRACT_ADDRESS_MOCK,
contractAddress: CONTRACT_ADDRESS_2_MOCK,
args: [
OTHER_ADDRESS_MOCK,
USER_ADDRESS_MOCK,
OTHER_ADDRESS_MOCK,
{ toHexString: () => VALUE_MOCK },
],
} as unknown as LogDescription;

const PARSED_ERC721_TRANSFER_EVENT_MOCK = {
name: 'Transfer',
contractAddress: CONTRACT_ADDRESS_MOCK,
contractAddress: CONTRACT_ADDRESS_1_MOCK,
args: [OTHER_ADDRESS_MOCK, USER_ADDRESS_MOCK, TOKEN_ID_MOCK],
} as unknown as LogDescription;

const PARSED_ERC1155_TRANSFER_SINGLE_EVENT_MOCK = {
name: 'TransferSingle',
contractAddress: CONTRACT_ADDRESS_MOCK,
contractAddress: CONTRACT_ADDRESS_1_MOCK,
args: [
OTHER_ADDRESS_MOCK,
OTHER_ADDRESS_MOCK,
Expand All @@ -66,7 +67,7 @@ const PARSED_ERC1155_TRANSFER_SINGLE_EVENT_MOCK = {

const PARSED_ERC1155_TRANSFER_BATCH_EVENT_MOCK = {
name: 'TransferBatch',
contractAddress: CONTRACT_ADDRESS_MOCK,
contractAddress: CONTRACT_ADDRESS_1_MOCK,
args: [
OTHER_ADDRESS_MOCK,
OTHER_ADDRESS_MOCK,
Expand All @@ -78,7 +79,7 @@ const PARSED_ERC1155_TRANSFER_BATCH_EVENT_MOCK = {

const PARSED_WRAPPED_ERC20_DEPOSIT_EVENT_MOCK = {
name: 'Deposit',
contractAddress: CONTRACT_ADDRESS_MOCK,
contractAddress: CONTRACT_ADDRESS_1_MOCK,
args: [USER_ADDRESS_MOCK, { toHexString: () => VALUE_MOCK }],
} as unknown as LogDescription;

Expand All @@ -92,7 +93,7 @@ const RESPONSE_NESTED_LOGS_MOCK: SimulationResponse = {
calls: [
{
calls: [],
logs: [createLogMock(CONTRACT_ADDRESS_MOCK)],
logs: [createLogMock(CONTRACT_ADDRESS_1_MOCK)],
},
],
logs: [],
Expand Down Expand Up @@ -200,7 +201,7 @@ function createBalanceOfResponse(
},
})),
{
return: '0x',
return: '0xabc',
callTrace: {
calls: [],
logs: [],
Expand Down Expand Up @@ -379,7 +380,7 @@ describe('Simulation Utils', () => {

simulateTransactionsMock
.mockResolvedValueOnce(
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_MOCK)]),
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_1_MOCK)]),
)
.mockResolvedValueOnce(
createBalanceOfResponse(previousBalances, newBalances),
Expand All @@ -392,7 +393,7 @@ describe('Simulation Utils', () => {
tokenBalanceChanges: [
{
standard: tokenStandard,
address: CONTRACT_ADDRESS_MOCK,
address: CONTRACT_ADDRESS_1_MOCK,
id: tokenId,
previousBalance: BALANCE_1_MOCK,
newBalance: BALANCE_2_MOCK,
Expand Down Expand Up @@ -480,8 +481,8 @@ describe('Simulation Utils', () => {
simulateTransactionsMock
.mockResolvedValueOnce(
createEventResponseMock([
createLogMock(CONTRACT_ADDRESS_MOCK),
createLogMock(CONTRACT_ADDRESS_MOCK),
createLogMock(CONTRACT_ADDRESS_1_MOCK),
createLogMock(CONTRACT_ADDRESS_1_MOCK),
]),
)
.mockResolvedValueOnce(
Expand All @@ -495,7 +496,7 @@ describe('Simulation Utils', () => {
tokenBalanceChanges: [
{
standard: SimulationTokenStandard.erc20,
address: CONTRACT_ADDRESS_MOCK,
address: CONTRACT_ADDRESS_1_MOCK,
id: undefined,
previousBalance: BALANCE_2_MOCK,
newBalance: BALANCE_1_MOCK,
Expand All @@ -521,8 +522,8 @@ describe('Simulation Utils', () => {
simulateTransactionsMock
.mockResolvedValueOnce(
createEventResponseMock([
createLogMock(CONTRACT_ADDRESS_MOCK),
createLogMock(CONTRACT_ADDRESS_MOCK),
createLogMock(CONTRACT_ADDRESS_1_MOCK),
createLogMock(CONTRACT_ADDRESS_1_MOCK),
]),
)
.mockResolvedValueOnce(
Expand All @@ -539,7 +540,7 @@ describe('Simulation Utils', () => {
tokenBalanceChanges: [
{
standard: SimulationTokenStandard.erc721,
address: CONTRACT_ADDRESS_MOCK,
address: CONTRACT_ADDRESS_1_MOCK,
id: TOKEN_ID_MOCK,
previousBalance: BALANCE_1_MOCK,
newBalance: BALANCE_2_MOCK,
Expand All @@ -548,7 +549,7 @@ describe('Simulation Utils', () => {
},
{
standard: SimulationTokenStandard.erc721,
address: CONTRACT_ADDRESS_MOCK,
address: CONTRACT_ADDRESS_1_MOCK,
id: OTHER_TOKEN_ID_MOCK,
previousBalance: BALANCE_1_MOCK,
newBalance: BALANCE_2_MOCK,
Expand All @@ -571,28 +572,57 @@ describe('Simulation Utils', () => {
},
});

// Pay for NFT mint with ERC20 token.
mockParseLog({
erc20: PARSED_ERC20_TRANSFER_EVENT_MOCK,
});

simulateTransactionsMock
.mockResolvedValueOnce(
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_MOCK)]),
createEventResponseMock([
createLogMock(CONTRACT_ADDRESS_1_MOCK),
createLogMock(CONTRACT_ADDRESS_2_MOCK),
]),
)
.mockResolvedValueOnce(
createBalanceOfResponse([], [USER_ADDRESS_MOCK]),
createBalanceOfResponse(
[BALANCE_2_MOCK], // The ERC20 balance before minting.
[
USER_ADDRESS_MOCK, // The user is the owner.
BALANCE_1_MOCK, // The ERC20 balance after minting.
],
),
);

const simulationData = await getSimulationData(REQUEST_MOCK);

expect(simulateTransactionsMock).toHaveBeenCalledTimes(2);
// The second call should only simulate the minting of the NFT and
// check the balance after, and not before.

// The balance of the ERC-20 token is checked before and after the transaction.
// The ERC-721 token balance is only checked after the transaction since it is minted.
expect(simulateTransactionsMock).toHaveBeenNthCalledWith(
2,
REQUEST_MOCK.chainId,
{
transactions: [
// ERC-20 balance before minting.
{
from: REQUEST_MOCK.from,
to: CONTRACT_ADDRESS_2_MOCK,
data: expect.any(String),
},
// Minting ERC-721 token.
REQUEST_MOCK,
// ERC-721 owner after minting.
{
from: REQUEST_MOCK.from,
to: CONTRACT_ADDRESS_MOCK,
to: CONTRACT_ADDRESS_1_MOCK,
data: expect.any(String),
},
// ERC-20 balance before minting.
{
from: REQUEST_MOCK.from,
to: CONTRACT_ADDRESS_2_MOCK,
data: expect.any(String),
},
],
Expand All @@ -603,13 +633,22 @@ describe('Simulation Utils', () => {
tokenBalanceChanges: [
{
standard: SimulationTokenStandard.erc721,
address: CONTRACT_ADDRESS_MOCK,
address: CONTRACT_ADDRESS_1_MOCK,
id: TOKEN_ID_MOCK,
previousBalance: '0x0',
newBalance: '0x1',
difference: '0x1',
isDecrease: false,
},
{
standard: SimulationTokenStandard.erc20,
address: CONTRACT_ADDRESS_2_MOCK,
id: undefined,
previousBalance: '0x1',
newBalance: '0x0',
difference: '0x1',
isDecrease: true,
},
],
});
});
Expand All @@ -619,7 +658,7 @@ describe('Simulation Utils', () => {

simulateTransactionsMock
.mockResolvedValueOnce(
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_MOCK)]),
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_1_MOCK)]),
)
.mockResolvedValueOnce(
createBalanceOfResponse([BALANCE_1_MOCK], [BALANCE_2_MOCK]),
Expand All @@ -646,7 +685,7 @@ describe('Simulation Utils', () => {
});

simulateTransactionsMock.mockResolvedValueOnce(
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_MOCK)]),
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_1_MOCK)]),
);

const simulationData = await getSimulationData(REQUEST_MOCK);
Expand All @@ -664,7 +703,7 @@ describe('Simulation Utils', () => {

simulateTransactionsMock
.mockResolvedValueOnce(
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_MOCK)]),
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_1_MOCK)]),
)
.mockResolvedValueOnce(
createBalanceOfResponse([BALANCE_1_MOCK], [BALANCE_1_MOCK]),
Expand Down Expand Up @@ -694,7 +733,7 @@ describe('Simulation Utils', () => {
tokenBalanceChanges: [
{
standard: SimulationTokenStandard.erc20,
address: CONTRACT_ADDRESS_MOCK,
address: CONTRACT_ADDRESS_1_MOCK,
id: undefined,
previousBalance: BALANCE_1_MOCK,
newBalance: BALANCE_2_MOCK,
Expand Down Expand Up @@ -741,7 +780,7 @@ describe('Simulation Utils', () => {

simulateTransactionsMock
.mockResolvedValueOnce(
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_MOCK)]),
createEventResponseMock([createLogMock(CONTRACT_ADDRESS_1_MOCK)]),
)
.mockResolvedValueOnce(createBalanceOfResponse([], []));

Expand Down
4 changes: 3 additions & 1 deletion packages/transaction-controller/src/utils/simulation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ async function getTokenBalanceChanges(
throw new SimulationInvalidResponseError();
}

let prevBalanceTxIndex = 0;
return [...balanceTxs.after.keys()]
.map((token, index) => {
const previousBalanceCheckSkipped = !balanceTxs.before.get(token);
Expand All @@ -318,7 +319,8 @@ async function getTokenBalanceChanges(
: getValueFromBalanceTransaction(
request.from,
token,
response.transactions[index],
// eslint-disable-next-line no-plusplus
response.transactions[prevBalanceTxIndex++],
);

const newBalance = getValueFromBalanceTransaction(
Expand Down

0 comments on commit 06b8c5f

Please sign in to comment.