From ffc3feb0677e87af738b7a93635e18ecbee512f9 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Tue, 17 Dec 2024 13:40:32 +0000 Subject: [PATCH] fix: always estimate relay `gasLimit` --- .../new-safe/create/logic/index.test.ts | 18 ++--- src/components/new-safe/create/logic/index.ts | 10 ++- .../create/steps/ReviewStep/index.tsx | 2 +- .../counterfactual/ActivateAccountFlow.tsx | 2 +- src/services/tx/__tests__/relaying.test.ts | 66 +++++++++++++++++++ src/services/tx/relaying.ts | 29 ++++++++ .../tx/tx-sender/__tests__/ts-sender.test.ts | 8 +++ src/services/tx/tx-sender/dispatch.ts | 10 ++- 8 files changed, 127 insertions(+), 18 deletions(-) create mode 100644 src/services/tx/__tests__/relaying.test.ts create mode 100644 src/services/tx/relaying.ts diff --git a/src/components/new-safe/create/logic/index.test.ts b/src/components/new-safe/create/logic/index.test.ts index dc04191981..eaab210a00 100644 --- a/src/components/new-safe/create/logic/index.test.ts +++ b/src/components/new-safe/create/logic/index.test.ts @@ -10,7 +10,7 @@ import { getRedirect, createNewUndeployedSafeWithoutSalt, } from '@/components/new-safe/create/logic/index' -import { relayTransaction } from '@safe-global/safe-gateway-typescript-sdk' +import * as relaying from '@/services/tx/relaying' import { toBeHex } from 'ethers' import { Gnosis_safe__factory, @@ -21,7 +21,6 @@ import { getReadOnlyGnosisSafeContract, getReadOnlyProxyFactoryContract, } from '@/services/contracts/safeContracts' -import * as gateway from '@safe-global/safe-gateway-typescript-sdk' import { FEATURES, getLatestSafeVersion } from '@/utils/chains' import { type FEATURES as GatewayFeatures } from '@safe-global/safe-gateway-typescript-sdk' import { chainBuilder } from '@/tests/builders/chains' @@ -68,13 +67,14 @@ describe('create/logic', () => { }) it('returns taskId if create Safe successfully relayed', async () => { + const gasLimit = faker.string.numeric() const mockSafeProvider = { getExternalProvider: jest.fn(), getExternalSigner: jest.fn(), getChainId: jest.fn().mockReturnValue(BigInt(1)), } as unknown as SafeProvider - jest.spyOn(gateway, 'relayTransaction').mockResolvedValue({ taskId: '0x123' }) + jest.spyOn(relaying, 'relayTransaction').mockResolvedValue({ taskId: '0x123' }) jest.spyOn(sdkHelpers, 'getSafeProvider').mockImplementation(() => mockSafeProvider) jest.spyOn(contracts, 'getReadOnlyFallbackHandlerContract').mockResolvedValue({ @@ -123,20 +123,22 @@ describe('create/logic', () => { expectedSaltNonce, ]) - const taskId = await relaySafeCreation(mockChainInfo, undeployedSafeProps) + const taskId = await relaySafeCreation(mockChainInfo, undeployedSafeProps, gasLimit) expect(taskId).toEqual('0x123') - expect(relayTransaction).toHaveBeenCalledTimes(1) - expect(relayTransaction).toHaveBeenCalledWith('1', { + expect(relaying.relayTransaction).toHaveBeenCalledTimes(1) + expect(relaying.relayTransaction).toHaveBeenCalledWith('1', { to: proxyFactoryAddress, data: expectedCallData, version: latestSafeVersion, + gasLimit, }) }) it('should throw an error if relaying fails', () => { + const gasLimit = faker.string.numeric() const relayFailedError = new Error('Relay failed') - jest.spyOn(gateway, 'relayTransaction').mockRejectedValue(relayFailedError) + jest.spyOn(relaying, 'relayTransaction').mockRejectedValue(relayFailedError) const undeployedSafeProps: ReplayedSafeProps = { safeAccountConfig: { @@ -155,7 +157,7 @@ describe('create/logic', () => { saltNonce: '69', } - expect(relaySafeCreation(mockChainInfo, undeployedSafeProps)).rejects.toEqual(relayFailedError) + expect(relaySafeCreation(mockChainInfo, undeployedSafeProps, gasLimit)).rejects.toEqual(relayFailedError) }) }) describe('getRedirect', () => { diff --git a/src/components/new-safe/create/logic/index.ts b/src/components/new-safe/create/logic/index.ts index 0361d80caf..22d060fab2 100644 --- a/src/components/new-safe/create/logic/index.ts +++ b/src/components/new-safe/create/logic/index.ts @@ -2,7 +2,8 @@ import type { SafeVersion } from '@safe-global/safe-core-sdk-types' import { type Eip1193Provider, type Provider } from 'ethers' import semverSatisfies from 'semver/functions/satisfies' -import { getSafeInfo, type SafeInfo, type ChainInfo, relayTransaction } from '@safe-global/safe-gateway-typescript-sdk' +import { getSafeInfo, type SafeInfo, type ChainInfo } from '@safe-global/safe-gateway-typescript-sdk' +import { relayTransaction } from '@/services/tx/relaying' import { getReadOnlyProxyFactoryContract } from '@/services/contracts/safeContracts' import type { UrlObject } from 'url' import { AppRoutes } from '@/config/routes' @@ -178,7 +179,11 @@ export const getRedirect = ( return redirectUrl + `${appendChar}safe=${address}` } -export const relaySafeCreation = async (chain: ChainInfo, undeployedSafeProps: UndeployedSafeProps) => { +export const relaySafeCreation = async ( + chain: ChainInfo, + undeployedSafeProps: UndeployedSafeProps, + gasLimit: string | undefined, +) => { const replayedSafeProps = assertNewUndeployedSafeProps(undeployedSafeProps, chain) const encodedSafeCreationTx = encodeSafeCreationTx(replayedSafeProps, chain) @@ -186,6 +191,7 @@ export const relaySafeCreation = async (chain: ChainInfo, undeployedSafeProps: U to: replayedSafeProps.factoryAddress, data: encodedSafeCreationTx, version: replayedSafeProps.safeVersion, + gasLimit, }) return relayResponse.taskId diff --git a/src/components/new-safe/create/steps/ReviewStep/index.tsx b/src/components/new-safe/create/steps/ReviewStep/index.tsx index 1e388ad0ce..e17a7f7a00 100644 --- a/src/components/new-safe/create/steps/ReviewStep/index.tsx +++ b/src/components/new-safe/create/steps/ReviewStep/index.tsx @@ -336,7 +336,7 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps { try { if (willRelay) { - const taskId = await relaySafeCreation(chain, undeployedSafe.props) + const taskId = await relaySafeCreation(chain, undeployedSafe.props, options?.gasLimit?.toString()) safeCreationDispatch(SafeCreationEvent.RELAYING, { groupKey: CF_TX_GROUP_KEY, taskId, safeAddress }) onSubmit() diff --git a/src/services/tx/__tests__/relaying.test.ts b/src/services/tx/__tests__/relaying.test.ts new file mode 100644 index 0000000000..d93cce0521 --- /dev/null +++ b/src/services/tx/__tests__/relaying.test.ts @@ -0,0 +1,66 @@ +import { faker } from '@faker-js/faker' +import { relayTransaction as _relayTransaction } from '@safe-global/safe-gateway-typescript-sdk' + +import { relayTransaction } from '@/services/tx/relaying' +import { getWeb3ReadOnly } from '@/hooks/wallets/web3' + +jest.mock('@/hooks/wallets/web3') +jest.mock('@safe-global/safe-gateway-typescript-sdk') + +describe('relayTransaction', () => { + let chainId: string + let body: { + version: string + to: string + data: string + gasLimit: string | undefined + } + + beforeEach(() => { + jest.clearAllMocks() + + chainId = faker.string.numeric() + body = { + version: faker.system.semver(), + to: faker.finance.ethereumAddress(), + data: faker.string.hexadecimal(), + gasLimit: undefined, + } + }) + + it('should relay with the correct parameters when gasLimit is provided', async () => { + const bodyWithGasLimit = { + ...body, + gasLimit: faker.string.numeric(), + } + + await relayTransaction(chainId, bodyWithGasLimit) + + expect(_relayTransaction).toHaveBeenCalledWith(chainId, bodyWithGasLimit) + }) + + it('should estimate gasLimit if not provided and relay with the estimated gasLimit', async () => { + const gasLimit = faker.number.bigInt() + const mockProvider = { + estimateGas: jest.fn().mockResolvedValue(gasLimit), + } + ;(getWeb3ReadOnly as jest.Mock).mockReturnValue(mockProvider) + + await relayTransaction(chainId, body) + + expect(mockProvider.estimateGas).toHaveBeenCalledWith(body) + expect(_relayTransaction).toHaveBeenCalledWith(chainId, { ...body, gasLimit: gasLimit.toString() }) + }) + + it('should relay with undefined gasLimit if estimation fails', async () => { + const mockProvider = { + estimateGas: jest.fn().mockRejectedValue(new Error('Estimation failed')), + } + ;(getWeb3ReadOnly as jest.Mock).mockReturnValue(mockProvider) + + await relayTransaction(chainId, body) + + expect(mockProvider.estimateGas).toHaveBeenCalledWith(body) + expect(_relayTransaction).toHaveBeenCalledWith(chainId, { ...body, gasLimit: undefined }) + }) +}) diff --git a/src/services/tx/relaying.ts b/src/services/tx/relaying.ts new file mode 100644 index 0000000000..9b147ccbb7 --- /dev/null +++ b/src/services/tx/relaying.ts @@ -0,0 +1,29 @@ +import { relayTransaction as _relayTransaction } from '@safe-global/safe-gateway-typescript-sdk' + +import { getWeb3ReadOnly } from '@/hooks/wallets/web3' + +export async function relayTransaction( + chainId: string, + body: { version: string; to: string; data: string; gasLimit: string | undefined }, +) { + /** + * Ensures `gasLimit` is estimate so 150k execution overhead buffer can be added by CGW. + * + * @see https://github.com/safe-global/safe-client-gateway/blob/b7640ed4bd7416ecb7696d21ba105fcb5af52a10/src/datasources/relay-api/gelato-api.service.ts#L62-L75 + * + * - If provided, CGW adds a 150k buffer before submission to Gelato. + * - If not provided, no buffer is added, and Gelato estimates the it. + * + * Note: particularly important on networks like Arbitrum, where estimation is unreliable. + */ + if (!body.gasLimit) { + const provider = getWeb3ReadOnly() + + body.gasLimit = await provider + ?.estimateGas(body) + .then(String) + .catch(() => undefined) + } + + return _relayTransaction(chainId, body) +} diff --git a/src/services/tx/tx-sender/__tests__/ts-sender.test.ts b/src/services/tx/tx-sender/__tests__/ts-sender.test.ts index 190434c93d..81afaf8fd9 100644 --- a/src/services/tx/tx-sender/__tests__/ts-sender.test.ts +++ b/src/services/tx/tx-sender/__tests__/ts-sender.test.ts @@ -22,6 +22,7 @@ import { type JsonRpcProvider, type JsonRpcSigner, } from 'ethers' +import { faker } from '@faker-js/faker' import * as safeContracts from '@/services/contracts/safeContracts' import * as web3 from '@/hooks/wallets/web3' @@ -515,6 +516,13 @@ describe('txSender', () => { describe('dispatchBatchExecutionRelay', () => { it('should relay a batch execution', async () => { + const gasLimit = faker.number.bigInt() + jest.spyOn(web3, 'getWeb3ReadOnly').mockImplementation(() => { + return { + estimateGas: jest.fn(() => Promise.resolve(gasLimit)), + } as unknown as JsonRpcProvider + }) + const mockMultisendAddress = zeroPadValue('0x1234', 20) const safeAddress = toBeHex('0x567', 20) diff --git a/src/services/tx/tx-sender/dispatch.ts b/src/services/tx/tx-sender/dispatch.ts index 15d256440b..742472c9a8 100644 --- a/src/services/tx/tx-sender/dispatch.ts +++ b/src/services/tx/tx-sender/dispatch.ts @@ -2,12 +2,8 @@ import type { ConnectedWallet } from '@/hooks/wallets/useOnboard' import { isMultisigExecutionInfo } from '@/utils/transaction-guards' import { isHardwareWallet, isSmartContractWallet } from '@/utils/wallets' import type { MultiSendCallOnlyContractImplementationType } from '@safe-global/protocol-kit' -import { - type ChainInfo, - relayTransaction, - type SafeInfo, - type TransactionDetails, -} from '@safe-global/safe-gateway-typescript-sdk' +import { type ChainInfo, type SafeInfo, type TransactionDetails } from '@safe-global/safe-gateway-typescript-sdk' +import { relayTransaction } from '@/services/tx/relaying' import type { SafeSignature, SafeTransaction, @@ -559,6 +555,8 @@ export const dispatchBatchExecutionRelay = async ( to, data, version: safeVersion, + // We have no estimation in place + gasLimit: undefined, }) } catch (error) { txs.forEach(({ txId }) => {