From d21ddd994ef6c904121ce633a44b5070285c7a4c Mon Sep 17 00:00:00 2001 From: Manuel Gellfart Date: Thu, 15 Feb 2024 20:36:47 +0100 Subject: [PATCH] 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 --- .../create/steps/ReviewStep/index.tsx | 2 +- .../tx/AdvancedParams/GasLimitInput.tsx | 4 + .../tx/GasParams/GasParams.test.tsx | 2 +- src/components/tx/GasParams/index.tsx | 2 +- .../counterfactual/ActivateAccount.tsx | 2 +- .../counterfactual/CounterfactualForm.tsx | 16 +-- .../__tests__/useDeployGasLimit.test.ts | 59 ++++++-- .../counterfactual/hooks/useDeployGasLimit.ts | 135 +++++++++++++----- src/hooks/__tests__/useGasPrice.test.ts | 19 +-- src/hooks/useGasPrice.ts | 12 +- src/hooks/useWalletCanPay.ts | 2 +- 11 files changed, 166 insertions(+), 89 deletions(-) diff --git a/src/components/new-safe/create/steps/ReviewStep/index.tsx b/src/components/new-safe/create/steps/ReviewStep/index.tsx index 1b0c97b4d7..5e956da674 100644 --- 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 { + 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 index 3d6178ef0d..b5aee6c446 100644 --- 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 index ce527316ea..69406c1b11 100644 --- 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 index 8294271ccd..7f20a4b5ff 100644 --- 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 index bdb72e4fe0..c09d120050 100644 --- a/src/features/counterfactual/CounterfactualForm.tsx +++ b/src/features/counterfactual/CounterfactualForm.tsx @@ -112,22 +112,12 @@ export const CounterfactualForm = ({ Executing this transaction will also activate your account. Additional network fees will apply. Base fee is{' '} - {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} , one time activation fee is{' '} - {getTotalFeeFormatted( - advancedParams.maxFeePerGas, - advancedParams.maxPriorityFeePerGas, - BigInt(gasLimit?.safeDeploymentGas || '0'), - chain, - )}{' '} + {getTotalFeeFormatted(advancedParams.maxFeePerGas, BigInt(gasLimit?.safeDeploymentGas || '0'), chain)}{' '} {chain?.nativeCurrency.symbol} . This is an estimation and the final cost can be higher. @@ -137,7 +127,7 @@ export const CounterfactualForm = ({ { 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 index 0807869423..499adb9232 100644 --- 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( - async () => { - if (!onboard) return + const [gasLimit, gasLimitError, gasLimitLoading] = useAsync(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 index a086530ae2..e9f1c389ee 100644 --- 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) }) }) diff --git a/src/hooks/useGasPrice.ts b/src/hooks/useGasPrice.ts index a5c6ba9ef5..6a355dd373 100644 --- a/src/hooks/useGasPrice.ts +++ b/src/hooks/useGasPrice.ts @@ -126,23 +126,17 @@ const getGasParameters = ( } } -export const getTotalFee = ( - maxFeePerGas: bigint, - maxPriorityFeePerGas: bigint | null | undefined, - gasLimit: bigint, -) => { - // maxPriorityFeePerGas is undefined if EIP-1559 disabled - return (maxFeePerGas + (maxPriorityFeePerGas || 0n)) * gasLimit +export const getTotalFee = (maxFeePerGas: bigint, gasLimit: bigint) => { + return maxFeePerGas * gasLimit } export const getTotalFeeFormatted = ( maxFeePerGas: bigint | null | undefined, - maxPriorityFeePerGas: bigint | null | undefined, gasLimit: bigint | undefined, chain: ChainInfo | undefined, ) => { return gasLimit && maxFeePerGas - ? formatVisualAmount(getTotalFee(maxFeePerGas, maxPriorityFeePerGas, gasLimit), chain?.nativeCurrency.decimals) + ? formatVisualAmount(getTotalFee(maxFeePerGas, gasLimit), chain?.nativeCurrency.decimals) : '> 0.001' } diff --git a/src/hooks/useWalletCanPay.ts b/src/hooks/useWalletCanPay.ts index 339c5a19bd..c651dcef15 100644 --- a/src/hooks/useWalletCanPay.ts +++ b/src/hooks/useWalletCanPay.ts @@ -16,7 +16,7 @@ const useWalletCanPay = ({ // if gasLimit, maxFeePerGas or their walletBalance are missing if (!gasLimit || !maxFeePerGas || walletBalance === undefined) return true - const totalFee = getTotalFee(maxFeePerGas, maxPriorityFeePerGas, gasLimit) + const totalFee = getTotalFee(maxFeePerGas, gasLimit) return walletBalance >= totalFee }