From 41abebbb515430209d7d213e612025ac467a65e6 Mon Sep 17 00:00:00 2001 From: Gergo Nagy Date: Sun, 30 Oct 2022 18:26:17 +0100 Subject: [PATCH 1/9] extend the transferAsset with nonce option This addition will create the path for canceling transactions and also user defined nonce handling. For this to work the nonce removal had to be moved into the provider bridge service. This should have been it's place originally. The reason for this is that a dApp can't know the correct nonce to assign to any transaction that it sends to be signed. That's because the number of sign requests is not a published information. --- background/redux-slices/assets.ts | 4 ++++ background/services/internal-ethereum-provider/index.ts | 2 -- background/services/provider-bridge/index.ts | 8 +++++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/background/redux-slices/assets.ts b/background/redux-slices/assets.ts index caf706097f..ea8b410f31 100644 --- a/background/redux-slices/assets.ts +++ b/background/redux-slices/assets.ts @@ -124,11 +124,13 @@ export const transferAsset = createBackgroundAsyncThunk( toAddressNetwork: { address: toAddress, network: toNetwork }, assetAmount, gasLimit, + nonce, }: { fromAddressNetwork: AddressOnNetwork toAddressNetwork: AddressOnNetwork assetAmount: AnyAssetAmount gasLimit?: bigint + nonce?: number }) => { if (!sameNetwork(fromNetwork, toNetwork)) { throw new Error("Only same-network transfers are supported for now.") @@ -147,6 +149,7 @@ export const transferAsset = createBackgroundAsyncThunk( to: toAddress, value: assetAmount.amount, gasLimit, + nonce, }) } else if (isSmartContractFungibleAsset(assetAmount.asset)) { logger.debug( @@ -167,6 +170,7 @@ export const transferAsset = createBackgroundAsyncThunk( await signer.sendUncheckedTransaction({ ...transactionDetails, gasLimit: gasLimit ?? transactionDetails.gasLimit, + nonce, }) } else { throw new Error( diff --git a/background/services/internal-ethereum-provider/index.ts b/background/services/internal-ethereum-provider/index.ts index c09fcb881b..f8d8802e87 100644 --- a/background/services/internal-ethereum-provider/index.ts +++ b/background/services/internal-ethereum-provider/index.ts @@ -207,8 +207,6 @@ export default class InternalEthereumProviderService extends BaseService return this.signTransaction( { ...(params[0] as JsonRpcTransactionRequest), - // we populate the nonce during the signing process. If we receive one, it's safer to just ignore it. - nonce: undefined, }, origin ).then(async (signed) => { diff --git a/background/services/provider-bridge/index.ts b/background/services/provider-bridge/index.ts index ced53a15fc..31e055c60b 100644 --- a/background/services/provider-bridge/index.ts +++ b/background/services/provider-bridge/index.ts @@ -434,7 +434,13 @@ export default class ProviderBridgeService extends BaseService { case "eth_signTransaction": case "eth_sendTransaction": checkPermissionSignTransaction( - params[0] as EthersTransactionRequest, + { + // A dApp can't know what should be the next nonce because it can't access + // the information about how many tx are in the signing process inside the + // wallet. Nonce should be assigned only by the wallet. + ...(params[0] as EthersTransactionRequest), + nonce: undefined, + }, enablingPermission ) From df9ca3268fb95cabce2042f65257610948935b55 Mon Sep 17 00:00:00 2001 From: Gergo Nagy Date: Sun, 30 Oct 2022 18:35:26 +0100 Subject: [PATCH 2/9] clean txs from our registry which are not on chain In certain edge cases it can happen, that we have stored transactions that did not get registered on chain. Before this change we didn't have a way to clean those up. This change implements a public `getOrCancelTransaction` that can be used later to trigger the clean up from the UI. These type of error should be also cleaned up automatically, so this function is also called from `retrieveTransaction`. This way if any faulty tx is registered will be cleaned up on `queuedTransactions` alarm. (The same cleanup would have happened after `TRANSACTION_CHECK_LIFETIME_MS` but since that's 10h it was perceived as a bug by the users. --- background/services/chain/index.ts | 91 ++++++++++++++++++++++-- background/services/chain/utils/index.ts | 2 +- 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/background/services/chain/index.ts b/background/services/chain/index.ts index 541165fbf5..15cde77535 100644 --- a/background/services/chain/index.ts +++ b/background/services/chain/index.ts @@ -1,4 +1,7 @@ -import { TransactionReceipt } from "@ethersproject/providers" +import { + TransactionReceipt, + TransactionResponse, +} from "@ethersproject/providers" import { ethers, utils } from "ethers" import { Logger, UnsignedTransaction } from "ethers/lib/utils" import logger from "../../lib/logger" @@ -740,7 +743,10 @@ export default class ChainService extends BaseService { * available for reuse all intervening nonces. */ releaseEVMTransactionNonce( - transactionRequest: TransactionRequestWithNonce | SignedTransaction + transactionRequest: + | TransactionRequestWithNonce + | SignedTransaction + | AnyEVMTransaction ): void { const chainID = "chainID" in transactionRequest @@ -749,6 +755,16 @@ export default class ChainService extends BaseService { if (CHAINS_WITH_MEMPOOL.has(chainID)) { const { nonce } = transactionRequest const normalizedAddress = normalizeEVMAddress(transactionRequest.from) + + if ( + !this.evmChainLastSeenNoncesByNormalizedAddress[chainID] || + !this.evmChainLastSeenNoncesByNormalizedAddress[chainID][ + normalizedAddress + ] + ) { + return + } + const lastSeenNonce = this.evmChainLastSeenNoncesByNormalizedAddress[chainID][ normalizedAddress @@ -1187,6 +1203,63 @@ export default class ChainService extends BaseService { return this.providerForNetworkOrThrow(network).send(method, params) } + /** + * Retrieves a confirmed or unconfirmed transaction's details from chain. + * If found, then returns the transaction result received from chain. + * If the tx hash is not found on chain, then remove it from the lookup queue + * and mark it as dropped in the db. This will filter and fix those situations + * when our records differ from what the chain/mempool sees. This can happen in + * case of unstable networking conditions. + * + * @param network + * @param hash + */ + async getOrCancelTransaction( + network: EVMNetwork, + hash: string + ): Promise { + const result = await this.providerForNetworkOrThrow(network).getTransaction( + hash + ) + + if (!result) { + logger.warn( + `Tx has ${hash} is found in our local registry but not on chain.` + ) + + if ( + this.transactionsToRetrieve.some((queuedTx) => queuedTx.hash === hash) + ) { + // Let's clean up the tx queue if the hash is present. + // The pending tx hash should be on chain as soon as it's broadcasted. + this.transactionsToRetrieve = this.transactionsToRetrieve.filter( + (queuedTx) => queuedTx.hash !== hash + ) + } + + const savedTx = await this.db.getTransaction(network, hash) + if (savedTx && !("status" in savedTx)) { + // Let's see if we have the tx in the db, and if yes let's mark it as dropped. + this.saveTransaction( + { + ...savedTx, + status: 0, // dropped status + error: + "Transaction was in our local db but was not found on chain.", + blockHash: null, + blockHeight: null, + }, + "alchemy" + ) + + // Let's also release the nonce from our bookkeeping. + await this.releaseEVMTransactionNonce(savedTx) + } + } + + return result + } + /* ***************** * PRIVATE METHODS * * **************** */ @@ -1405,16 +1478,22 @@ export default class ChainService extends BaseService { firstSeen: number ): Promise { try { - const result = await this.providerForNetworkOrThrow( - network - ).getTransaction(hash) + const result = await this.getOrCancelTransaction(network, hash) + + if (!result) { + throw new Error(`Tx hash: ${hash} was not found on ${network.name}`) + } const transaction = transactionFromEthersTransaction(result, network) // TODO make this provider type specific await this.saveTransaction(transaction, "alchemy") - if (!transaction.blockHash && !transaction.blockHeight) { + if ( + !("status" in transaction) && // if status field is present then it's not a pending tx anymore. + !transaction.blockHash && + !transaction.blockHeight + ) { this.subscribeToTransactionConfirmation( transaction.network, transaction diff --git a/background/services/chain/utils/index.ts b/background/services/chain/utils/index.ts index 41debdcbc2..ec53fc0c30 100644 --- a/background/services/chain/utils/index.ts +++ b/background/services/chain/utils/index.ts @@ -317,7 +317,7 @@ export function transactionFromEthersTransaction( }, network: EVMNetwork ): AnyEVMTransaction { - if (tx.hash === undefined) { + if (!tx || tx.hash === undefined) { throw new Error("Malformed transaction") } if (!isKnownTxType(tx.type)) { From cb3cd85a60630585836a464b7b2a734a4b8a53c0 Mon Sep 17 00:00:00 2001 From: Gergo Nagy Date: Mon, 31 Oct 2022 20:19:06 +0100 Subject: [PATCH 3/9] add all tx to retrieve queue for health check Originally listeners were created for transactions that are pending. But if a transaction has wrong nonce it will be dropped by the mempool. This takes ~5-10 minutes. In this case the listener will never fire and the tx will be stuck. In this solution we can use the `transactionsToRetrieve` to check if the chain still knows about the tx, and if not we can mark dropped. There was a problem with the existing queuing solution, because it was not editable at runtime. This has caused the tx to be stuck until extension reload. A queueing solution should be put in place, but so far couldn't find an implementation that didn't felt overly complex or hacky. --- background/services/chain/index.ts | 78 ++++++++++++++++-------------- 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/background/services/chain/index.ts b/background/services/chain/index.ts index 15cde77535..70005b53f0 100644 --- a/background/services/chain/index.ts +++ b/background/services/chain/index.ts @@ -28,7 +28,6 @@ import { ARBITRUM_ONE, OPTIMISM, GOERLI, - SECOND, NETWORK_BY_CHAIN_ID, MINUTE, CHAINS_WITH_MEMPOOL, @@ -323,13 +322,7 @@ export default class ChainService extends BaseService { logger.debug( `Queuing pending transaction ${hash} for status lookup.` ) - this.queueTransactionHashToRetrieve( - network, - hash, - firstSeen - ).catch((e) => { - logger.error(e) - }) + this.queueTransactionHashToRetrieve(network, hash, firstSeen) }) }) .catch((e) => { @@ -957,11 +950,11 @@ export default class ChainService extends BaseService { * seen; used to treat transactions as dropped after a certain amount * of time. */ - async queueTransactionHashToRetrieve( + queueTransactionHashToRetrieve( network: EVMNetwork, txHash: HexString, firstSeen: UNIXTime - ): Promise { + ): void { const seen = this.transactionsToRetrieve.some(({ hash }) => hash === txHash) if (!seen) { @@ -970,6 +963,23 @@ export default class ChainService extends BaseService { } } + removeTransactionHashFromQueue(network: EVMNetwork, hash: HexString): void { + const seen = this.transactionsToRetrieve.some( + (queuedTx) => queuedTx.hash === hash + ) + + if (seen) { + // Let's clean up the tx queue if the hash is present. + // The pending tx hash should be on chain as soon as it's broadcasted. + this.transactionsToRetrieve = this.transactionsToRetrieve.filter( + (queuedTx) => queuedTx.hash !== hash + ) + + // Let's clean up the subscriptions + this.providerForNetwork(network)?.off(hash) + } + } + /** * Estimate the gas needed to make a transaction. Adds 10% as a safety net to * the base estimate returned by the provider. @@ -1218,24 +1228,15 @@ export default class ChainService extends BaseService { network: EVMNetwork, hash: string ): Promise { - const result = await this.providerForNetworkOrThrow(network).getTransaction( - hash - ) + const provider = this.providerForNetworkOrThrow(network) + const result = await provider.getTransaction(hash) if (!result) { logger.warn( `Tx has ${hash} is found in our local registry but not on chain.` ) - if ( - this.transactionsToRetrieve.some((queuedTx) => queuedTx.hash === hash) - ) { - // Let's clean up the tx queue if the hash is present. - // The pending tx hash should be on chain as soon as it's broadcasted. - this.transactionsToRetrieve = this.transactionsToRetrieve.filter( - (queuedTx) => queuedTx.hash !== hash - ) - } + this.removeTransactionHashFromQueue(network, hash) const savedTx = await this.db.getTransaction(network, hash) if (savedTx && !("status" in savedTx)) { @@ -1429,8 +1430,6 @@ export default class ChainService extends BaseService { private async handleQueuedTransactionAlarm(): Promise { const fetchedByNetwork: { [chainID: string]: number } = {} - const wait = (ms: number) => new Promise((r) => setTimeout(r, ms)) - let queue = Promise.resolve() // Drop all transactions that weren't retrieved from the queue. this.transactionsToRetrieve = this.transactionsToRetrieve.filter( @@ -1449,13 +1448,10 @@ export default class ChainService extends BaseService { // retrieve the transaction, and drop from the updated queue. fetchedByNetwork[network.chainID] += 1 - // Do not request all transactions and their related data at once - queue = queue.finally(() => - this.retrieveTransaction(network, hash, firstSeen) - // Only wait if call doesn't throw - .then(() => wait(2.5 * SECOND)) - ) - + // Note: this starts retrieving TRANSACTIONS_RETRIEVED_PER_ALARM * networks at the same time + // another queue solution should come here, but one that's editable runtime. Haven't found + // a solution yet that seemed to worth the complexity. + this.retrieveTransaction({ network, hash, firstSeen }) return false } ) @@ -1472,16 +1468,20 @@ export default class ChainService extends BaseService { * @param network the EVM network we're interested in * @param transaction the confirmed transaction we're interested in */ - private async retrieveTransaction( - network: EVMNetwork, - hash: string, + private async retrieveTransaction({ + network, + hash, + firstSeen, + }: { + network: EVMNetwork + hash: string firstSeen: number - ): Promise { + }): Promise { try { const result = await this.getOrCancelTransaction(network, hash) if (!result) { - throw new Error(`Tx hash: ${hash} was not found on ${network.name}`) + return } const transaction = transactionFromEthersTransaction(result, network) @@ -1746,7 +1746,13 @@ export default class ChainService extends BaseService { enrichTransactionWithReceipt(transaction, confirmedReceipt), "alchemy" ) + + this.removeTransactionHashFromQueue(network, transaction.hash) }) + + // Let's add the transaction to the queued lookup. If the transaction is dropped + // because of wrong nonce on chain the event will never arrive. + this.queueTransactionHashToRetrieve(network, transaction.hash, Date.now()) } /** From 64ed7bce7c09061333356db6fa8fe1d204b06531 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Nagy?= Date: Fri, 2 Dec 2022 15:53:53 +0100 Subject: [PATCH 4/9] Simplify checking logic Co-authored-by: Jagoda Berry Rybacka --- background/services/chain/index.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/background/services/chain/index.ts b/background/services/chain/index.ts index 4fd72ccaf5..5cacf60a5f 100644 --- a/background/services/chain/index.ts +++ b/background/services/chain/index.ts @@ -761,8 +761,7 @@ export default class ChainService extends BaseService { const normalizedAddress = normalizeEVMAddress(transactionRequest.from) if ( - !this.evmChainLastSeenNoncesByNormalizedAddress[chainID] || - !this.evmChainLastSeenNoncesByNormalizedAddress[chainID][ + !this.evmChainLastSeenNoncesByNormalizedAddress[chainID]?.[ normalizedAddress ] ) { From 6d5aac41107779201cd97da11b5d77789d19a21c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Nagy?= Date: Fri, 2 Dec 2022 15:54:16 +0100 Subject: [PATCH 5/9] fix commit typo Co-authored-by: Jagoda Berry Rybacka --- background/services/chain/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/background/services/chain/index.ts b/background/services/chain/index.ts index 5cacf60a5f..2f5b2f0b59 100644 --- a/background/services/chain/index.ts +++ b/background/services/chain/index.ts @@ -1269,7 +1269,7 @@ export default class ChainService extends BaseService { if (!result) { logger.warn( - `Tx has ${hash} is found in our local registry but not on chain.` + `Tx hash ${hash} is found in our local registry but not on chain.` ) this.removeTransactionHashFromQueue(network, hash) From 428a16933f40a120d0923bc6823c30a84ebc1d95 Mon Sep 17 00:00:00 2001 From: Gergo Nagy Date: Fri, 2 Dec 2022 19:33:29 +0100 Subject: [PATCH 6/9] refactor isTransactionHashQueued into a function --- background/services/chain/index.ts | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/background/services/chain/index.ts b/background/services/chain/index.ts index 2f5b2f0b59..b7aefb776e 100644 --- a/background/services/chain/index.ts +++ b/background/services/chain/index.ts @@ -991,7 +991,7 @@ export default class ChainService extends BaseService { txHash: HexString, firstSeen: UNIXTime ): void { - const seen = this.transactionsToRetrieve.some(({ hash }) => hash === txHash) + const seen = this.isTransactionHashQueued(network, txHash) if (!seen) { // @TODO Interleave initial transaction retrieval by network @@ -999,20 +999,37 @@ export default class ChainService extends BaseService { } } - removeTransactionHashFromQueue(network: EVMNetwork, hash: HexString): void { - const seen = this.transactionsToRetrieve.some( - (queuedTx) => queuedTx.hash === hash + /** + * Checks if a transaction with a given hash on a network is in the queue or not. + * + * @param txHash The hash of a tx to check. + * @returns true if the tx hash is in the queue, false otherwise. + */ + isTransactionHashQueued(txNetwork: EVMNetwork, txHash: HexString): boolean { + return this.transactionsToRetrieve.some( + ({ hash, network }) => + hash === txHash && txNetwork.chainID === network.chainID ) + } + + /** + * Removes a particular hash from our queue. + * + * @param network The network on which the transaction has been broadcast. + * @param txHash The tx hash identifier of the transaction we want to retrieve. + */ + removeTransactionHashFromQueue(network: EVMNetwork, txHash: HexString): void { + const seen = this.isTransactionHashQueued(network, txHash) if (seen) { // Let's clean up the tx queue if the hash is present. // The pending tx hash should be on chain as soon as it's broadcasted. this.transactionsToRetrieve = this.transactionsToRetrieve.filter( - (queuedTx) => queuedTx.hash !== hash + (queuedTx) => queuedTx.hash !== txHash ) // Let's clean up the subscriptions - this.providerForNetwork(network)?.off(hash) + this.providerForNetwork(network)?.off(txHash) } } From b9b73b6698f510dddb6bfbb9ed65de0486f5e42a Mon Sep 17 00:00:00 2001 From: Gergo Nagy Date: Fri, 2 Dec 2022 19:39:39 +0100 Subject: [PATCH 7/9] refactor transaction retrieval scheduling We need to distribute the transaction requests in time otherwise it can cause a performance hit on the users's machine and rate limitations with the providers. But we need to be prepared for the background script shut down any time so we need to be in sync with what's persisted. The alarms can fire 1+ minute intervals, so we need an internal more granular timer --- background/services/chain/index.ts | 63 +++++++++++++++++------------- 1 file changed, 36 insertions(+), 27 deletions(-) diff --git a/background/services/chain/index.ts b/background/services/chain/index.ts index b7aefb776e..c36061e5e0 100644 --- a/background/services/chain/index.ts +++ b/background/services/chain/index.ts @@ -33,6 +33,7 @@ import { CHAINS_WITH_MEMPOOL, EIP_1559_COMPLIANT_CHAIN_IDS, AVALANCHE, + SECOND, } from "../../constants" import { FeatureFlags, isEnabled } from "../../features" import PreferenceService from "../preferences" @@ -67,11 +68,6 @@ import { } from "./utils/optimismGasPriceOracle" import KeyringService from "../keyring" -// How many queued transactions should be retrieved on every tx alarm, per -// network. To get frequency, divide by the alarm period. 5 tx / 5 minutes → -// max 1 tx/min. -const TRANSACTIONS_RETRIEVED_PER_ALARM = 5 - // The number of blocks to query at a time for historic asset transfers. // Unfortunately there's no "right" answer here that works well across different // people's account histories. If the number is too large relative to a @@ -192,6 +188,20 @@ export default class ChainService extends BaseService { firstSeen: UNIXTime }[] + /** + * Internal timer for the transactionsToRetrieve FIFO queue. + * Starting multiple transaction requests at the same time is resource intensive + * on the user's machine and also can result in rate limitations with the provider. + * + * Because of this we need to smooth out the retrieval scheduling. + * + * Limitations + * - handlers can fire only in 1+ minute intervals + * - in manifest v3 / service worker context the background thread can be shut down any time. + * Because of this we need to keep the granular queue tied to the persisted list of txs + */ + private transactionToRetrieveGranularTimer: NodeJS.Timer | undefined + static create: ServiceCreatorFunction< Events, ChainService, @@ -1482,32 +1492,30 @@ export default class ChainService extends BaseService { } private async handleQueuedTransactionAlarm(): Promise { - const fetchedByNetwork: { [chainID: string]: number } = {} - - // Drop all transactions that weren't retrieved from the queue. - this.transactionsToRetrieve = this.transactionsToRetrieve.filter( - ({ network, hash, firstSeen }) => { - fetchedByNetwork[network.chainID] ??= 0 - + if ( + !this.transactionToRetrieveGranularTimer && + this.transactionsToRetrieve.length + ) { + this.transactionToRetrieveGranularTimer = setInterval(() => { if ( - fetchedByNetwork[network.chainID] >= TRANSACTIONS_RETRIEVED_PER_ALARM + !this.transactionsToRetrieve.length && + this.transactionToRetrieveGranularTimer ) { - // Once a given network has hit its limit, include any additional - // transactions in the updated queue. - return true - } + clearInterval(this.transactionToRetrieveGranularTimer) + this.transactionToRetrieveGranularTimer = undefined - // If more transactions can be retrieved in this alarm, bump the count, - // retrieve the transaction, and drop from the updated queue. - fetchedByNetwork[network.chainID] += 1 + return + } - // Note: this starts retrieving TRANSACTIONS_RETRIEVED_PER_ALARM * networks at the same time - // another queue solution should come here, but one that's editable runtime. Haven't found - // a solution yet that seemed to worth the complexity. - this.retrieveTransaction({ network, hash, firstSeen }) - return false - } - ) + // TODO: balance getting txs between networks + const txToRetrieve = this.transactionsToRetrieve[0] + this.removeTransactionHashFromQueue( + txToRetrieve.network, + txToRetrieve.hash + ) + this.retrieveTransaction(txToRetrieve) + }, 2 * SECOND) + } } /** @@ -1547,6 +1555,7 @@ export default class ChainService extends BaseService { !transaction.blockHash && !transaction.blockHeight ) { + // It's a pending tx, let's subscribe to events. this.subscribeToTransactionConfirmation( transaction.network, transaction From ebc77d1675ea67fdc3eaddb6e4b8176c7f4985cc Mon Sep 17 00:00:00 2001 From: Gergo Nagy Date: Wed, 7 Dec 2022 13:44:25 +0100 Subject: [PATCH 8/9] add tests for queued transaction retrieve --- background/services/chain/index.ts | 25 +++-- .../services/chain/tests/index.unit.test.ts | 97 ++++++++++++++++++- background/tests/factories.ts | 54 ++++++++++- 3 files changed, 158 insertions(+), 18 deletions(-) diff --git a/background/services/chain/index.ts b/background/services/chain/index.ts index c36061e5e0..1cf65c5ea0 100644 --- a/background/services/chain/index.ts +++ b/background/services/chain/index.ts @@ -125,6 +125,12 @@ interface Events extends ServiceLifecycleEvents { blockPrices: { blockPrices: BlockPrices; network: EVMNetwork } } +export type QueuedTxToRetrieve = { + network: EVMNetwork + hash: HexString + firstSeen: UNIXTime +} + /** * ChainService is responsible for basic network monitoring and interaction. * Other services rely on the chain service rather than polling networks @@ -182,11 +188,7 @@ export default class ChainService extends BaseService { * cached, alongside information about when that hash request was first seen * for expiration purposes. */ - private transactionsToRetrieve: { - network: EVMNetwork - hash: HexString - firstSeen: UNIXTime - }[] + private transactionsToRetrieve: QueuedTxToRetrieve[] /** * Internal timer for the transactionsToRetrieve FIFO queue. @@ -1037,9 +1039,6 @@ export default class ChainService extends BaseService { this.transactionsToRetrieve = this.transactionsToRetrieve.filter( (queuedTx) => queuedTx.hash !== txHash ) - - // Let's clean up the subscriptions - this.providerForNetwork(network)?.off(txHash) } } @@ -1300,6 +1299,8 @@ export default class ChainService extends BaseService { ) this.removeTransactionHashFromQueue(network, hash) + // Let's clean up the subscriptions + this.providerForNetwork(network)?.off(hash) const savedTx = await this.db.getTransaction(network, hash) if (savedTx && !("status" in savedTx)) { @@ -1501,9 +1502,9 @@ export default class ChainService extends BaseService { !this.transactionsToRetrieve.length && this.transactionToRetrieveGranularTimer ) { + // Clean up if we have a timer, but we don't have anything in the queue clearInterval(this.transactionToRetrieveGranularTimer) this.transactionToRetrieveGranularTimer = undefined - return } @@ -1533,11 +1534,7 @@ export default class ChainService extends BaseService { network, hash, firstSeen, - }: { - network: EVMNetwork - hash: string - firstSeen: number - }): Promise { + }: QueuedTxToRetrieve): Promise { try { const result = await this.getOrCancelTransaction(network, hash) diff --git a/background/services/chain/tests/index.unit.test.ts b/background/services/chain/tests/index.unit.test.ts index c52e9f23ef..966b54aa7b 100644 --- a/background/services/chain/tests/index.unit.test.ts +++ b/background/services/chain/tests/index.unit.test.ts @@ -1,12 +1,13 @@ import sinon from "sinon" -import ChainService from ".." -import { ETHEREUM, MINUTE, OPTIMISM, POLYGON } from "../../../constants" +import ChainService, { QueuedTxToRetrieve } from ".." +import { ETHEREUM, MINUTE, OPTIMISM, POLYGON, SECOND } from "../../../constants" import { EVMNetwork } from "../../../networks" import * as gas from "../../../lib/gas" import { createAddressOnNetwork, createBlockPrices, createChainService, + createTransactionsToRetrieve, } from "../../../tests/factories" import { UNIXTime } from "../../../types" import { @@ -29,6 +30,10 @@ type ChainServiceExternalized = Omit & { addressNetwork: AddressOnNetwork, incomingOnly: boolean ) => Promise + retrieveTransaction: (queuedTx: QueuedTxToRetrieve) => Promise + transactionsToRetrieve: QueuedTxToRetrieve[] + handleQueuedTransactionAlarm: () => Promise + transactionToRetrieveGranularTimer: NodeJS.Timer | undefined } describe("Chain Service", () => { @@ -228,4 +233,92 @@ describe("Chain Service", () => { expect(activeNetworksMock).toEqual([ETHEREUM, POLYGON]) }) }) + describe("Queued Transaction Retrieve", () => { + describe("handleQueuedTransactionAlarm", () => { + let clock: sinon.SinonFakeTimers + let setIntervalSpy: sinon.SinonSpy + let chainServiceExternalized: ChainServiceExternalized + let retrieveTransactionStub: sinon.SinonStub + beforeEach(() => { + clock = sinon.useFakeTimers() + + setIntervalSpy = sinon.spy(global, "setInterval") + + chainServiceExternalized = + chainService as unknown as ChainServiceExternalized + + retrieveTransactionStub = sandbox.stub( + chainServiceExternalized, + "retrieveTransaction" + ) + }) + afterEach(() => { + clock.restore() + }) + it("should not start the granular timer if the queue is empty", () => { + chainServiceExternalized.handleQueuedTransactionAlarm() + + clock.tick(2 * SECOND) + + expect(setIntervalSpy.calledOnce).toBe(false) + expect(retrieveTransactionStub.callCount).toBe(0) + expect(chainServiceExternalized.transactionsToRetrieve.length).toBe(0) + }) + it("should not recreate the timer when the alarm fires periodically", () => { + const clearIntervalSpy = sinon.spy(global, "clearInterval") + + chainServiceExternalized.transactionsToRetrieve = + createTransactionsToRetrieve(100) + + chainServiceExternalized.handleQueuedTransactionAlarm() + clock.tick(60 * SECOND) + chainServiceExternalized.handleQueuedTransactionAlarm() + clock.tick(60 * SECOND) + chainServiceExternalized.handleQueuedTransactionAlarm() + clock.tick(60 * SECOND) + + expect(setIntervalSpy.calledOnce).toBe(true) + expect(clearIntervalSpy.calledOnce).toBe(false) + }) + it("should retrieve 1 tx every 2 seconds, remove the tx from the queue and call the retrieve function", async () => { + const txInQueueCount = 100 + const txRetrievedCount = 98 + + chainServiceExternalized.transactionsToRetrieve = + createTransactionsToRetrieve(txInQueueCount) + + chainServiceExternalized.handleQueuedTransactionAlarm() + + clock.tick(txRetrievedCount * 2 * SECOND) + + expect(retrieveTransactionStub.callCount).toBe(txRetrievedCount) + expect(chainServiceExternalized.transactionsToRetrieve.length).toBe( + txInQueueCount - txRetrievedCount + ) + }) + it("should clean up the timer after the queue is emptied", async () => { + const clearIntervalSpy = sinon.spy(global, "clearInterval") + const numberOfTxInQueue = 100 + + chainServiceExternalized.transactionsToRetrieve = + createTransactionsToRetrieve(numberOfTxInQueue) + + chainServiceExternalized.handleQueuedTransactionAlarm() + + clock.tick(numberOfTxInQueue * 2 * SECOND) + + expect(setIntervalSpy.calledOnce).toBe(true) + expect(retrieveTransactionStub.callCount).toBe(numberOfTxInQueue) + expect(chainServiceExternalized.transactionsToRetrieve.length).toBe(0) + + // the clean up happens on the next tick + clock.tick(2 * SECOND) + + expect(clearIntervalSpy.calledOnce).toBe(true) + expect( + chainServiceExternalized.transactionToRetrieveGranularTimer + ).toBe(undefined) + }) + }) + }) }) diff --git a/background/tests/factories.ts b/background/tests/factories.ts index b425076664..5ab1c878ab 100644 --- a/background/tests/factories.ts +++ b/background/tests/factories.ts @@ -1,10 +1,22 @@ /* eslint-disable class-methods-use-this */ -import { Block, FeeData } from "@ethersproject/abstract-provider" +import { + Block, + FeeData, + TransactionReceipt, + TransactionResponse, +} from "@ethersproject/abstract-provider" import { DexieOptions } from "dexie" import { BigNumber } from "ethers" import { keccak256 } from "ethers/lib/utils" import { AccountBalance, AddressOnNetwork } from "../accounts" -import { ETH, ETHEREUM, OPTIMISM } from "../constants" +import { + ARBITRUM_ONE, + AVALANCHE, + ETH, + ETHEREUM, + OPTIMISM, + POLYGON, +} from "../constants" import { AnyEVMTransaction, LegacyEVMTransactionRequest, @@ -20,6 +32,7 @@ import { PreferenceService, SigningService, } from "../services" +import { QueuedTxToRetrieve } from "../services/chain" import SerialFallbackProvider from "../services/chain/serial-fallback-provider" const createRandom0xHash = () => @@ -206,6 +219,43 @@ export const createBlockPrices = ( ...overrides, }) +export const createQueuedTransaction = ( + overrides: Partial = {} +): QueuedTxToRetrieve => ({ + network: OPTIMISM, + hash: createRandom0xHash(), + firstSeen: Date.now(), + ...overrides, +}) + +export const createTransactionsToRetrieve = ( + numberOfTx = 100 +): QueuedTxToRetrieve[] => { + const NETWORKS = [ETHEREUM, POLYGON, ARBITRUM_ONE, AVALANCHE, OPTIMISM] + + return [...Array(numberOfTx).keys()].map((_, ind) => + createQueuedTransaction({ network: NETWORKS[ind % NETWORKS.length] }) + ) +} + +export const createTransactionResponse = ( + overrides: Partial = {} +): TransactionResponse => ({ + hash: createRandom0xHash(), + blockNumber: 25639147, + blockHash: createRandom0xHash(), + timestamp: Date.now(), + confirmations: 0, + from: createRandom0xHash(), + nonce: 570, + gasLimit: BigNumber.from(15000000), + data: "...", + value: BigNumber.from(15000000), + chainId: Number(OPTIMISM.chainID), + wait: () => Promise.resolve({} as TransactionReceipt), + ...overrides, +}) + export const makeEthersBlock = (overrides?: Partial): Block => { return { hash: "0x20567436620bf18c07cf34b3ec4af3e530d7a2391d7a87fb0661565186f4e834", From 51c0952871dfd5e0e389689c01d387fb312f83ce Mon Sep 17 00:00:00 2001 From: Gergo Nagy Date: Wed, 7 Dec 2022 14:42:05 +0100 Subject: [PATCH 9/9] remove unused wait function --- background/services/chain/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/background/services/chain/index.ts b/background/services/chain/index.ts index 0dc6b815a6..ce4bfdf47e 100644 --- a/background/services/chain/index.ts +++ b/background/services/chain/index.ts @@ -51,7 +51,7 @@ import { ethersTransactionFromTransactionRequest, unsignedTransactionFromEVMTransaction, } from "./utils" -import { normalizeEVMAddress, sameEVMAddress, wait } from "../../lib/utils" +import { normalizeEVMAddress, sameEVMAddress } from "../../lib/utils" import type { EnrichedEIP1559TransactionRequest, EnrichedEIP1559TransactionSignatureRequest,