Fix: safeTxGasEstimation race condition (#3683)

* Fix: safeTxGasEstimation race condition

* Fix tests

* Add useAsync tests

* Rm redundant check

* Don't estimate safeTxGas if no txData

* Set baseGas and gasPrice in createTransaction

* Pass safeTxGas to gas limit estimation

* Remove pedning if receipt status false

* Restore getRecommendedNonce

* Fix tests

* Revert pending watcher changes

* Add a warning on tx fail

src/logic/hooks/__tests__/useAsync.test.ts

@@ -0,0 +1,63 @@
+import { renderHook } from '@testing-library/react-hooks'
+import useAsync from '../useAsync'
+
+describe('useAsync tests', () => {
+  it('returns a successful result', async () => {
+    const fakeCallback = jest.fn(() => Promise.resolve('success'))
+
+    const { result, waitForNextUpdate } = renderHook(() => useAsync(fakeCallback))
+
+    await waitForNextUpdate()
+
+    expect(result.current).toEqual({ result: 'success', error: undefined })
+
+    expect(fakeCallback).toHaveBeenCalledTimes(1)
+  })
+
+  it('returns an error', async () => {
+    const fakeCallback = jest.fn(() => Promise.reject(new Error('failure')))
+
+    const { result, waitForNextUpdate } = renderHook(() => useAsync(fakeCallback))
+
+    await waitForNextUpdate()
+
+    expect(result.current.result).toBe(undefined)
+    expect(result.current.error).toBeDefined()
+    expect(result.current.error!.message).toBe('failure')
+
+    expect(fakeCallback).toHaveBeenCalledTimes(1)
+  })
+
+  it('ignores a stale result if a new request has been launched already', async () => {
+    // This will resolve AFTER the second callback but the result should be ignored
+    const fakeCallback1 = jest.fn(() => {
+      return new Promise((resolve) => {
+        setTimeout(() => {
+          resolve('success 1')
+        }, 10)
+      })
+    })
+
+    const fakeCallback2 = jest.fn(() => Promise.resolve('success 2'))
+
+    let counter = 0
+    const { result, waitForNextUpdate, rerender } = renderHook(() => {
+      const callback = counter > 0 ? fakeCallback2 : fakeCallback1
+      counter += 1
+      return useAsync(callback)
+    })
+
+    expect(result.current.result).toBe(undefined)
+    expect(result.current.error).toBe(undefined)
+
+    rerender() // re-render immediately
+
+    await waitForNextUpdate()
+
+    expect(result.current.result).toBe('success 2')
+    expect(result.current.error).toBe(undefined)
+
+    expect(fakeCallback1).toHaveBeenCalledTimes(1)
+    expect(fakeCallback2).toHaveBeenCalledTimes(1)
+  })
+})

src/logic/hooks/__tests__/useEstimateSafeTxGas.test.ts

@@ -1,5 +1,5 @@
+import { renderHook, act } from '@testing-library/react-hooks'
 import { useEstimateSafeTxGas } from 'src/logic/hooks/useEstimateSafeTxGas'
-import { renderHook } from '@testing-library/react-hooks'
 import * as gas from 'src/logic/safe/transactions/gas'
 
 jest.mock('react-redux', () => {
@@ -10,69 +10,111 @@ jest.mock('react-redux', () => {
   }
 })
 
+const actResolve = async (callback: () => unknown): Promise<void> => {
+  await act(() => {
+    callback()
+    return new Promise((reslolve) => {
+      setTimeout(reslolve, 0)
+    })
+  })
+}
+
 describe('useEstimateSafeTxGas', () => {
-  it(`should return 0 if it is not a tx creation`, () => {
+  it(`should return 0 if it is not a tx creation`, async () => {
+    const spy = jest.spyOn(gas, 'estimateSafeTxGas')
+
+    await actResolve(() => {
+      const { result } = renderHook(() =>
+        useEstimateSafeTxGas({
+          txAmount: '',
+          txData: '0x',
+          txRecipient: '',
+          isCreation: false,
+          isRejectTx: false,
+        }),
+      )
+
+      expect(result.current).toBe('0')
+    })
+
+    expect(spy).toHaveBeenCalledTimes(0)
+  })
+
+  it(`should return 0 if it is a reject tx`, async () => {
     const spy = jest.spyOn(gas, 'estimateSafeTxGas')
 
-    const { result } = renderHook(() =>
-      useEstimateSafeTxGas({
-        txAmount: '',
-        txData: '',
-        txRecipient: '',
-        isCreation: false,
-        isRejectTx: false,
-      }),
-    )
-    expect(result.current).toBe('0')
+    await actResolve(() => {
+      const { result } = renderHook(() =>
+        useEstimateSafeTxGas({
+          txAmount: '',
+          txData: '0x',
+          txRecipient: '',
+          isCreation: false,
+          isRejectTx: true,
+        }),
+      )
+      expect(result.current).toBe('0')
+    })
+
     expect(spy).toHaveBeenCalledTimes(0)
   })
 
-  it(`should return 0 if it is a reject tx`, () => {
+  it(`should return 0 if tx data is not passed`, async () => {
     const spy = jest.spyOn(gas, 'estimateSafeTxGas')
 
-    const { result } = renderHook(() =>
-      useEstimateSafeTxGas({
-        txAmount: '',
-        txData: '',
-        txRecipient: '',
-        isCreation: false,
-        isRejectTx: true,
-      }),
-    )
-    expect(result.current).toBe('0')
+    await actResolve(() => {
+      const { result } = renderHook(() =>
+        useEstimateSafeTxGas({
+          txAmount: '',
+          txData: '',
+          txRecipient: '',
+          isCreation: true,
+          isRejectTx: false,
+        }),
+      )
+
+      expect(result.current).toBe('0')
+    })
+
     expect(spy).toHaveBeenCalledTimes(0)
   })
 
-  it(`calls estimateSafeTxGas if it is a tx creation`, () => {
+  it(`calls estimateSafeTxGas if it is a tx creation`, async () => {
     const spy = jest.spyOn(gas, 'estimateSafeTxGas')
 
-    renderHook(() =>
-      useEstimateSafeTxGas({
-        txAmount: '',
-        txData: '',
-        txRecipient: '',
-        isCreation: true,
-        isRejectTx: false,
-      }),
-    )
+    await actResolve(() => {
+      renderHook(() =>
+        useEstimateSafeTxGas({
+          txAmount: '',
+          txData: '0x',
+          txRecipient: '',
+          isCreation: true,
+          isRejectTx: false,
+        }),
+      )
+    })
+
     expect(spy).toHaveBeenCalledTimes(1)
   })
 
-  it(`returns 0 if estimateSafeTxGas throws`, () => {
+  it(`returns 0 if estimateSafeTxGas throws`, async () => {
     const spy = jest.spyOn(gas, 'estimateSafeTxGas').mockImplementation(() => {
       throw new Error()
     })
 
-    const { result } = renderHook(() =>
-      useEstimateSafeTxGas({
-        txAmount: '',
-        txData: '',
-        txRecipient: '',
-        isCreation: true,
-        isRejectTx: false,
-      }),
-    )
+    await actResolve(() => {
+      const { result } = renderHook(() =>
+        useEstimateSafeTxGas({
+          txAmount: '',
+          txData: '0x',
+          txRecipient: '',
+          isCreation: true,
+          isRejectTx: false,
+        }),
+      )
+      expect(result.current).toBe('0')
+    })
+
     expect(spy).toHaveBeenCalledTimes(1)
-    expect(result.current).toBe('0')
   })
 })

src/logic/hooks/useAsync.ts

@@ -0,0 +1,34 @@
+import { useEffect, useState } from 'react'
+
+type AsyncResult<T> = {
+  error: Error | undefined
+  result: T | undefined
+}
+
+const useAsync = <T>(asyncCall: () => Promise<T>): AsyncResult<T> => {
+  const [asyncVal, setAsyncVal] = useState<T>()
+  const [err, setErr] = useState<Error>()
+
+  useEffect(() => {
+    let isCurrent = true
+
+    setAsyncVal(undefined)
+    setErr(undefined)
+
+    asyncCall()
+      .then((val: T) => {
+        if (isCurrent) setAsyncVal(val)
+      })
+      .catch((error) => {
+        if (isCurrent) setErr(error)
+      })
+
+    return () => {
+      isCurrent = false
+    }
+  }, [asyncCall])
+
+  return { error: err, result: asyncVal }
+}
+
+export default useAsync

src/logic/hooks/useEstimateSafeTxGas.tsx

@@ -1,9 +1,10 @@
 import { Operation } from '@gnosis.pm/safe-react-gateway-sdk'
-import { useEffect, useState } from 'react'
+import { useCallback } from 'react'
 import { useSelector } from 'react-redux'
 
 import { estimateSafeTxGas } from 'src/logic/safe/transactions/gas'
 import { currentSafe } from 'src/logic/safe/store/selectors'
+import useAsync from 'src/logic/hooks/useAsync'
 
 type UseEstimateSafeTxGasProps = {
   isCreation: boolean
@@ -22,30 +23,25 @@ export const useEstimateSafeTxGas = ({
   txAmount,
   operation,
 }: UseEstimateSafeTxGasProps): string => {
-  const [safeTxGasEstimation, setSafeTxGasEstimation] = useState<string>('0')
+  const defaultEstimation = '0'
   const { address: safeAddress, currentVersion: safeVersion } = useSelector(currentSafe) ?? {}
 
-  useEffect(() => {
-    if (!isCreation || isRejectTx) return
-    const estimateSafeTxGasCall = async () => {
-      try {
-        const safeTxGasEstimation = await estimateSafeTxGas(
-          {
-            safeAddress,
-            txData,
-            txRecipient,
-            txAmount: txAmount || '0',
-            operation: operation || Operation.CALL,
-          },
-          safeVersion,
-        )
-        setSafeTxGasEstimation(safeTxGasEstimation)
-      } catch (error) {
-        console.warn(error.message)
-      }
-    }
-    estimateSafeTxGasCall()
+  const requestSafeTxGas = useCallback((): Promise<string> => {
+    if (!isCreation || isRejectTx || !txData) return Promise.resolve(defaultEstimation)
+
+    return estimateSafeTxGas(
+      {
+        safeAddress,
+        txData,
+        txRecipient,
+        txAmount: txAmount || '0',
+        operation: operation || Operation.CALL,
+      },
+      safeVersion,
+    )
   }, [isCreation, isRejectTx, operation, safeAddress, safeVersion, txAmount, txData, txRecipient])
 
-  return safeTxGasEstimation
+  const { result } = useAsync<string>(requestSafeTxGas)
+
+  return result || defaultEstimation
 }

src/logic/hooks/useRecommendedNonce.tsx

@@ -1,36 +1,19 @@
-import { SafeTransactionEstimation } from '@gnosis.pm/safe-react-gateway-sdk'
-import { useEffect, useState } from 'react'
+import { useCallback } from 'react'
 import { useSelector } from 'react-redux'
 import { getRecommendedNonce } from 'src/logic/safe/api/fetchSafeTxGasEstimation'
 import { getLastTxNonce } from 'src/logic/safe/store/selectors/gatewayTransactions'
+import useAsync from 'src/logic/hooks/useAsync'
 
 const useRecommendedNonce = (safeAddress: string): number => {
-  const lastTxNonce = useSelector(getLastTxNonce)
-  const [recommendedNonce, setRecommendedNonce] = useState<number>(lastTxNonce ? lastTxNonce + 1 : 0)
+  const lastTxNonce = useSelector(getLastTxNonce) || -1
 
-  useEffect(() => {
-    let isCurrent = true
+  const getNonce = useCallback(() => {
+    return getRecommendedNonce(safeAddress)
+  }, [safeAddress])
 
-    const fetchRecommendedNonce = async () => {
-      let recommendedNonce: SafeTransactionEstimation['recommendedNonce']
-      try {
-        recommendedNonce = await getRecommendedNonce(safeAddress)
-      } catch (e) {
-        return
-      }
+  const { result } = useAsync<number>(getNonce)
 
-      if (isCurrent) {
-        setRecommendedNonce(recommendedNonce)
-      }
-    }
-    fetchRecommendedNonce()
-
-    return () => {
-      isCurrent = false
-    }
-  }, [lastTxNonce, safeAddress])
-
-  return recommendedNonce
+  return result == null ? lastTxNonce + 1 : result
 }
 
 export default useRecommendedNonce

src/logic/safe/transactions/gas.ts

@@ -116,7 +116,11 @@ export const checkTransactionExecution = async ({
       from,
       gas: gasLimit,
     })
-    .catch(() => false)
+    .then(() => true)
+    .catch((e) => {
+      console.warn('Transaction will fail\n\n', e)
+      return false
+    })
 }
 
 export const isMaxFeeParam = (): boolean => {