Merge pull request #980 from ensdomains/fix/safe-gas-estimates

fix: safe (or other sca) gas estimates

Author: laurgk

src/components/@molecules/TransactionDialogManager/stage/TransactionStageModal.test.tsx

@@ -14,6 +14,7 @@ import { useAddRecentTransaction } from '@app/hooks/transactions/useAddRecentTra
 import { useRecentTransactions } from '@app/hooks/transactions/useRecentTransactions'
 import { useIsSafeApp } from '@app/hooks/useIsSafeApp'
 import { GenericTransaction } from '@app/transaction-flow/types'
+import { createAccessList } from '@app/utils/query/createAccessList'
 import { checkIsSafeApp } from '@app/utils/safe'
 
 import { makeMockIntersectionObserver } from '../../../../../test/mock/makeMockIntersectionObserver'
@@ -28,6 +29,7 @@ vi.mock('@app/hooks/transactions/useAddRecentTransaction')
 vi.mock('@app/hooks/transactions/useRecentTransactions')
 vi.mock('@app/hooks/chain/useInvalidateOnBlock')
 vi.mock('@app/utils/safe')
+vi.mock('@app/utils/query/createAccessList')
 vi.mock('@wagmi/core', async () => {
   const actual = await vi.importActual('@wagmi/core')
   return {
@@ -79,6 +81,7 @@ const mockUseClient = mockFunction(useClient)
 const mockUseConnectorClient = mockFunction(useConnectorClient)
 
 const mockEstimateGas = mockFunction(estimateGas)
+const mockCreateAccessList = createAccessList as MockedFunctionDeep<typeof createAccessList>
 const mockPrepareTransactionRequest = prepareTransactionRequest as MockedFunctionDeep<
   typeof prepareTransactionRequest
 >
@@ -208,6 +211,7 @@ describe('TransactionStageModal', () => {
       })
 
       it('should disable confirm button and re-estimate gas if a unique identifier is changed', async () => {
+        mockCreateAccessList.mockResolvedValue({ accessList: [], gasUsed: '0x64' })
         mockEstimateGas.mockResolvedValue(1n)
         mockUseIsSafeApp.mockReturnValue({ data: false })
         mockUseSendTransaction.mockReturnValue({
@@ -236,6 +240,7 @@ describe('TransactionStageModal', () => {
       })
 
       it('should only show confirm button as enabled if gas is estimated and sendTransaction func is defined', async () => {
+        mockCreateAccessList.mockResolvedValue({ accessList: [], gasUsed: '0x64' })
         mockEstimateGas.mockResolvedValue(1n)
         mockUseSendTransaction.mockReturnValue({
           sendTransaction: () => Promise.resolve(),
@@ -246,6 +251,7 @@ describe('TransactionStageModal', () => {
         )
       })
       it('should run set sendTransaction on action click', async () => {
+        mockCreateAccessList.mockResolvedValue({ accessList: [], gasUsed: '0x64' })
         mockEstimateGas.mockResolvedValue(1n)
         const mockSendTransaction = vi.fn()
         mockUseSendTransaction.mockReturnValue({
@@ -286,6 +292,7 @@ describe('TransactionStageModal', () => {
       })
       it('should pass the request to send transaction', async () => {
         mockUseIsSafeApp.mockReturnValue({ data: false })
+        mockCreateAccessList.mockResolvedValue({ accessList: [], gasUsed: '0x64' })
         mockEstimateGas.mockResolvedValue(1n)
         const mockSendTransaction = vi.fn()
         mockUseSendTransaction.mockReturnValue({
@@ -298,7 +305,7 @@ describe('TransactionStageModal', () => {
             expect.objectContaining({
               ...mockTransactionRequest,
               gas: 1n,
-              accessList: undefined,
+              accessList: [],
             }),
           ),
         )
@@ -500,59 +507,37 @@ describe('calculateGasLimit', () => {
     data: '0x12345678',
   } as const
   const mockTransactionName = 'registerName'
-  const mockIsSafeApp = false
+  const mockAccessListResponse = {
+    accessList: [
+      {
+        address: '0x1234567890123456789012345678901234567890' as const,
+        storageKeys: [
+          '0x1234567890123456789012345678901234567890123456789012345678901234' as const,
+        ],
+      },
+    ],
+    gasUsed: '0x64' as const,
+  }
 
   beforeEach(() => {
     vi.clearAllMocks()
   })
 
-  it('should calculate gas limit for non-safe apps', async () => {
+  it('should calculate gas limit', async () => {
     mockEstimateGas.mockResolvedValueOnce(100000n)
+    mockCreateAccessList.mockResolvedValueOnce(mockAccessListResponse)
     const result = await calculateGasLimit({
-      isSafeApp: mockIsSafeApp,
       txWithZeroGas: mockTxWithZeroGas,
       transactionName: mockTransactionName,
       client: mockClient as any,
       connectorClient: mockConnectorClient as any,
     })
     expect(result.gasLimit).toEqual(105000n)
-    expect(result.accessList).toBeUndefined()
+    expect(result.accessList).toEqual(mockAccessListResponse.accessList)
     expect(mockEstimateGas).toHaveBeenCalledWith(mockClient, {
       ...mockTxWithZeroGas,
+      accessList: mockAccessListResponse.accessList,
       account: mockConnectorClient.account,
     })
   })
-
-  it('should calculate gas limit for safe apps', async () => {
-    const mockAccessListResponse = {
-      gasUsed: '0x64',
-      accessList: [
-        {
-          address: '0x1234567890123456789012345678901234567890',
-          storageKeys: ['0x1234567890123456789012345678901234567890123456789012345678901234'],
-        },
-      ],
-    }
-    mockClient.request.mockResolvedValueOnce(mockAccessListResponse)
-    const result = await calculateGasLimit({
-      isSafeApp: true,
-      txWithZeroGas: mockTxWithZeroGas,
-      transactionName: mockTransactionName,
-      client: mockClient as any,
-      connectorClient: mockConnectorClient as any,
-    })
-    expect(result.gasLimit).toEqual(5100n)
-    expect(result.accessList).toEqual(mockAccessListResponse.accessList)
-    expect(mockClient.request).toHaveBeenCalledWith({
-      method: 'eth_createAccessList',
-      params: [
-        {
-          from: mockConnectorClient.account.address,
-          ...mockTxWithZeroGas,
-          value: '0x0',
-        },
-        'latest',
-      ],
-    })
-  })
 })

src/components/@molecules/TransactionDialogManager/stage/TransactionStageModal.tsx

@@ -296,13 +296,24 @@ const getPreTransactionError = ({
   return match({ stage, err: transactionError || requestError })
     .with({ stage: P.union('complete', 'sent') }, () => null)
     .with({ err: P.nullish }, () => null)
-    .with({ err: P.not(P.instanceOf(BaseError)) }, ({ err }) => ({
-      message: 'message' in err! ? err.message : 'transaction.error.unknown',
-      type: 'unknown' as const,
-    }))
+    .with({ err: P.not(P.instanceOf(BaseError)) }, ({ err }) => {
+      return {
+        message: 'message' in err! ? err.message : 'transaction.error.unknown',
+        type: 'unknown' as const,
+      }
+    })
     .otherwise(({ err }) => {
       const readableError = getReadableError(err)
-      return readableError || { message: (err as BaseError).shortMessage, type: 'unknown' as const }
+      const error = readableError || {
+        message: (err as BaseError).shortMessage,
+        type: 'unknown' as const,
+      }
+      // TODO: Remove this when the following issue is fixed: https://github.com/paradigmxyz/reth/issues/15762
+      // Cause reth throws an error on eth_createAccessList when `sender is not an EOA`
+      if (error.message === 'Transaction creation failed.') {
+        return null
+      }
+      return error
     })
 }
 
@@ -395,7 +406,7 @@ export const TransactionStageModal = ({
 
   const preparedOptions = queryOptions({
     queryKey: initialOptions.queryKey,
-    queryFn: initialOptions.queryFn({ connectorClient, isSafeApp, connections }),
+    queryFn: initialOptions.queryFn({ connectorClient, connections }),
   })
 
   const transactionRequestQuery = useQuery({

src/components/@molecules/TransactionDialogManager/stage/query.ts

@@ -1,16 +1,7 @@
 import { QueryFunctionContext } from '@tanstack/react-query'
 import { CallParameters, getFeeHistory, SendTransactionReturnType } from '@wagmi/core'
 import { Dispatch } from 'react'
-import {
-  Address,
-  BlockTag,
-  Hash,
-  Hex,
-  PrepareTransactionRequestRequest,
-  toHex,
-  Transaction,
-  TransactionRequest,
-} from 'viem'
+import { Hash, PrepareTransactionRequestRequest, toHex, Transaction } from 'viem'
 import { call, estimateGas, getTransaction, prepareTransactionRequest } from 'viem/actions'
 import { useConnections } from 'wagmi'
 
@@ -33,18 +24,10 @@ import {
   CreateQueryKey,
 } from '@app/types'
 import { getReadableError } from '@app/utils/errors'
+import { createAccessList } from '@app/utils/query/createAccessList'
 import { wagmiConfig } from '@app/utils/query/wagmi'
-import { CheckIsSafeAppReturnType } from '@app/utils/safe'
 import { hasParaConnection } from '@app/utils/utils'
 
-type AccessListResponse = {
-  accessList: {
-    address: Address
-    storageKeys: Hex[]
-  }[]
-  gasUsed: Hex
-}
-
 export const getUniqueTransaction = ({
   txKey,
   currentStep,
@@ -120,47 +103,30 @@ export const registrationGasFeeModifier = (gasLimit: bigint, transactionName: Tr
 export const calculateGasLimit = async ({
   client,
   connectorClient,
-  isSafeApp,
   txWithZeroGas,
   transactionName,
 }: {
   client: ClientWithEns
   connectorClient: ConnectorClientWithEns
-  isSafeApp: boolean
   txWithZeroGas: BasicTransactionRequest
   transactionName: TransactionName
 }) => {
-  if (isSafeApp) {
-    const accessListResponse = await client.request<{
-      Method: 'eth_createAccessList'
-      Parameters: [tx: TransactionRequest<Hex>, blockTag: BlockTag]
-      ReturnType: AccessListResponse
-    }>({
-      method: 'eth_createAccessList',
-      params: [
-        {
-          to: txWithZeroGas.to,
-          data: txWithZeroGas.data,
-          from: connectorClient.account!.address,
-          value: toHex(txWithZeroGas.value ? txWithZeroGas.value + 1000000n : 0n),
-        },
-        'latest',
-      ],
-    })
-
-    return {
-      gasLimit: registrationGasFeeModifier(BigInt(accessListResponse.gasUsed), transactionName),
-      accessList: accessListResponse.accessList,
-    }
-  }
+  const accessListResponse = await createAccessList(client, {
+    to: txWithZeroGas.to,
+    data: txWithZeroGas.data,
+    from: connectorClient.account!.address,
+    value: toHex(txWithZeroGas.value ? txWithZeroGas.value + 1000000n : 0n),
+  })
 
   const gasEstimate = await estimateGas(client, {
     ...txWithZeroGas,
-    account: connectorClient.account!,
+    accessList: accessListResponse.accessList,
+    account: connectorClient.account,
   })
+
   return {
     gasLimit: registrationGasFeeModifier(gasEstimate, transactionName),
-    accessList: undefined,
+    accessList: accessListResponse.accessList,
   }
 }
 
@@ -195,7 +161,6 @@ export type CreateTransactionRequestQueryKey = CreateQueryKey<
 type CreateTransactionRequestUnsafeParameters = {
   client: ClientWithEns
   connectorClient: ConnectorClientWithEns
-  isSafeApp: CheckIsSafeAppReturnType | undefined
   params: UniqueTransaction
   chainId: SupportedChain['id']
   connections: any
@@ -204,7 +169,6 @@ type CreateTransactionRequestUnsafeParameters = {
 export const createTransactionRequestUnsafe = async ({
   client,
   connectorClient,
-  isSafeApp,
   params,
   chainId,
   connections,
@@ -225,7 +189,6 @@ export const createTransactionRequestUnsafe = async ({
   const { gasLimit, accessList } = await calculateGasLimit({
     client,
     connectorClient,
-    isSafeApp: !!isSafeApp,
     txWithZeroGas,
     transactionName: params.name,
   })
@@ -261,11 +224,9 @@ export const createTransactionRequestQueryFn =
   (config: ConfigWithEns) =>
   ({
     connectorClient,
-    isSafeApp,
     connections,
   }: {
     connectorClient: ConnectorClientWithEns | undefined
-    isSafeApp: CheckIsSafeAppReturnType | undefined
     connections: ReturnType<typeof useConnections>
   }) =>
   async ({
@@ -282,7 +243,6 @@ export const createTransactionRequestQueryFn =
         data: await createTransactionRequestUnsafe({
           client,
           connectorClient,
-          isSafeApp,
           params,
           chainId,
           connections,

src/hooks/chain/useEstimateGasWithStateOverride.ts

@@ -14,7 +14,6 @@ import {
   parseEther,
   RpcTransactionRequest,
   toHex,
-  TransactionRequest,
 } from 'viem'
 import { useConnectorClient } from 'wagmi'
 
@@ -31,8 +30,10 @@ import {
   Prettify,
   QueryConfig,
 } from '@app/types'
+import { emptyAddress } from '@app/utils/constants'
 import { getIsCachedData } from '@app/utils/getIsCachedData'
 import { prepareQueryOptions } from '@app/utils/prepareQueryOptions'
+import { createAccessList } from '@app/utils/query/createAccessList'
 import { useQuery } from '@app/utils/query/useQuery'
 
 import { useGasPrice } from './useGasPrice'
@@ -164,10 +165,28 @@ const estimateIndividualGas = async <TName extends TransactionName>({
     name,
   })
 
+  // SCAs use >21k gas to execute a transfer (most of the time, but depends on implementation)
+  // For any SCA transaction involving a transfer, the transaction will probably fail by default due to the extra gas.
+  // To fix this, we can use an access list of what storage slots are accessed in a transfer to pre-pay the cold gas,
+  // then when the transfer is done, only warm gas is used and the transfer is under the limit.
+  // Normally, you can just pull the estimated gas via `eth_getAccessList`, since it returns the access list + gas used,
+  // but since we're using a state override and that method doesn't support it, we have to do it manually.
+  // (Technically geth now has an implementation, but it's very new and only in geth, see https://github.com/ethereum/go-ethereum/issues/27630)
+  //
+  // To get the access list, we're executing the bytecode of this Yul code: https://gist.github.com/TateB/777287c9a63d5f02fcd905232ce5748a
+  // (note: 0xed3869F3020315C839b2f4E9a73bEbE9a9670534 is replaced with `connectorClient.account.address`)
+  // It does a simple transfer, and accesses any storage slot that would be accessed by any other transfer.
+  const accessList = await createAccessList(client, {
+    from: emptyAddress,
+    data: concatHex(['0x5f808080600173', connectorClient.account.address, '0x5af100']),
+    value: '0x1',
+  })
+
   const formattedRequest = formatTransactionRequest({
     ...generatedRequest,
     from: connectorClient.account.address,
-  } as TransactionRequest)
+    accessList: accessList.accessList,
+  })
 
   const stateOverrideWithBalance = stateOverride?.find(
     (s) => s.address === connectorClient.account.address,

src/hooks/transactions/transactionStore.test.ts

@@ -56,10 +56,8 @@ describe('transactionStore', () => {
       setTimeout(() => {
         onReplaced!({
           reason: 'repriced',
-          transaction: {
-            hash: newHash,
-          },
-        } as any)
+          transactionHash: newHash,
+        })
       }, 0)
 
       return {