Skip to content

refactor(transaction-controller): remove existing transaction deduplication logic #8256

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

Draft
matthewwalsh0 wants to merge 1 commit into
base: main
Choose a base branch
from
+94 −393
Draft

refactor(transaction-controller): remove existing transaction deduplication logic #8256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions packages/transaction-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed

- Bump `@metamask/core-backend` from `^6.1.1` to `^6.2.0` ([#8232](https://github.com/MetaMask/core/pull/8232))
- Remove legacy existing transaction deduplication logic based on `actionId` ([#8256](https://github.com/MetaMask/core/pull/8256))
- `addTransaction` no longer reuses an existing transaction when called with a previously used `actionId`. A new transaction is always created.
- `stopTransaction` and `speedUpTransaction` no longer skip execution when called with a previously used `actionId`.
- The `actionId` property is still persisted on `TransactionMeta` and included in event payloads.

## [63.0.0]

Expand Down
273 changes: 0 additions & 273 deletions packages/transaction-controller/src/TransactionController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1532,219 +1532,6 @@ describe('TransactionController', () => {
});
});

describe('with actionId', () => {
it('adds single unapproved transaction when called twice with same actionId', async () => {
const { controller, mockTransactionApprovalRequest } = setupController();

const mockOrigin = 'origin';

await controller.addTransaction(
{
from: ACCOUNT_MOCK,
to: ACCOUNT_MOCK,
},
{
origin: mockOrigin,
actionId: ACTION_ID_MOCK,
networkClientId: NETWORK_CLIENT_ID_MOCK,
},
);

const firstTransactionCount = controller.state.transactions.length;

await controller.addTransaction(
{
from: ACCOUNT_MOCK,
to: ACCOUNT_MOCK,
},
{
origin: mockOrigin,
actionId: ACTION_ID_MOCK,
networkClientId: NETWORK_CLIENT_ID_MOCK,
},
);
const secondTransactionCount = controller.state.transactions.length;

expect(firstTransactionCount).toStrictEqual(secondTransactionCount);
expect(
mockTransactionApprovalRequest.actionHandlerMock,
).toHaveBeenCalledTimes(1);
expect(
mockTransactionApprovalRequest.actionHandlerMock,
).toHaveBeenCalledWith(
{
id: expect.any(String),
origin: mockOrigin,
type: 'transaction',
requestData: { txId: expect.any(String) },
expectsResult: true,
},
true,
);
});

it('adds multiple transactions with same actionId and ensures second transaction result does not resolves before the first transaction result', async () => {
const { controller, mockTransactionApprovalRequest } = setupController();

const mockOrigin = 'origin';
let firstTransactionCompleted = false;
let secondTransactionCompleted = false;

const { result: firstResult } = await controller.addTransaction(
{
from: ACCOUNT_MOCK,
to: ACCOUNT_MOCK,
},
{
origin: mockOrigin,
actionId: ACTION_ID_MOCK,
networkClientId: NETWORK_CLIENT_ID_MOCK,
},
);

firstResult
.then(() => {
firstTransactionCompleted = true;
return undefined;
})
.catch(() => undefined);

const { result: secondResult } = await controller.addTransaction(
{
from: ACCOUNT_MOCK,
to: ACCOUNT_MOCK,
},
{
origin: mockOrigin,
actionId: ACTION_ID_MOCK,
networkClientId: NETWORK_CLIENT_ID_MOCK,
},
);
secondResult
.then(() => {
secondTransactionCompleted = true;
return undefined;
})
.catch(() => undefined);

await wait(0);

expect(firstTransactionCompleted).toBe(false);
expect(secondTransactionCompleted).toBe(false);

mockTransactionApprovalRequest.approve();
await firstResult;
await secondResult;

expect(firstTransactionCompleted).toBe(true);
expect(secondTransactionCompleted).toBe(true);
});

it.each([
[
'does not add duplicate transaction if actionId already used',
ACTION_ID_MOCK,
ACTION_ID_MOCK,
1,
],
[
'adds additional transaction if actionId not used',
ACTION_ID_MOCK,
'00000',
2,
],
])(
'%s',
async (_, firstActionId, secondActionId, expectedTransactionCount) => {
const { controller, mockTransactionApprovalRequest } =
setupController();
const expectedRequestApprovalCalledTimes = expectedTransactionCount;

const mockOrigin = 'origin';

await controller.addTransaction(
{
from: ACCOUNT_MOCK,
to: ACCOUNT_MOCK,
},
{
origin: mockOrigin,
actionId: firstActionId,
networkClientId: NETWORK_CLIENT_ID_MOCK,
},
);

await controller.addTransaction(
{
from: ACCOUNT_MOCK,
to: ACCOUNT_MOCK,
},
{
origin: mockOrigin,
actionId: secondActionId,
networkClientId: NETWORK_CLIENT_ID_MOCK,
},
);
const { transactions } = controller.state;

expect(transactions).toHaveLength(expectedTransactionCount);
expect(
mockTransactionApprovalRequest.actionHandlerMock,
).toHaveBeenCalledTimes(expectedRequestApprovalCalledTimes);
},
);

it.each([
[
'adds single transaction when speed up called twice with the same actionId',
ACTION_ID_MOCK,
2,
1,
],
[
'adds multiple transactions when speed up called with non-existent actionId',
'00000',
3,
2,
],
])(
'%s',
async (
_,
actionId,
expectedTransactionCount,
expectedSignCalledTimes,
) => {
const { controller } = setupController();

const { transactionMeta } = await controller.addTransaction(
{
from: ACCOUNT_MOCK,
gas: '0x0',
gasPrice: '0x50fd51da',
to: ACCOUNT_MOCK,
value: '0x0',
},
{
networkClientId: NETWORK_CLIENT_ID_MOCK,
},
);

await controller.speedUpTransaction(transactionMeta.id, undefined, {
actionId: ACTION_ID_MOCK,
});

await controller.speedUpTransaction(transactionMeta.id, undefined, {
actionId,
});

const { transactions } = controller.state;
expect(transactions).toHaveLength(expectedTransactionCount);
expect(signMock).toHaveBeenCalledTimes(expectedSignCalledTimes);
},
);
});

describe('addTransaction', () => {
it('adds unapproved transaction to state', async () => {
const { controller } = setupController();
Expand Down Expand Up @@ -3978,36 +3765,6 @@ describe('TransactionController', () => {
});

describe('stopTransaction', () => {
it('should avoid creating cancel transaction if actionId already exist', async () => {
const mockActionId = 'mockActionId';
const { controller } = setupController({
options: {
state: {
transactions: [
{
actionId: mockActionId,
id: '2',
chainId: toHex(5),
networkClientId: NETWORK_CLIENT_ID_MOCK,
status: TransactionStatus.submitted,
type: TransactionType.cancel,
time: 123456789,
txParams: {
from: ACCOUNT_MOCK,
},
},
],
},
},
});

await controller.stopTransaction('2', undefined, {
actionId: mockActionId,
});

expect(controller.state.transactions).toHaveLength(1);
});

it('should throw error if transaction already confirmed', async () => {
const { controller } = setupController({
options: {
Expand Down Expand Up @@ -4366,36 +4123,6 @@ describe('TransactionController', () => {
expect(speedUpTransaction.type).toBe(TransactionType.retry);
});

it('should avoid creating speedup transaction if actionId already exist', async () => {
const mockActionId = 'mockActionId';
const { controller } = setupController({
options: {
state: {
transactions: [
{
actionId: mockActionId,
id: '2',
networkClientId: NETWORK_CLIENT_ID_MOCK,
chainId: toHex(5),
status: TransactionStatus.submitted,
type: TransactionType.retry,
time: 123456789,
txParams: {
from: ACCOUNT_MOCK,
},
},
],
},
},
});

await controller.speedUpTransaction('2', undefined, {
actionId: mockActionId,
});

expect(controller.state.transactions).toHaveLength(1);
});

it('should throw error if transaction already confirmed', async () => {
const { controller } = setupController({
options: {
Expand Down
Loading
Loading