From d635ed6f21213bafb63ce1a21990edcc511b6f33 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 23 Mar 2022 11:59:14 +0100 Subject: [PATCH 1/2] fix: cache block of pending transaction --- .../safe/store/actions/createTransaction.ts | 4 +- .../safe/store/actions/pendingTransactions.ts | 18 ++++++ .../safe/store/reducer/pendingTransactions.ts | 20 ++++++- .../__tests__/pendingTxMonitor.test.ts | 57 +++++++++++++++++-- .../safe/transactions/pendingTxMonitor.ts | 8 +-- 5 files changed, 93 insertions(+), 14 deletions(-) diff --git a/src/logic/safe/store/actions/createTransaction.ts b/src/logic/safe/store/actions/createTransaction.ts index fd77061970..e8e21b5ae0 100644 --- a/src/logic/safe/store/actions/createTransaction.ts +++ b/src/logic/safe/store/actions/createTransaction.ts @@ -25,7 +25,7 @@ import { Dispatch, DispatchReturn } from './types' import { checkIfOffChainSignatureIsPossible, getPreValidatedSignatures } from 'src/logic/safe/safeTxSigner' import { TxParameters } from 'src/routes/safe/container/hooks/useTransactionParameters' import { Errors, logError } from 'src/logic/exceptions/CodedException' -import { removePendingTransaction, addPendingTransaction } from 'src/logic/safe/store/actions/pendingTransactions' +import { removePendingTransaction, setPendingTransaction } from 'src/logic/safe/store/actions/pendingTransactions' import { _getChainId } from 'src/config' import { GnosisSafe } from 'src/types/contracts/gnosis_safe.d' import * as aboutToExecuteTx from 'src/logic/safe/utils/aboutToExecuteTx' @@ -109,7 +109,7 @@ export class TxSender { } if (isFinalization && this.txId && this.txHash) { - dispatch(addPendingTransaction({ id: this.txId, txHash: this.txHash })) + dispatch(setPendingTransaction({ id: this.txId, txHash: this.txHash })) } notifications.closePending() diff --git a/src/logic/safe/store/actions/pendingTransactions.ts b/src/logic/safe/store/actions/pendingTransactions.ts index 39a2d0afc0..8881252a67 100644 --- a/src/logic/safe/store/actions/pendingTransactions.ts +++ b/src/logic/safe/store/actions/pendingTransactions.ts @@ -3,6 +3,8 @@ import { AddPendingTransactionPayload, RemovePendingTransactionPayload, } from 'src/logic/safe/store/reducer/pendingTransactions' +import { getWeb3 } from 'src/logic/wallets/getWeb3' +import { Dispatch } from './types' export enum PENDING_TRANSACTIONS_ACTIONS { ADD = 'pendingTransactions/add', @@ -13,3 +15,19 @@ export const addPendingTransaction = createAction( export const removePendingTransaction = createAction( PENDING_TRANSACTIONS_ACTIONS.REMOVE, ) + +export const setPendingTransaction = (details: { id: string; txHash: string }) => { + return async (dispatch: Dispatch): Promise => { + let block: number | undefined + try { + block = await getWeb3().eth.getBlockNumber() + } catch {} + + const pendingTransaction = { + ...details, + block, + } + + dispatch(addPendingTransaction(pendingTransaction)) + } +} diff --git a/src/logic/safe/store/reducer/pendingTransactions.ts b/src/logic/safe/store/reducer/pendingTransactions.ts index 34de06170a..cb4f587fbb 100644 --- a/src/logic/safe/store/reducer/pendingTransactions.ts +++ b/src/logic/safe/store/reducer/pendingTransactions.ts @@ -7,7 +7,14 @@ import { _getChainId } from 'src/config' export const PENDING_TRANSACTIONS_ID = 'pendingTransactions' -export type PendingTransactionsState = Record> +export type PendingTransactionsState = { + [chainId: ChainId]: { + [id: string]: { + txHash: string + block?: number + } + } +} const initialPendingTxsState = session.getItem(PENDING_TRANSACTIONS_ID) || {} @@ -18,6 +25,7 @@ export type RemovePendingTransactionPayload = { export type AddPendingTransactionPayload = RemovePendingTransactionPayload & { txHash: string + block?: number } export type PendingTransactionPayloads = AddPendingTransactionPayload | RemovePendingTransactionPayload @@ -29,11 +37,17 @@ export const pendingTransactionsReducer = handleActions, ) => { const chainId = _getChainId() - const { id, txHash } = action.payload + const { id, txHash, block } = action.payload return { ...state, - [chainId]: { ...state[chainId], [id]: txHash }, + [chainId]: { + ...state[chainId], + [id]: { + txHash, + block, + }, + }, } }, [PENDING_TRANSACTIONS_ACTIONS.REMOVE]: ( diff --git a/src/logic/safe/transactions/__tests__/pendingTxMonitor.test.ts b/src/logic/safe/transactions/__tests__/pendingTxMonitor.test.ts index c017424bd6..106d2975df 100644 --- a/src/logic/safe/transactions/__tests__/pendingTxMonitor.test.ts +++ b/src/logic/safe/transactions/__tests__/pendingTxMonitor.test.ts @@ -185,9 +185,18 @@ describe('PendingTxMonitor', () => { jest.spyOn(store.store, 'getState').mockImplementation(() => ({ pendingTransactions: { '4': { - fakeTxId: 'fakeTxHash', - fakeTxId2: 'fakeTxHash2', - fakeTxId3: 'fakeTxHash3', + fakeTxId: { + txHash: 'fakeTxHash', + block: 0, + }, + fakeTxId2: { + txHash: 'fakeTxHash2', + block: 1, + }, + fakeTxId3: { + txHash: 'fakeTxHash3', + block: 2, + }, }, }, config: { @@ -219,9 +228,47 @@ describe('PendingTxMonitor', () => { expect((PendingTxMonitor._isTxMined as jest.Mock).mock.calls).toEqual([ [0, 'fakeTxHash'], - [0, 'fakeTxHash2'], - [0, 'fakeTxHash3'], + [1, 'fakeTxHash2'], + [2, 'fakeTxHash3'], ]) }) + it('falls back to the current block number if none was set', async () => { + jest.spyOn(store.store, 'getState').mockImplementation(() => ({ + pendingTransactions: { + '4': { + fakeTxId: { + txHash: 'fakeTxHash', + }, + }, + }, + config: { + chainId: '4', + }, + })) + + jest.spyOn(web3.getWeb3().eth, 'getBlockNumber').mockImplementation(() => Promise.resolve(0)) + + PendingTxMonitor._isTxMined = jest.fn(() => + Promise.resolve({ + blockHash: '0x123', + blockNumber: 1, + transactionHash: 'fakeTxHash', + transactionIndex: 0, + from: '0x123', + to: '0x123', + cumulativeGasUsed: 1, + gasUsed: 1, + contractAddress: '0x123', + logs: [], + status: true, // Mined successfully + logsBloom: '0x123', + effectiveGasPrice: 0, + }), + ) + + await PendingTxMonitor.monitorAllTxs() + + expect((PendingTxMonitor._isTxMined as jest.Mock).mock.calls).toEqual([[0, 'fakeTxHash']]) + }) }) }) diff --git a/src/logic/safe/transactions/pendingTxMonitor.ts b/src/logic/safe/transactions/pendingTxMonitor.ts index 664e44d806..588bacad48 100644 --- a/src/logic/safe/transactions/pendingTxMonitor.ts +++ b/src/logic/safe/transactions/pendingTxMonitor.ts @@ -9,8 +9,8 @@ import { removePendingTransaction } from 'src/logic/safe/store/actions/pendingTr import { pendingTxIdsByChain } from 'src/logic/safe/store/selectors/pendingTransactions' import { didTxRevert } from 'src/logic/safe/store/actions/transactions/utils/transactionHelpers' -const _isTxMined = async (sessionBlockNumber: number, txHash: string): Promise => { - const MAX_WAITING_BLOCK = sessionBlockNumber + 50 +const _isTxMined = async (blockNumber: number, txHash: string): Promise => { + const MAX_WAITING_BLOCK = blockNumber + 50 const web3 = getWeb3() @@ -72,8 +72,8 @@ const monitorAllTxs = async (): Promise => { try { const sessionBlockNumber = await web3.eth.getBlockNumber() await Promise.all( - pendingTxs.map(([txId, txHash]) => { - return PendingTxMonitor.monitorTx(sessionBlockNumber, txId, txHash) + pendingTxs.map(([txId, { txHash, block = sessionBlockNumber }]) => { + return PendingTxMonitor.monitorTx(block, txId, txHash) }), ) } catch { From 53a9c3e9f0f41c99af341d7293be5a16a5d26bf3 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 23 Mar 2022 13:07:53 +0100 Subject: [PATCH 2/2] fix: optimise `_isTxMined` + fix tests --- .../store/actions/__tests__/TxSender.test.ts | 12 +- .../__tests__/pendingTxMonitor.test.ts | 107 ++++++------------ .../safe/transactions/pendingTxMonitor.ts | 24 ++-- 3 files changed, 54 insertions(+), 89 deletions(-) diff --git a/src/logic/safe/store/actions/__tests__/TxSender.test.ts b/src/logic/safe/store/actions/__tests__/TxSender.test.ts index 972d8607c8..47e48d1495 100644 --- a/src/logic/safe/store/actions/__tests__/TxSender.test.ts +++ b/src/logic/safe/store/actions/__tests__/TxSender.test.ts @@ -112,7 +112,7 @@ const mockPromiEvent = { } describe('TxSender', () => { - let tryOffChainSigningSpy, saveTxToHistorySpy, addPendingTransactionSpy, navigateToTxSpy, fetchTransactionsSpy + let tryOffChainSigningSpy, saveTxToHistorySpy, setPendingTransactionSpy, navigateToTxSpy, fetchTransactionsSpy beforeEach(() => { jest.restoreAllMocks() @@ -135,7 +135,7 @@ describe('TxSender', () => { saveTxToHistorySpy = jest .spyOn(txHistory, 'saveTxToHistory') .mockImplementation(() => Promise.resolve(mockTransactionDetails as any)) - addPendingTransactionSpy = jest.spyOn(pendingTransactions, 'addPendingTransaction') + setPendingTransactionSpy = jest.spyOn(pendingTransactions, 'setPendingTransaction') navigateToTxSpy = jest.spyOn(utils, 'navigateToTx') fetchTransactionsSpy = jest.spyOn(fetchTransactions, 'default') @@ -173,7 +173,7 @@ describe('TxSender', () => { await waitFor(() => { expect(tryOffChainSigningSpy).toHaveBeenCalledTimes(1) expect(saveTxToHistorySpy).toHaveBeenCalledTimes(1) - expect(addPendingTransactionSpy).toHaveBeenCalledTimes(0) + expect(setPendingTransactionSpy).toHaveBeenCalledTimes(0) expect(navigateToTxSpy).toHaveBeenCalledTimes(0) expect(fetchTransactionsSpy).toHaveBeenCalledTimes(1) }) @@ -210,7 +210,7 @@ describe('TxSender', () => { await waitFor(() => { expect(tryOffChainSigningSpy).toHaveBeenCalledTimes(1) expect(saveTxToHistorySpy).toHaveBeenCalledTimes(1) - expect(addPendingTransactionSpy).toHaveBeenCalledTimes(0) + expect(setPendingTransactionSpy).toHaveBeenCalledTimes(0) expect(navigateToTxSpy).toHaveBeenCalledTimes(1) expect(fetchTransactionsSpy).toHaveBeenCalledTimes(1) }) @@ -266,7 +266,7 @@ describe('TxSender', () => { expect(getExecutionTransactionSpy).toHaveBeenCalledTimes(1) expect(setNonceSpy).toHaveBeenCalledTimes(1) expect(saveTxToHistorySpy).toHaveBeenCalledTimes(1) - expect(addPendingTransactionSpy).toHaveBeenCalledTimes(1) + expect(setPendingTransactionSpy).toHaveBeenCalledTimes(1) expect(navigateToTxSpy).toHaveBeenCalledTimes(1) expect(fetchTransactionsSpy).toHaveBeenCalledTimes(1) }) @@ -317,7 +317,7 @@ describe('TxSender', () => { await waitFor(() => { expect(getExecutionTransactionSpy).toHaveBeenCalledTimes(1) expect(setNonceSpy).toHaveBeenCalledTimes(1) - expect(addPendingTransactionSpy).toHaveBeenCalledTimes(1) + expect(setPendingTransactionSpy).toHaveBeenCalledTimes(1) expect(saveTxToHistorySpy).toHaveBeenCalledTimes(0) expect(navigateToTxSpy).toHaveBeenCalledTimes(0) expect(fetchTransactionsSpy).toHaveBeenCalledTimes(1) diff --git a/src/logic/safe/transactions/__tests__/pendingTxMonitor.test.ts b/src/logic/safe/transactions/__tests__/pendingTxMonitor.test.ts index 106d2975df..b468765612 100644 --- a/src/logic/safe/transactions/__tests__/pendingTxMonitor.test.ts +++ b/src/logic/safe/transactions/__tests__/pendingTxMonitor.test.ts @@ -65,23 +65,7 @@ describe('PendingTxMonitor', () => { describe('monitorTx', () => { it("doesn't clear the pending transaction if it was mined", async () => { - PendingTxMonitor._isTxMined = jest.fn(() => - Promise.resolve({ - blockHash: '0x123', - blockNumber: 1, - transactionHash: 'fakeTxHash', - transactionIndex: 0, - from: '0x123', - to: '0x123', - cumulativeGasUsed: 1, - gasUsed: 1, - contractAddress: '0x123', - logs: [], - status: true, // Mined successfully - logsBloom: '0x123', - effectiveGasPrice: 0, - }), - ) + PendingTxMonitor._isTxMined = jest.fn(() => Promise.resolve(true)) const dispatchSpy = jest.spyOn(store.store, 'dispatch').mockImplementation(() => jest.fn()) @@ -94,23 +78,7 @@ describe('PendingTxMonitor', () => { expect(dispatchSpy).not.toBeCalled() }) it('clears the pending transaction if it failed', async () => { - PendingTxMonitor._isTxMined = jest.fn(() => - Promise.resolve({ - blockHash: '0x123', - blockNumber: 1, - transactionHash: 'fakeTxHash', - transactionIndex: 0, - from: '0x123', - to: '0x123', - cumulativeGasUsed: 1, - gasUsed: 1, - contractAddress: '0x123', - logs: [], - status: false, // Mining failed - logsBloom: '0x123', - effectiveGasPrice: 0, - }), - ) + PendingTxMonitor._isTxMined = jest.fn(() => Promise.resolve(false)) const dispatchSpy = jest.spyOn(store.store, 'dispatch').mockImplementation(() => jest.fn()) @@ -127,8 +95,39 @@ describe('PendingTxMonitor', () => { }) it('clears the pending transaction it the tx was not mined within 50 blocks', async () => { - // Can return null if transaction is pending: https://web3js.readthedocs.io/en/v1.2.11/web3-eth.html#gettransactionreceipt - PendingTxMonitor._isTxMined = jest.fn(() => Promise.reject(null as unknown as TransactionReceipt)) + PendingTxMonitor._isTxMined = jest.fn(() => Promise.resolve(false)) + + const dispatchSpy = jest.spyOn(store.store, 'dispatch').mockImplementation(() => jest.fn()) + + await PendingTxMonitor.monitorTx(0, 'fakeTxId', 'fakeTxHash', { + numOfAttempts: 1, + startingDelay: 0, + timeMultiple: 0, + }) + + expect(dispatchSpy).toHaveBeenCalledTimes(1) + }) + + it('clears the pending transaction it retrieving the block number throws', async () => { + jest + .spyOn(web3.getWeb3().eth, 'getTransactionReceipt') + // Returns `null` if transaction is pending: https://web3js.readthedocs.io/en/v1.2.11/web3-eth.html#gettransactionreceipt + .mockImplementation(() => Promise.resolve(null as any)) + jest.spyOn(web3.getWeb3().eth, 'getBlockNumber').mockImplementation(() => Promise.reject()) + + const dispatchSpy = jest.spyOn(store.store, 'dispatch').mockImplementation(() => jest.fn()) + + await PendingTxMonitor.monitorTx(0, 'fakeTxId', 'fakeTxHash', { + numOfAttempts: 1, + startingDelay: 0, + timeMultiple: 0, + }) + + expect(dispatchSpy).toHaveBeenCalledTimes(2) + }) + + it('clears the pending transaction it throws in the final backOff', async () => { + PendingTxMonitor._isTxMined = jest.fn(() => Promise.reject()) const dispatchSpy = jest.spyOn(store.store, 'dispatch').mockImplementation(() => jest.fn()) @@ -206,23 +205,7 @@ describe('PendingTxMonitor', () => { jest.spyOn(web3.getWeb3().eth, 'getBlockNumber').mockImplementation(() => Promise.resolve(0)) - PendingTxMonitor._isTxMined = jest.fn(() => - Promise.resolve({ - blockHash: '0x123', - blockNumber: 1, - transactionHash: 'fakeTxHash', - transactionIndex: 0, - from: '0x123', - to: '0x123', - cumulativeGasUsed: 1, - gasUsed: 1, - contractAddress: '0x123', - logs: [], - status: true, // Mined successfully - logsBloom: '0x123', - effectiveGasPrice: 0, - }), - ) + PendingTxMonitor._isTxMined = jest.fn(() => Promise.resolve(true)) await PendingTxMonitor.monitorAllTxs() @@ -248,23 +231,7 @@ describe('PendingTxMonitor', () => { jest.spyOn(web3.getWeb3().eth, 'getBlockNumber').mockImplementation(() => Promise.resolve(0)) - PendingTxMonitor._isTxMined = jest.fn(() => - Promise.resolve({ - blockHash: '0x123', - blockNumber: 1, - transactionHash: 'fakeTxHash', - transactionIndex: 0, - from: '0x123', - to: '0x123', - cumulativeGasUsed: 1, - gasUsed: 1, - contractAddress: '0x123', - logs: [], - status: true, // Mined successfully - logsBloom: '0x123', - effectiveGasPrice: 0, - }), - ) + PendingTxMonitor._isTxMined = jest.fn(() => Promise.resolve(true)) await PendingTxMonitor.monitorAllTxs() diff --git a/src/logic/safe/transactions/pendingTxMonitor.ts b/src/logic/safe/transactions/pendingTxMonitor.ts index 588bacad48..7dcfb059c3 100644 --- a/src/logic/safe/transactions/pendingTxMonitor.ts +++ b/src/logic/safe/transactions/pendingTxMonitor.ts @@ -1,5 +1,4 @@ import { backOff, IBackOffOptions } from 'exponential-backoff' -import { TransactionReceipt } from 'web3-core' import { NOTIFICATIONS } from 'src/logic/notifications' import enqueueSnackbar from 'src/logic/notifications/store/actions/enqueueSnackbar' @@ -9,23 +8,23 @@ import { removePendingTransaction } from 'src/logic/safe/store/actions/pendingTr import { pendingTxIdsByChain } from 'src/logic/safe/store/selectors/pendingTransactions' import { didTxRevert } from 'src/logic/safe/store/actions/transactions/utils/transactionHelpers' -const _isTxMined = async (blockNumber: number, txHash: string): Promise => { +const _isTxMined = async (blockNumber: number, txHash: string): Promise => { const MAX_WAITING_BLOCK = blockNumber + 50 const web3 = getWeb3() const receipt = await web3.eth.getTransactionReceipt(txHash) - if ( - // Transaction hasn't yet been mined - !receipt && - // The current block is within waiting window - (await web3.eth.getBlockNumber()) <= MAX_WAITING_BLOCK - ) { + if (receipt) { + return !didTxRevert(receipt) + } + + if ((await web3.eth.getBlockNumber()) <= MAX_WAITING_BLOCK) { + // backOff retries throw new Error('Pending transaction not found') } - return receipt + return false } // Progressively after 10s, 20s, 40s, 80s, 160s, 320s - total of 6.5 minutes @@ -44,12 +43,11 @@ const monitorTx = async ( }, ): Promise => { return backOff(() => PendingTxMonitor._isTxMined(sessionBlockNumber, txHash), options) - .then((receipt) => { - if (didTxRevert(receipt)) { + .then((isMined) => { + if (!isMined) { store.dispatch(removePendingTransaction({ id: txId })) } - // If successfully mined, pending status is removed in the transaction - // middleware when a transaction is added to historical transactions list + // If successfully mined the transaction will be removed by the automatic polling }) .catch(() => { // Unsuccessfully mined (threw in last backOff attempt)