Skip to content
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.

Sign up for GitHub

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

fix splitting of transaction nonce groups in state #11103

Merged
merged 4 commits into from
May 17, 2021
Merged

fix splitting of transaction nonce groups in state #11103

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d6c9039
fix splitting of transaction nonce groups in state
brad-decker May 14, 2021
f6f222d
add nonlimit test to ensure no cut off
brad-decker May 17, 2021
8f60118
Update app/scripts/controllers/transactions/tx-state-manager.test.js
brad-decker May 17, 2021
284c1b5
add nonlimit test to ensure no cut off
brad-decker May 17, 2021
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
51 changes: 41 additions & 10 deletions app/scripts/controllers/transactions/tx-state-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,28 +187,43 @@ export default class TransactionStateManager extends EventEmitter {
const transactions = this.getTransactions({
filterToCurrentNetwork: false,
});
const txCount = transactions.length;
const { txHistoryLimit } = this;

// checks if the length of the tx history is longer then desired persistence
// limit and then if it is removes the oldest confirmed or rejected tx.
// Pending or unapproved transactions will not be removed by this
// operation.
// operation. For safety of presenting a fully functional transaction UI
// representation, this function will not break apart transactions with the
// same nonce, per network. Not accounting for transactions of the same
// nonce and network combo can result in confusing or broken experiences
// in the UI.
//
// TODO: we are already limiting what we send to the UI, and in the future
// we will send UI only collected groups of transactions *per page* so at
// some point in the future, this persistence limit can be adjusted. When
// we do that I think we should figure out a better storage solution for
// transaction history entries.
if (txCount > txHistoryLimit - 1) {
const index = transactions.findIndex((metaTx) => {
return getFinalStates().includes(metaTx.status);
});
if (index !== -1) {
this._deleteTransaction(transactions[index].id);
}
}
const nonceNetworkSet = new Set();
Copy link
Contributor

Choose a reason for hiding this comment

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

I want to make sure I understand this.

So the goal is to ensure that if there are two or more transactions with the same nonce in the transaction list, that they don't get deleted.

So we iterate through the transactions. If a transaction is either counted as below txHistoryLimit - 1 or is not in a final state, then it is NOT deleted. If a transaction is counted higher than txHistoryLimit - 1 or is in a final state BUT has the same nonce as a transaction already flagged as one that shouldn't be deleted, then it is NOT deleted. If a transaction is above the txHistoryLimit count or is in a final state and does not match the nonce of a transaction already identified as one that shouldn't be deleted, then it is deleted...

So something about this isn't adding up for me... probably me misunderstanding, but I want to check.

Suppose that there are only 10 transactions and they are all "final states". Wouldn't they all be deleted according to this logic?

Should the if (txCount > txHistoryLimit - 1) { that was removed above wrap this tx deletion code?

And this is in a function that only adds a single tx. Before it only removed a single tx. Why now attempt to remove multiple within this function?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe I forget how filter works...

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe not

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on my reading of the tests I think I did misunderstand the goal though

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the purpose of this code change be stated as:

If the addition of a transaction brings the transaction count above the txHistoryLimit then all transactions that share a nonce with the oldest "finalized" transaction should be deleted.

?

Copy link
Member

Choose a reason for hiding this comment

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

So the goal is to ensure that if there are two or more transactions with the same nonce in the transaction list, that they don't get deleted.

It's to ensure that one transaction in a group isn't deleted without deleting the entire group.

We can't let all groups of more than 1 transaction persist indefinitely - then we'd have no limit on storage. But we can't break groups apart either without breaking the UI.

So the solution we went with here was to interpret the transaction history limit of 50 as meaning 50 groups of transactions. If there are over 50 groups, we delete any groups beyond that 50 (which should always be a single group if this is the only means of adding transactions, but might be more than 1 group if transactions are added elsewhere).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we return false from this method we are Not including it, when we return true we are including it. So when return true we are saying "this is an item we want to delete". if there are only ten transactions non will be deleted because they will never hit the limit and thus will all be added to the nonceSet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically yes as stated that will work

const txsToDelete = transactions
.reverse()
.filter((tx) => {
const { nonce } = tx.txParams;
const { chainId, metamaskNetworkId, status } = tx;
const key = `${nonce}-${chainId ?? metamaskNetworkId}`;
if (nonceNetworkSet.has(key)) {
return false;
} else if (
nonceNetworkSet.size < txHistoryLimit - 1 ||
getFinalStates().includes(status) === false
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
) {
nonceNetworkSet.add(key);
return false;
}
return true;
})
.map((tx) => tx.id);

this._deleteTransactions(txsToDelete);
this._addTransactionsToState([txMeta]);
return txMeta;
}
Expand Down Expand Up @@ -612,4 +627,20 @@ export default class TransactionStateManager extends EventEmitter {
transactions,
});
}

/**
* removes multiple transaction from state. This is not intended for external use.
*
* @private
* @param {number[]} targetTransactionIds - the transactions to delete
*/
_deleteTransactions(targetTransactionIds) {
const { transactions } = this.store.getState();
targetTransactionIds.forEach((transactionId) => {
delete transactions[transactionId];
});
this.store.updateState({
transactions,
});
}
}
249 changes: 198 additions & 51 deletions app/scripts/controllers/transactions/tx-state-manager.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { strict as assert } from 'assert';
import sinon from 'sinon';
import { TRANSACTION_STATUSES } from '../../../../shared/constants/transaction';
import {
TRANSACTION_STATUSES,
TRANSACTION_TYPES,
} from '../../../../shared/constants/transaction';
import {
KOVAN_CHAIN_ID,
KOVAN_NETWORK_ID,
Expand All @@ -10,6 +13,36 @@ import { snapshotFromTxMeta } from './lib/tx-state-history-helpers';

const VALID_ADDRESS = '0x0000000000000000000000000000000000000000';
const VALID_ADDRESS_TWO = '0x0000000000000000000000000000000000000001';

function generateTransactions(
numToGen,
{
chainId,
to,
from,
status,
type = TRANSACTION_TYPES.SENT_ETHER,
nonce = (i) => `${i}`,
},
) {
const txs = [];
for (let i = 0; i < numToGen; i++) {
const tx = {
id: i,
time: new Date() * i,
status: typeof status === 'function' ? status(i) : status,
chainId: typeof chainId === 'function' ? chainId(i) : chainId,
txParams: {
nonce: nonce(i),
to,
from,
},
type: typeof type === 'function' ? type(i) : type,
};
txs.push(tx);
}
return txs;
}
describe('TransactionStateManager', function () {
let txStateManager;
const currentNetworkId = KOVAN_NETWORK_ID;
Expand Down Expand Up @@ -540,72 +573,56 @@ describe('TransactionStateManager', function () {

it('cuts off early txs beyond a limit', function () {
const limit = txStateManager.txHistoryLimit;
for (let i = 0; i < limit + 1; i++) {
const tx = {
id: i,
time: new Date(),
status: TRANSACTION_STATUSES.CONFIRMED,
metamaskNetworkId: currentNetworkId,
txParams: {
to: VALID_ADDRESS,
from: VALID_ADDRESS,
},
};
txStateManager.addTransaction(tx);
}
const txs = generateTransactions(limit + 1, {
chainId: currentChainId,
to: VALID_ADDRESS,
from: VALID_ADDRESS_TWO,
status: TRANSACTION_STATUSES.CONFIRMED,
});
txs.forEach((tx) => txStateManager.addTransaction(tx));
const result = txStateManager.getTransactions();
assert.equal(result.length, limit, `limit of ${limit} txs enforced`);
assert.equal(result[0].id, 1, 'early txs truncated');
});

it('cuts off early txs beyond a limit whether or not it is confirmed or rejected', function () {
const limit = txStateManager.txHistoryLimit;
for (let i = 0; i < limit + 1; i++) {
const tx = {
id: i,
time: new Date(),
status: TRANSACTION_STATUSES.REJECTED,
metamaskNetworkId: currentNetworkId,
txParams: {
to: VALID_ADDRESS,
from: VALID_ADDRESS,
},
};
txStateManager.addTransaction(tx);
}
const txs = generateTransactions(limit + 1, {
chainId: currentChainId,
to: VALID_ADDRESS,
from: VALID_ADDRESS_TWO,
status: TRANSACTION_STATUSES.REJECTED,
});
txs.forEach((tx) => txStateManager.addTransaction(tx));
const result = txStateManager.getTransactions();
assert.equal(result.length, limit, `limit of ${limit} txs enforced`);
assert.equal(result[0].id, 1, 'early txs truncated');
});

it('cuts off early txs beyond a limit but does not cut unapproved txs', function () {
const unconfirmedTx = {
id: 0,
time: new Date(),
status: TRANSACTION_STATUSES.UNAPPROVED,
metamaskNetworkId: currentNetworkId,
txParams: {
const limit = txStateManager.txHistoryLimit;
const txs = generateTransactions(
// we add two transactions over limit here to first insert the must be always present
// unapproved tx, then another to force the original logic of adding
// one more beyond the first additional.
limit + 2,
{
chainId: currentChainId,
to: VALID_ADDRESS,
from: VALID_ADDRESS,
from: VALID_ADDRESS_TWO,
status: (i) =>
i === 0
? TRANSACTION_STATUSES.UNAPPROVED
: TRANSACTION_STATUSES.CONFIRMED,
},
};
txStateManager.addTransaction(unconfirmedTx);
const limit = txStateManager.txHistoryLimit;
for (let i = 1; i < limit + 1; i++) {
const tx = {
id: i,
time: new Date(),
status: TRANSACTION_STATUSES.CONFIRMED,
metamaskNetworkId: currentNetworkId,
txParams: {
to: VALID_ADDRESS,
from: VALID_ADDRESS,
},
};
txStateManager.addTransaction(tx);
}
);
txs.forEach((tx) => txStateManager.addTransaction(tx));
const result = txStateManager.getTransactions();
assert.equal(result.length, limit, `limit of ${limit} txs enforced`);
assert.equal(
result.length,
limit + 1,
`limit of ${limit} + 1 for the unapproved tx is enforced`,
);
assert.equal(result[0].id, 0, 'first tx should still be there');
assert.equal(
result[0].status,
Expand All @@ -614,6 +631,136 @@ describe('TransactionStateManager', function () {
);
assert.equal(result[1].id, 2, 'early txs truncated');
});

it('cuts off entire groups of transactions by nonce when adding new transaction', function () {
const limit = txStateManager.txHistoryLimit;
// In this test case the earliest two transactions are a dropped attempted ether send and a
// following cancel transaction with the same nonce. these two transactions should be dropped
// together as soon as the 11th unique nonce is attempted to be added, mocked here with limit + 1
const txs = generateTransactions(limit + 2, {
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
chainId: currentChainId,
Copy link
Contributor

Choose a reason for hiding this comment

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

should the nonce method param be defined here, to ensure more than once transaction of the same nonce is in the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! must have dropped during code cleanup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

to: VALID_ADDRESS,
from: VALID_ADDRESS_TWO,
nonce: (i) => (i === 1 ? `0` : `${i}`),
status: (i) =>
i === 0
? TRANSACTION_STATUSES.DROPPED
: TRANSACTION_STATUSES.CONFIRMED,
type: (i) =>
i === 1 ? TRANSACTION_TYPES.CANCEL : TRANSACTION_STATUSES.SENT_ETHER,
});
txs.forEach((tx) => txStateManager.addTransaction(tx));
const result = txStateManager.getTransactions();
assert.equal(result.length, limit, `limit of ${limit} is enforced`);
assert.notEqual(result[0].id, 0, 'first tx should be removed');
assert.notEqual(
result[0].status,
TRANSACTION_STATUSES.DROPPED,
'first tx should not be dropped',
);
assert.notEqual(
result[1].type,
TRANSACTION_TYPES.CANCEL,
'Cancel tx should be dropped',
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
);
});

it('cuts off entire groups of transactions by nonce + network when adding new transaction', function () {
const limit = txStateManager.txHistoryLimit;
// In this test case the earliest two transactions are a dropped attempted ether send and a
// following cancel transaction with the same nonce. Then, a bit later the same scenario on a
// different network. The first two transactions should be dropped after adding even another
// single transaction but the other shouldn't be dropped until adding the fifth additional
// transaction
const txs = generateTransactions(limit + 5, {
chainId: (i) => ([0, 1, 4, 5].includes(i) ? currentChainId : '0x1'),
to: VALID_ADDRESS,
from: VALID_ADDRESS_TWO,
nonce: (i) => {
if (i === 1) return '0';
else if (i === 5) return '4';
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
return `${i}`;
},
status: (i) =>
i === 0 || i === 4
? TRANSACTION_STATUSES.DROPPED
: TRANSACTION_STATUSES.CONFIRMED,
type: (i) =>
i === 1 || i === 5
? TRANSACTION_TYPES.CANCEL
: TRANSACTION_STATUSES.SENT_ETHER,
});
txs.forEach((tx) => txStateManager.addTransaction(tx));
const result = txStateManager.getTransactions({
filterToCurrentNetwork: false,
});
assert.equal(
result.length,
limit + 1,
`limit of ${limit} + 1 for the grouped transactions is enforced`,
);
assert.notEqual(result[0].id, 0, 'first tx should be removed');
assert.equal(
result[0].status,
TRANSACTION_STATUSES.DROPPED,
'first tx should be dropped',
);
assert.equal(
result[0].txParams.nonce,
'0x4',
'the first tx should be from the second group',
);
assert.equal(
result[1].type,
TRANSACTION_TYPES.CANCEL,
'second transaction should be a cancel',
);
assert.equal(
result[1].txParams.nonce,
'0x4',
'the second tx should be from the second group',
);
});

it('does not cut off entire groups of transactions when adding new transaction when under limit', function () {
// In this test case the earliest two transactions are a dropped attempted ether send and a
// following cancel transaction with the same nonce. Then, a bit later the same scenario on a
// different network. None of these should be dropped because we haven't yet reached the limit
const txs = generateTransactions(9, {
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
chainId: (i) => ([0, 1, 4, 5].includes(i) ? currentChainId : '0x1'),
to: VALID_ADDRESS,
from: VALID_ADDRESS_TWO,
nonce: (i) => {
if (i === 1) return '0';
else if (i === 5) return '4';
return `${i}`;
},
status: (i) =>
i === 0 || i === 4
? TRANSACTION_STATUSES.DROPPED
: TRANSACTION_STATUSES.CONFIRMED,
type: (i) =>
i === 1 || i === 5
? TRANSACTION_TYPES.CANCEL
: TRANSACTION_STATUSES.SENT_ETHER,
});
txs.forEach((tx) => txStateManager.addTransaction(tx));
const result = txStateManager.getTransactions({
filterToCurrentNetwork: false,
});
assert.equal(result.length, 9, `all nine transactions should be present`);
assert.equal(result[0].id, 0, 'first tx should be present');
assert.equal(
result[0].status,
TRANSACTION_STATUSES.DROPPED,
'first tx should be dropped',
);
assert.equal(
result[1].type,
TRANSACTION_TYPES.CANCEL,
'second transaction should be a cancel',
);
});
});

describe('#updateTransaction', function () {
Expand Down