fix: [Counterfactual] gasEstimation for counterfactual deployments (#…

…3257) * fix: gasEstimation for counterfactual deployments * fix: gas estimation * fix: adjust comment * fix: useWalletCanPay * test: fix gas-related tests * fix: remove unnecessary file * chore: revert empty line * fix: Disable gasLimitInput for undeployed safes --------- Co-authored-by: Usame Algan <[email protected]>
safe-global · Feb 15, 2024 · d21ddd9 · d21ddd9
1 parent fb2fdb1
commit d21ddd9
Show file tree

Hide file tree

Showing 11 changed files with 166 additions and 89 deletions.
diff --git a/src/components/new-safe/create/steps/ReviewStep/index.tsx b/src/components/new-safe/create/steps/ReviewStep/index.tsx
@@ -179,7 +179,7 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe
 
   const walletCanPay = useWalletCanPay({ gasLimit, maxFeePerGas, maxPriorityFeePerGas })
 
-  const totalFee = getTotalFeeFormatted(maxFeePerGas, maxPriorityFeePerGas, gasLimit, chain)
+  const totalFee = getTotalFeeFormatted(maxFeePerGas, gasLimit, chain)
 
   // Only 1 out of 1 safe setups are supported for now
   const isCounterfactual = data.threshold === 1 && data.owners.length === 1 && isCounterfactualEnabled

diff --git a/src/components/tx/AdvancedParams/GasLimitInput.tsx b/src/components/tx/AdvancedParams/GasLimitInput.tsx
@@ -1,3 +1,4 @@
+import useSafeInfo from '@/hooks/useSafeInfo'
 import { FormControl, IconButton, Tooltip } from '@mui/material'
 import RotateLeftIcon from '@mui/icons-material/RotateLeft'
 import { useFormContext } from 'react-hook-form'
@@ -6,6 +7,8 @@ import { AdvancedField } from './types.d'
 import NumberField from '@/components/common/NumberField'
 
 const GasLimitInput = ({ recommendedGasLimit }: { recommendedGasLimit?: string }) => {
+  const { safe } = useSafeInfo()
+
   const {
     register,
     watch,
@@ -48,6 +51,7 @@ const GasLimitInput = ({ recommendedGasLimit }: { recommendedGasLimit?: string }
         InputLabelProps={{
           shrink: currentGasLimit !== undefined,
         }}
+        disabled={!safe.deployed}
         required
         {...register(AdvancedField.gasLimit, { required: true, min: BASE_TX_GAS })}
       />

diff --git a/src/components/tx/GasParams/GasParams.test.tsx b/src/components/tx/GasParams/GasParams.test.tsx
@@ -49,7 +49,7 @@ describe('GasParams', () => {
     const params: AdvancedParameters = {
       gasLimit: BigInt('21000'),
       userNonce: 1,
-      maxFeePerGas: BigInt('10000'),
+      maxFeePerGas: BigInt('20000'),
       maxPriorityFeePerGas: BigInt('10000'),
     }
 

diff --git a/src/components/tx/GasParams/index.tsx b/src/components/tx/GasParams/index.tsx
@@ -53,7 +53,7 @@ export const _GasParams = ({
 
   // Total gas cost
   const totalFee = !isLoading
-    ? formatVisualAmount(getTotalFee(maxFeePerGas, maxPriorityFeePerGas, gasLimit), chain?.nativeCurrency.decimals)
+    ? formatVisualAmount(getTotalFee(maxFeePerGas, gasLimit), chain?.nativeCurrency.decimals)
     : '> 0.001'
 
   // Individual gas params

diff --git a/src/features/counterfactual/ActivateAccount.tsx b/src/features/counterfactual/ActivateAccount.tsx
@@ -48,7 +48,7 @@ const useActivateAccount = () => {
       }
     : { gasPrice: maxFeePerGas?.toString(), gasLimit: gasLimit?.totalGas.toString() }
 
-  const totalFee = getTotalFeeFormatted(maxFeePerGas, maxPriorityFeePerGas, gasLimit?.totalGas, chain)
+  const totalFee = getTotalFeeFormatted(maxFeePerGas, gasLimit?.totalGas, chain)
   const walletCanPay = useWalletCanPay({ gasLimit: gasLimit?.totalGas, maxFeePerGas, maxPriorityFeePerGas })
 
   return { options, totalFee, walletCanPay }

diff --git a/src/features/counterfactual/CounterfactualForm.tsx b/src/features/counterfactual/CounterfactualForm.tsx
@@ -112,22 +112,12 @@ export const CounterfactualForm = ({
         <Alert severity="info" sx={{ mb: 2 }}>
           Executing this transaction will also activate your account. Additional network fees will apply. Base fee is{' '}
           <strong>
-            {getTotalFeeFormatted(
-              advancedParams.maxFeePerGas,
-              advancedParams.maxPriorityFeePerGas,
-              BigInt(gasLimit?.baseGas || '0') + BigInt(gasLimit?.safeTxGas || '0'),
-              chain,
-            )}{' '}
+            {getTotalFeeFormatted(advancedParams.maxFeePerGas, BigInt(gasLimit?.safeTxGas || '0'), chain)}{' '}
             {chain?.nativeCurrency.symbol}
           </strong>
           , one time activation fee is{' '}
           <strong>
-            {getTotalFeeFormatted(
-              advancedParams.maxFeePerGas,
-              advancedParams.maxPriorityFeePerGas,
-              BigInt(gasLimit?.safeDeploymentGas || '0'),
-              chain,
-            )}{' '}
+            {getTotalFeeFormatted(advancedParams.maxFeePerGas, BigInt(gasLimit?.safeDeploymentGas || '0'), chain)}{' '}
             {chain?.nativeCurrency.symbol}
           </strong>
           . This is an estimation and the final cost can be higher.
@@ -137,7 +127,7 @@ export const CounterfactualForm = ({
           <AdvancedParams
             willExecute
             params={advancedParams}
-            recommendedGasLimit={gasLimit}
+            recommendedGasLimit={gasLimit?.totalGas}
             onFormSubmit={setAdvancedParams}
             gasLimitError={gasLimitError}
             willRelay={false}

diff --git a/src/features/counterfactual/__tests__/useDeployGasLimit.test.ts b/src/features/counterfactual/__tests__/useDeployGasLimit.test.ts
@@ -3,9 +3,14 @@ import * as onboard from '@/hooks/wallets/useOnboard'
 import * as sdk from '@/services/tx/tx-sender/sdk'
 import { safeTxBuilder } from '@/tests/builders/safeTx'
 import * as protocolKit from '@safe-global/protocol-kit'
+import * as protocolKitContracts from '@safe-global/protocol-kit/dist/src/contracts/safeDeploymentContracts'
+import type Safe from '@safe-global/protocol-kit'
+
 import { renderHook } from '@/tests/test-utils'
 import { waitFor } from '@testing-library/react'
 import type { OnboardAPI } from '@web3-onboard/core'
+import { faker } from '@faker-js/faker'
+import type { CompatibilityFallbackHandlerContract, SimulateTxAccessorContract } from '@safe-global/safe-core-sdk-types'
 
 describe('useDeployGasLimit hook', () => {
   it('returns undefined in onboard is not initialized', () => {
@@ -17,7 +22,8 @@ describe('useDeployGasLimit hook', () => {
 
   it('returns safe deployment gas estimation', async () => {
     const mockGas = '100'
-    jest.spyOn(onboard, 'default').mockImplementation(jest.fn(() => ({} as OnboardAPI)))
+    const mockOnboard = {} as OnboardAPI
+    jest.spyOn(onboard, 'default').mockReturnValue(mockOnboard)
     jest.spyOn(sdk, 'getSafeSDKWithSigner').mockImplementation(jest.fn())
     const mockEstimateSafeDeploymentGas = jest
       .spyOn(protocolKit, 'estimateSafeDeploymentGas')
@@ -31,8 +37,9 @@ describe('useDeployGasLimit hook', () => {
     })
   })
 
-  it('does not estimate baseGas and safeTxGas if there is no safeTx and returns 0 for them instead', async () => {
-    jest.spyOn(onboard, 'default').mockImplementation(jest.fn(() => ({} as OnboardAPI)))
+  it('does not estimate safeTxGas if there is no safeTx and returns 0 for them instead', async () => {
+    const mockOnboard = {} as OnboardAPI
+    jest.spyOn(onboard, 'default').mockReturnValue(mockOnboard)
     jest.spyOn(sdk, 'getSafeSDKWithSigner').mockImplementation(jest.fn())
     jest.spyOn(protocolKit, 'estimateSafeDeploymentGas').mockReturnValue(Promise.resolve('100'))
 
@@ -44,22 +51,52 @@ describe('useDeployGasLimit hook', () => {
     await waitFor(() => {
       expect(mockEstimateTxBaseGas).not.toHaveBeenCalled()
       expect(mockEstimateSafeTxGas).not.toHaveBeenCalled()
-      expect(result.current.gasLimit?.baseGas).toEqual('0')
-      expect(result.current.gasLimit?.safeTxGas).toEqual('0')
+      expect(result.current.gasLimit?.safeTxGas).toEqual(0n)
     })
   })
 
   it('returns the totalFee', async () => {
-    jest.spyOn(onboard, 'default').mockImplementation(jest.fn(() => ({} as OnboardAPI)))
-    jest.spyOn(sdk, 'getSafeSDKWithSigner').mockImplementation(jest.fn())
+    const mockOnboard = {} as OnboardAPI
+    jest.spyOn(onboard, 'default').mockReturnValue(mockOnboard)
+    jest.spyOn(sdk, 'getSafeSDKWithSigner').mockResolvedValue({
+      getContractManager: () =>
+        ({
+          contractNetworks: {},
+        } as any),
+      getContractVersion: () => Promise.resolve('1.3.0'),
+      getEthAdapter: () => ({
+        estimateGas: () => Promise.resolve('420000'),
+        getSignerAddress: () => Promise.resolve(faker.finance.ethereumAddress()),
+      }),
+      createSafeDeploymentTransaction: () =>
+        Promise.resolve({
+          to: faker.finance.ethereumAddress(),
+          value: '0',
+          data: '0x1234',
+        }),
+      getAddress: () => Promise.resolve(faker.finance.ethereumAddress()),
+      createTransactionBatch: () =>
+        Promise.resolve({
+          to: faker.finance.ethereumAddress(),
+          value: '0',
+          data: '0x2345',
+        }),
+    } as unknown as Safe)
+    jest.spyOn(protocolKitContracts, 'getCompatibilityFallbackHandlerContract').mockResolvedValue({
+      encode: () => '0x3456',
+    } as unknown as CompatibilityFallbackHandlerContract)
+    jest.spyOn(protocolKitContracts, 'getSimulateTxAccessorContract').mockResolvedValue({
+      encode: () => '0x4567',
+      getAddress: () => Promise.resolve(faker.finance.ethereumAddress()),
+    } as unknown as SimulateTxAccessorContract)
     jest.spyOn(protocolKit, 'estimateSafeDeploymentGas').mockReturnValue(Promise.resolve('100'))
-    jest.spyOn(protocolKit, 'estimateTxBaseGas').mockReturnValue(Promise.resolve('100'))
-    jest.spyOn(protocolKit, 'estimateSafeTxGas').mockReturnValue(Promise.resolve('100'))
+    jest.spyOn(protocolKit, 'estimateTxBaseGas').mockReturnValue(Promise.resolve('21000'))
 
-    const { result } = renderHook(() => useDeployGasLimit(safeTxBuilder().build()))
+    const safeTx = safeTxBuilder().build()
+    const { result } = renderHook(() => useDeployGasLimit(safeTx))
 
     await waitFor(() => {
-      expect(result.current.gasLimit?.totalGas).toEqual(300n)
+      expect(result.current.gasLimit?.totalGas).toEqual(420000n + 21000n - 20000n)
     })
   })
 })
diff --git a/src/features/counterfactual/hooks/useDeployGasLimit.ts b/src/features/counterfactual/hooks/useDeployGasLimit.ts
@@ -1,29 +1,20 @@
-import { id } from 'ethers'
 import useAsync from '@/hooks/useAsync'
 import useChainId from '@/hooks/useChainId'
 import useOnboard from '@/hooks/wallets/useOnboard'
 import { getSafeSDKWithSigner } from '@/services/tx/tx-sender/sdk'
-import { estimateSafeDeploymentGas, estimateSafeTxGas, estimateTxBaseGas } from '@safe-global/protocol-kit'
-import type { SafeTransaction } from '@safe-global/safe-core-sdk-types'
+import { estimateSafeDeploymentGas, estimateTxBaseGas } from '@safe-global/protocol-kit'
+import type Safe from '@safe-global/protocol-kit'
 
-const ADD_OWNER_WITH_THRESHOLD_SIG_HASH = id('addOwnerWithThreshold(address,uint256)').slice(0, 10)
-const SWAP_OWNER_SIG_HASH = id('swapOwner(address,address,address)').slice(0, 10)
+import { OperationType, type SafeTransaction } from '@safe-global/safe-core-sdk-types'
+import {
+  getCompatibilityFallbackHandlerContract,
+  getSimulateTxAccessorContract,
+} from '@safe-global/protocol-kit/dist/src/contracts/safeDeploymentContracts'
 
-/**
- * The estimation from the protocol-kit is too low for swapOwner and addOwnerWithThreshold,
- * so we add a fixed amount
- * @param safeTx
- */
-const getExtraGasForSafety = (safeTx?: SafeTransaction): bigint => {
-  return safeTx?.data.data.startsWith(ADD_OWNER_WITH_THRESHOLD_SIG_HASH) ||
-    safeTx?.data.data.startsWith(SWAP_OWNER_SIG_HASH)
-    ? 25000n
-    : 0n
-}
+import { ZERO_ADDRESS } from '@safe-global/protocol-kit/dist/src/utils/constants'
 
 type DeployGasLimitProps = {
-  baseGas: string
-  safeTxGas: string
+  safeTxGas: bigint
   safeDeploymentGas: string
   totalGas: bigint
 }
@@ -32,29 +23,103 @@ const useDeployGasLimit = (safeTx?: SafeTransaction) => {
   const onboard = useOnboard()
   const chainId = useChainId()
 
-  const [gasLimit, gasLimitError, gasLimitLoading] = useAsync<DeployGasLimitProps | undefined>(
-    async () => {
-      if (!onboard) return
+  const [gasLimit, gasLimitError, gasLimitLoading] = useAsync<DeployGasLimitProps | undefined>(async () => {
+    if (!onboard) return
+    const sdk = await getSafeSDKWithSigner(onboard, chainId)
 
-      const sdk = await getSafeSDKWithSigner(onboard, chainId)
+    const [baseGas, batchTxGas, safeDeploymentGas] = await Promise.all([
+      safeTx ? estimateTxBaseGas(sdk, safeTx) : '0',
+      safeTx ? estimateBatchDeploymentTransaction(safeTx, sdk, chainId) : '0',
+      estimateSafeDeploymentGas(sdk),
+    ])
 
-      const extraGasForSafety = getExtraGasForSafety(safeTx)
+    const totalGas = safeTx ? BigInt(baseGas) + BigInt(batchTxGas) : BigInt(safeDeploymentGas)
+    const safeTxGas = totalGas - BigInt(safeDeploymentGas)
 
-      const [baseGas, safeTxGas, safeDeploymentGas] = await Promise.all([
-        safeTx ? estimateTxBaseGas(sdk, safeTx) : '0',
-        safeTx ? estimateSafeTxGas(sdk, safeTx) : '0',
-        estimateSafeDeploymentGas(sdk),
-      ])
+    return { safeTxGas, safeDeploymentGas, totalGas }
+  }, [onboard, chainId, safeTx])
 
-      const totalGas = BigInt(baseGas) + BigInt(safeTxGas) + BigInt(safeDeploymentGas) + extraGasForSafety
+  return { gasLimit, gasLimitError, gasLimitLoading }
+}
 
-      return { baseGas, safeTxGas, safeDeploymentGas, totalGas }
-    },
-    [onboard, chainId, safeTx],
-    false,
-  )
+/**
+ * Estimates batch transaction containing the safe deployment and the first transaction.
+ *
+ * This estimation is done by calling `eth_estimateGas` with a MultiSendCallOnly batch transaction that
+ *   1. Calls the SafeProxyFactory to deploy the SafeProxy
+ *   2. Call the `simulate` function on the now deployed SafeProxy with the first transaction data.
+ * Then we substract a flat gas amount for the overhead of simulating the transaction.
+ *
+ * Note: To have a more accurate estimation the base gas of a Safe Transaction has to be added to the result
+ * @param safeTransaction - first SafeTransaction that should be batched with the deployment
+ * @param sdk - predicted Safe instance
+ * @param chainId - chainId of the Safe
+ * @returns the gas estimation for the batch (as `bigint`)
+ */
+export const estimateBatchDeploymentTransaction = async (
+  safeTransaction: SafeTransaction,
+  sdk: Safe,
+  chainId: string,
+) => {
+  const customContracts = sdk.getContractManager().contractNetworks?.[chainId]
+  const safeVersion = await sdk.getContractVersion()
+  const ethAdapter = sdk.getEthAdapter()
+  const fallbackHandlerContract = await getCompatibilityFallbackHandlerContract({
+    ethAdapter,
+    safeVersion,
+    customContracts,
+  })
 
-  return { gasLimit, gasLimitError, gasLimitLoading }
+  const simulateTxAccessorContract = await getSimulateTxAccessorContract({
+    ethAdapter,
+    safeVersion,
+    customContracts,
+  })
+
+  // 1. Get Deploy tx
+  const safeDeploymentTransaction = await sdk.createSafeDeploymentTransaction()
+  const safeDeploymentBatchTransaction = {
+    to: safeDeploymentTransaction.to,
+    value: safeDeploymentTransaction.value,
+    data: safeDeploymentTransaction.data,
+    operation: OperationType.Call,
+  }
+
+  // 2. Add a simulate call to the predicted SafeProxy as second transaction
+  const transactionDataToEstimate: string = simulateTxAccessorContract.encode('simulate', [
+    safeTransaction.data.to,
+    safeTransaction.data.value,
+    safeTransaction.data.data,
+    safeTransaction.data.operation,
+  ])
+
+  const safeFunctionToEstimate: string = fallbackHandlerContract.encode('simulate', [
+    await simulateTxAccessorContract.getAddress(),
+    transactionDataToEstimate,
+  ])
+
+  const simulateBatchTransaction = {
+    to: await sdk.getAddress(),
+    value: '0',
+    data: safeFunctionToEstimate,
+    operation: OperationType.Call,
+  }
+
+  const safeDeploymentBatch = await sdk.createTransactionBatch([
+    safeDeploymentBatchTransaction,
+    simulateBatchTransaction,
+  ])
+
+  const signerAddress = await ethAdapter.getSignerAddress()
+
+  // estimate the entire batch
+  const safeTxGas = await ethAdapter.estimateGas({
+    ...safeDeploymentBatch,
+    from: signerAddress || ZERO_ADDRESS, // This address should not really matter
+  })
+
+  // Substract ~20k gas for the simulation overhead
+  return BigInt(safeTxGas) - 20_000n
 }
 
 export default useDeployGasLimit
diff --git a/src/hooks/__tests__/useGasPrice.test.ts b/src/hooks/__tests__/useGasPrice.test.ts
@@ -289,26 +289,13 @@ describe('useGasPrice', () => {
 
 describe('getTotalFee', () => {
   it('returns the totalFee', () => {
-    const result = getTotalFee(1n, 1n, 100n)
-
-    expect(result).toEqual(200n)
-  })
-
-  it('ignores maxPriorityFeePerGas if its undefined', () => {
-    const result = getTotalFee(1n, undefined, 100n)
-
-    expect(result).toEqual(100n)
-  })
-
-  it('ignores maxPriorityFeePerGas if its null', () => {
-    const result = getTotalFee(1n, null, 100n)
-
+    const result = getTotalFee(1n, 100n)
     expect(result).toEqual(100n)
   })
 
   it('handles large numbers', () => {
-    const result = getTotalFee(11230000000000123n, 2000000000000n, 123123123n)
+    const result = getTotalFee(10000000000000000n, 123123123n)
 
-    expect(result).toEqual(1382918917536015144144129n)
+    expect(result).toEqual(1231231230000000000000000n)
   })
 })