From e78b6d61f205294a45d360909a9c07466f993429 Mon Sep 17 00:00:00 2001 From: Moody Salem Date: Sun, 26 Jul 2020 18:07:02 -0500 Subject: [PATCH] improvement(transactions): some clean up and unit tests - fetch transaction state less often for old transactions - fix a bug calling non payable methods with value 0 --- src/connectors/index.ts | 3 +- src/hooks/useSwapCallback.ts | 8 +- src/state/transactions/actions.ts | 16 ++- src/state/transactions/hooks.tsx | 4 +- src/state/transactions/reducer.test.ts | 192 +++++++++++++++++++++++++ src/state/transactions/reducer.ts | 53 ++++--- src/state/transactions/updater.test.ts | 35 +++++ src/state/transactions/updater.tsx | 37 ++++- src/theme/index.tsx | 1 + 9 files changed, 312 insertions(+), 37 deletions(-) create mode 100644 src/state/transactions/reducer.test.ts create mode 100644 src/state/transactions/updater.test.ts diff --git a/src/connectors/index.ts b/src/connectors/index.ts index b993853d1c..fe62211a56 100644 --- a/src/connectors/index.ts +++ b/src/connectors/index.ts @@ -6,7 +6,6 @@ import { PortisConnector } from '@web3-react/portis-connector' import { FortmaticConnector } from './Fortmatic' import { NetworkConnector } from './NetworkConnector' -const POLLING_INTERVAL = 12500 const NETWORK_URL = process.env.REACT_APP_NETWORK_URL const FORMATIC_KEY = process.env.REACT_APP_FORTMATIC_KEY const PORTIS_ID = process.env.REACT_APP_PORTIS_ID @@ -28,7 +27,7 @@ export const walletconnect = new WalletConnectConnector({ rpc: { 1: NETWORK_URL }, bridge: 'https://bridge.walletconnect.org', qrcode: true, - pollingInterval: POLLING_INTERVAL + pollingInterval: 15000 }) // mainnet only diff --git a/src/hooks/useSwapCallback.ts b/src/hooks/useSwapCallback.ts index 946b01b72a..2a1431329c 100644 --- a/src/hooks/useSwapCallback.ts +++ b/src/hooks/useSwapCallback.ts @@ -12,6 +12,10 @@ import { useV1ExchangeContract } from './useContract' import useENS from './useENS' import { Version } from './useToggledVersion' +function isZero(hexNumber: string) { + return /^0x0*$/.test(hexNumber) +} + // returns a function that will execute a swap, if the parameters are all valid // and the user has approved the slippage adjusted input amount for the trade export function useSwapCallback( @@ -76,7 +80,7 @@ export function useSwapCallback( const safeGasEstimates: (BigNumber | undefined)[] = await Promise.all( swapMethods.map(({ args, methodName, value }) => - contract.estimateGas[methodName](...args, value ? { value } : {}) + contract.estimateGas[methodName](...args, value && !isZero(value) ? { value } : {}) .then(calculateGasMargin) .catch(error => { console.error(`estimateGas failed for ${methodName}`, error) @@ -127,7 +131,7 @@ export function useSwapCallback( return contract[methodName](...args, { gasLimit: safeGasEstimate, - ...(value ? { value } : {}) + ...(value && !isZero(value) ? { value } : {}) }) .then((response: any) => { const inputSymbol = trade.inputAmount.currency.symbol diff --git a/src/state/transactions/actions.ts b/src/state/transactions/actions.ts index af5792a405..324e02016a 100644 --- a/src/state/transactions/actions.ts +++ b/src/state/transactions/actions.ts @@ -1,4 +1,5 @@ import { createAction } from '@reduxjs/toolkit' +import { ChainId } from '@uniswap/sdk' export interface SerializableTransactionReceipt { to: string @@ -12,15 +13,20 @@ export interface SerializableTransactionReceipt { } export const addTransaction = createAction<{ - chainId: number + chainId: ChainId hash: string from: string approval?: { tokenAddress: string; spender: string } summary?: string -}>('addTransaction') -export const clearAllTransactions = createAction<{ chainId: number }>('clearAllTransactions') +}>('transactions/addTransaction') +export const clearAllTransactions = createAction<{ chainId: ChainId }>('transactions/clearAllTransactions') export const finalizeTransaction = createAction<{ - chainId: number + chainId: ChainId hash: string receipt: SerializableTransactionReceipt -}>('finalizeTransaction') +}>('transactions/finalizeTransaction') +export const checkedTransaction = createAction<{ + chainId: ChainId + hash: string + blockNumber: number +}>('transactions/checkedTransaction') diff --git a/src/state/transactions/hooks.tsx b/src/state/transactions/hooks.tsx index db392e81fc..3a7f248613 100644 --- a/src/state/transactions/hooks.tsx +++ b/src/state/transactions/hooks.tsx @@ -5,7 +5,7 @@ import { useDispatch, useSelector } from 'react-redux' import { useActiveWeb3React } from '../../hooks' import { AppDispatch, AppState } from '../index' import { addTransaction } from './actions' -import { TransactionDetails, TransactionState } from './reducer' +import { TransactionDetails } from './reducer' // helper that can take a ethers library transaction response and add it to the list of transactions export function useTransactionAdder(): ( @@ -37,7 +37,7 @@ export function useTransactionAdder(): ( export function useAllTransactions(): { [txHash: string]: TransactionDetails } { const { chainId } = useActiveWeb3React() - const state = useSelector(state => state.transactions) + const state = useSelector(state => state.transactions) return chainId ? state[chainId] ?? {} : {} } diff --git a/src/state/transactions/reducer.test.ts b/src/state/transactions/reducer.test.ts new file mode 100644 index 0000000000..941d48c7a3 --- /dev/null +++ b/src/state/transactions/reducer.test.ts @@ -0,0 +1,192 @@ +import { ChainId } from '@uniswap/sdk' +import { createStore, Store } from 'redux' +import { addTransaction, checkedTransaction, clearAllTransactions, finalizeTransaction } from './actions' +import reducer, { initialState, TransactionState } from './reducer' + +describe('transaction reducer', () => { + let store: Store + + beforeEach(() => { + store = createStore(reducer, initialState) + }) + + describe('addTransaction', () => { + it('adds the transaction', () => { + const beforeTime = new Date().getTime() + store.dispatch( + addTransaction({ + chainId: ChainId.MAINNET, + summary: 'hello world', + hash: '0x0', + approval: { tokenAddress: 'abc', spender: 'def' }, + from: 'abc' + }) + ) + const txs = store.getState() + expect(txs[ChainId.MAINNET]).toBeTruthy() + expect(txs[ChainId.MAINNET]?.['0x0']).toBeTruthy() + const tx = txs[ChainId.MAINNET]?.['0x0'] + expect(tx).toBeTruthy() + expect(tx?.hash).toEqual('0x0') + expect(tx?.summary).toEqual('hello world') + expect(tx?.approval).toEqual({ tokenAddress: 'abc', spender: 'def' }) + expect(tx?.from).toEqual('abc') + expect(tx?.addedTime).toBeGreaterThanOrEqual(beforeTime) + }) + }) + + describe('finalizeTransaction', () => { + it('no op if not valid transaction', () => { + store.dispatch( + finalizeTransaction({ + chainId: ChainId.RINKEBY, + hash: '0x0', + receipt: { + status: 1, + transactionIndex: 1, + transactionHash: '0x0', + to: '0x0', + from: '0x0', + contractAddress: '0x0', + blockHash: '0x0', + blockNumber: 1 + } + }) + ) + expect(store.getState()).toEqual({}) + }) + it('sets receipt', () => { + store.dispatch( + addTransaction({ + hash: '0x0', + chainId: ChainId.RINKEBY, + approval: { spender: '0x0', tokenAddress: '0x0' }, + summary: 'hello world', + from: '0x0' + }) + ) + const beforeTime = new Date().getTime() + store.dispatch( + finalizeTransaction({ + chainId: ChainId.RINKEBY, + hash: '0x0', + receipt: { + status: 1, + transactionIndex: 1, + transactionHash: '0x0', + to: '0x0', + from: '0x0', + contractAddress: '0x0', + blockHash: '0x0', + blockNumber: 1 + } + }) + ) + const tx = store.getState()[ChainId.RINKEBY]?.['0x0'] + expect(tx?.summary).toEqual('hello world') + expect(tx?.confirmedTime).toBeGreaterThanOrEqual(beforeTime) + expect(tx?.receipt).toEqual({ + status: 1, + transactionIndex: 1, + transactionHash: '0x0', + to: '0x0', + from: '0x0', + contractAddress: '0x0', + blockHash: '0x0', + blockNumber: 1 + }) + }) + }) + + describe('checkedTransaction', () => { + it('no op if not valid transaction', () => { + store.dispatch( + checkedTransaction({ + chainId: ChainId.RINKEBY, + hash: '0x0', + blockNumber: 1 + }) + ) + expect(store.getState()).toEqual({}) + }) + it('sets lastCheckedBlockNumber', () => { + store.dispatch( + addTransaction({ + hash: '0x0', + chainId: ChainId.RINKEBY, + approval: { spender: '0x0', tokenAddress: '0x0' }, + summary: 'hello world', + from: '0x0' + }) + ) + store.dispatch( + checkedTransaction({ + chainId: ChainId.RINKEBY, + hash: '0x0', + blockNumber: 1 + }) + ) + const tx = store.getState()[ChainId.RINKEBY]?.['0x0'] + expect(tx?.lastCheckedBlockNumber).toEqual(1) + }) + it('never decreases', () => { + store.dispatch( + addTransaction({ + hash: '0x0', + chainId: ChainId.RINKEBY, + approval: { spender: '0x0', tokenAddress: '0x0' }, + summary: 'hello world', + from: '0x0' + }) + ) + store.dispatch( + checkedTransaction({ + chainId: ChainId.RINKEBY, + hash: '0x0', + blockNumber: 3 + }) + ) + store.dispatch( + checkedTransaction({ + chainId: ChainId.RINKEBY, + hash: '0x0', + blockNumber: 1 + }) + ) + const tx = store.getState()[ChainId.RINKEBY]?.['0x0'] + expect(tx?.lastCheckedBlockNumber).toEqual(3) + }) + }) + + describe('clearAllTransactions', () => { + it('removes all transactions for the chain', () => { + store.dispatch( + addTransaction({ + chainId: ChainId.MAINNET, + summary: 'hello world', + hash: '0x0', + approval: { tokenAddress: 'abc', spender: 'def' }, + from: 'abc' + }) + ) + store.dispatch( + addTransaction({ + chainId: ChainId.RINKEBY, + summary: 'hello world', + hash: '0x1', + approval: { tokenAddress: 'abc', spender: 'def' }, + from: 'abc' + }) + ) + expect(Object.keys(store.getState())).toHaveLength(2) + expect(Object.keys(store.getState())).toEqual([String(ChainId.MAINNET), String(ChainId.RINKEBY)]) + expect(Object.keys(store.getState()[ChainId.MAINNET] ?? {})).toEqual(['0x0']) + expect(Object.keys(store.getState()[ChainId.RINKEBY] ?? {})).toEqual(['0x1']) + store.dispatch(clearAllTransactions({ chainId: ChainId.MAINNET })) + expect(Object.keys(store.getState())).toHaveLength(2) + expect(Object.keys(store.getState())).toEqual([String(ChainId.MAINNET), String(ChainId.RINKEBY)]) + expect(Object.keys(store.getState()[ChainId.MAINNET] ?? {})).toEqual([]) + expect(Object.keys(store.getState()[ChainId.RINKEBY] ?? {})).toEqual(['0x1']) + }) + }) +}) diff --git a/src/state/transactions/reducer.ts b/src/state/transactions/reducer.ts index 5315b11529..de5a91a65e 100644 --- a/src/state/transactions/reducer.ts +++ b/src/state/transactions/reducer.ts @@ -1,5 +1,11 @@ import { createReducer } from '@reduxjs/toolkit' -import { addTransaction, clearAllTransactions, finalizeTransaction, SerializableTransactionReceipt } from './actions' +import { + addTransaction, + checkedTransaction, + clearAllTransactions, + finalizeTransaction, + SerializableTransactionReceipt +} from './actions' const now = () => new Date().getTime() @@ -8,12 +14,10 @@ export interface TransactionDetails { approval?: { tokenAddress: string; spender: string } summary?: string receipt?: SerializableTransactionReceipt + lastCheckedBlockNumber?: number addedTime: number confirmedTime?: number from: string - - // set to true when we receive a transaction count that exceeds the nonce of this transaction - unknownStatus?: boolean } export interface TransactionState { @@ -22,28 +26,39 @@ export interface TransactionState { } } -const initialState: TransactionState = {} +export const initialState: TransactionState = {} export default createReducer(initialState, builder => builder - .addCase(addTransaction, (state, { payload: { chainId, from, hash, approval, summary } }) => { - if (state[chainId]?.[hash]) { + .addCase(addTransaction, (transactions, { payload: { chainId, from, hash, approval, summary } }) => { + if (transactions[chainId]?.[hash]) { throw Error('Attempted to add existing transaction.') } - state[chainId] = state[chainId] ?? {} - state[chainId][hash] = { hash, approval, summary, from, addedTime: now() } + const txs = transactions[chainId] ?? {} + txs[hash] = { hash, approval, summary, from, addedTime: now() } + transactions[chainId] = txs }) - .addCase(clearAllTransactions, (state, { payload: { chainId } }) => { - if (!state[chainId]) return - state[chainId] = {} + .addCase(clearAllTransactions, (transactions, { payload: { chainId } }) => { + if (!transactions[chainId]) return + transactions[chainId] = {} }) - .addCase(finalizeTransaction, (state, { payload: { hash, chainId, receipt } }) => { - if (!state[chainId]?.[hash]) { - throw Error('Attempted to finalize non-existent transaction.') + .addCase(checkedTransaction, (transactions, { payload: { chainId, hash, blockNumber } }) => { + const tx = transactions[chainId]?.[hash] + if (!tx) { + return } - state[chainId] = state[chainId] ?? {} - state[chainId][hash].receipt = receipt - state[chainId][hash].unknownStatus = false - state[chainId][hash].confirmedTime = now() + if (!tx.lastCheckedBlockNumber) { + tx.lastCheckedBlockNumber = blockNumber + } else { + tx.lastCheckedBlockNumber = Math.max(blockNumber, tx.lastCheckedBlockNumber) + } + }) + .addCase(finalizeTransaction, (transactions, { payload: { hash, chainId, receipt } }) => { + const tx = transactions[chainId]?.[hash] + if (!tx) { + return + } + tx.receipt = receipt + tx.confirmedTime = now() }) ) diff --git a/src/state/transactions/updater.test.ts b/src/state/transactions/updater.test.ts new file mode 100644 index 0000000000..45f5d46fdc --- /dev/null +++ b/src/state/transactions/updater.test.ts @@ -0,0 +1,35 @@ +import { shouldCheck } from './updater' + +describe('transactions updater', () => { + describe('shouldCheck', () => { + it('returns true if no receipt and never checked', () => { + expect(shouldCheck(10, { addedTime: 100 })).toEqual(true) + }) + it('returns false if has receipt and never checked', () => { + expect(shouldCheck(10, { addedTime: 100, receipt: {} })).toEqual(false) + }) + it('returns true if has not been checked in 1 blocks', () => { + expect(shouldCheck(10, { addedTime: new Date().getTime(), lastCheckedBlockNumber: 9 })).toEqual(true) + }) + it('returns false if checked in last 3 blocks and greater than 20 minutes old', () => { + expect(shouldCheck(10, { addedTime: new Date().getTime() - 21 * 60 * 1000, lastCheckedBlockNumber: 8 })).toEqual( + false + ) + }) + it('returns true if not checked in last 5 blocks and greater than 20 minutes old', () => { + expect(shouldCheck(10, { addedTime: new Date().getTime() - 21 * 60 * 1000, lastCheckedBlockNumber: 5 })).toEqual( + true + ) + }) + it('returns false if checked in last 10 blocks and greater than 60 minutes old', () => { + expect(shouldCheck(20, { addedTime: new Date().getTime() - 61 * 60 * 1000, lastCheckedBlockNumber: 11 })).toEqual( + false + ) + }) + it('returns true if checked in last 3 blocks and greater than 20 minutes old', () => { + expect(shouldCheck(20, { addedTime: new Date().getTime() - 61 * 60 * 1000, lastCheckedBlockNumber: 10 })).toEqual( + true + ) + }) + }) +}) diff --git a/src/state/transactions/updater.tsx b/src/state/transactions/updater.tsx index 8fb0e9ce25..8adb6a7e29 100644 --- a/src/state/transactions/updater.tsx +++ b/src/state/transactions/updater.tsx @@ -3,7 +3,28 @@ import { useDispatch, useSelector } from 'react-redux' import { useActiveWeb3React } from '../../hooks' import { useAddPopup, useBlockNumber } from '../application/hooks' import { AppDispatch, AppState } from '../index' -import { finalizeTransaction } from './actions' +import { checkedTransaction, finalizeTransaction } from './actions' + +export function shouldCheck( + lastBlockNumber: number, + tx: { addedTime: number; receipt?: {}; lastCheckedBlockNumber?: number } +): boolean { + if (tx.receipt) return false + if (!tx.lastCheckedBlockNumber) return true + const blocksSinceCheck = lastBlockNumber - tx.lastCheckedBlockNumber + if (blocksSinceCheck < 1) return false + const minutesPending = (new Date().getTime() - tx.addedTime) / 1000 / 60 + if (minutesPending > 60) { + // every 10 blocks if pending for longer than an hour + return blocksSinceCheck > 9 + } else if (minutesPending > 5) { + // every 3 blocks if pending more than 5 minutes + return blocksSinceCheck > 2 + } else { + // otherwise every block + return true + } +} export default function Updater() { const { chainId, library } = useActiveWeb3React() @@ -11,9 +32,9 @@ export default function Updater() { const lastBlockNumber = useBlockNumber() const dispatch = useDispatch() - const transactions = useSelector(state => state.transactions) + const state = useSelector(state => state.transactions) - const allTransactions = chainId ? transactions[chainId] ?? {} : {} + const transactions = chainId ? state[chainId] ?? {} : {} // show popup on confirm const addPopup = useAddPopup() @@ -21,8 +42,8 @@ export default function Updater() { useEffect(() => { if (!chainId || !library || !lastBlockNumber) return - Object.keys(allTransactions) - .filter(hash => !allTransactions[hash].receipt) + Object.keys(transactions) + .filter(hash => shouldCheck(lastBlockNumber, transactions[hash])) .forEach(hash => { library .getTransactionReceipt(hash) @@ -50,18 +71,20 @@ export default function Updater() { txn: { hash, success: receipt.status === 1, - summary: allTransactions[hash]?.summary + summary: transactions[hash]?.summary } }, hash ) + } else { + dispatch(checkedTransaction({ chainId, hash, blockNumber: lastBlockNumber })) } }) .catch(error => { console.error(`failed to check transaction hash: ${hash}`, error) }) }) - }, [chainId, library, allTransactions, lastBlockNumber, dispatch, addPopup]) + }, [chainId, library, transactions, lastBlockNumber, dispatch, addPopup]) return null } diff --git a/src/theme/index.tsx b/src/theme/index.tsx index 66a494c117..abf74bb059 100644 --- a/src/theme/index.tsx +++ b/src/theme/index.tsx @@ -175,6 +175,7 @@ html, input, textarea, button { @supports (font-variation-settings: normal) { html, input, textarea, button { font-family: 'Inter var', sans-serif; + font-display: fallback; } }