Await txDetails; disallow batching delegate calls; other PR comments

safe-global · Jul 28, 2023 · 3663f32 · 3663f32
1 parent f8856e3
commit 3663f32
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 56 deletions.
diff --git a/src/components/batch/BatchSidebar/BatchTxItem.tsx b/src/components/batch/BatchSidebar/BatchTxItem.tsx
@@ -32,14 +32,13 @@ const BatchTxItem = ({
   draggable = false,
 }: BatchTxItemProps) => {
   const txSummary = useMemo(
-    () =>
-      ({
-        timestamp,
-        id: txDetails.txId,
-        txInfo: txDetails.txInfo,
-        txStatus: txDetails.txStatus,
-        safeAppInfo: txDetails.safeAppInfo,
-      }),
+    () => ({
+      timestamp,
+      id: txDetails.txId,
+      txInfo: txDetails.txInfo,
+      txStatus: txDetails.txStatus,
+      safeAppInfo: txDetails.safeAppInfo,
+    }),
     [timestamp, txDetails],
   )
 

diff --git a/src/components/tx/SignOrExecuteForm/SignForm.tsx b/src/components/tx/SignOrExecuteForm/SignForm.tsx
@@ -22,7 +22,7 @@ const SignForm = ({
   disableSubmit = false,
   origin,
   isBatch,
-  isMultisend,
+  isBatchable,
 }: SignOrExecuteProps & {
   safeTx?: SafeTransaction
 }): ReactElement => {
@@ -90,8 +90,8 @@ const SignForm = ({
           {isCreation && !isBatch && (
             <BatchButton
               onClick={onBatchClick}
-              disabled={submitDisabled || isMultisend}
-              tooltip={isMultisend ? `Cannot batch this transaction because it's already a batch` : undefined}
+              disabled={submitDisabled || !isBatchable}
+              tooltip={!isBatchable ? `Cannot batch this transaction because it's already a batch` : undefined}
             />
           )}
 

diff --git a/src/components/tx/SignOrExecuteForm/hooks.ts b/src/components/tx/SignOrExecuteForm/hooks.ts
@@ -68,7 +68,7 @@ export const useTxActions = (): TxActions => {
       assertWallet(wallet)
 
       const id = await proposeTx(wallet.address, safeTx, undefined, origin)
-      addTxToBatch(id)
+      await addTxToBatch(id)
       return id
     }
 

diff --git a/src/components/tx/SignOrExecuteForm/index.tsx b/src/components/tx/SignOrExecuteForm/index.tsx
@@ -15,7 +15,7 @@ import { selectSettings } from '@/store/settingsSlice'
 import { RedefineBalanceChanges } from '../security/redefine/RedefineBalanceChange'
 import UnknownContractError from './UnknownContractError'
 import RiskConfirmationError from './RiskConfirmationError'
-import useDecodeTx, { isMultisendTx } from '@/hooks/useDecodeTx'
+import useDecodeTx, { isDelegateCall, isMultisendTx } from '@/hooks/useDecodeTx'
 import { ErrorBoundary } from '@sentry/react'
 import ApprovalEditor from '../ApprovalEditor'
 
@@ -26,7 +26,7 @@ export type SignOrExecuteProps = {
   isExecutable?: boolean
   isRejection?: boolean
   isBatch?: boolean
-  isMultisend?: boolean
+  isBatchable?: boolean
   onlyExecute?: boolean
   disableSubmit?: boolean
   origin?: string
@@ -40,6 +40,7 @@ const SignOrExecuteForm = (props: SignOrExecuteProps): ReactElement => {
   const isNewExecutableTx = useImmediatelyExecutable() && isCreation
   const isCorrectNonce = useValidateNonce(safeTx)
   const [decodedData, decodedDataError, decodedDataLoading] = useDecodeTx(safeTx)
+  const isBatchable = safeTx && !isDelegateCall(safeTx) && !isMultisendTx(decodedData)
 
   // If checkbox is checked and the transaction is executable, execute it, otherwise sign it
   const canExecute = isCorrectNonce && (props.isExecutable || isNewExecutableTx)
@@ -93,7 +94,7 @@ const SignOrExecuteForm = (props: SignOrExecuteProps): ReactElement => {
         {willExecute ? (
           <ExecuteForm {...props} safeTx={safeTx} />
         ) : (
-          <SignForm {...props} safeTx={safeTx} isMultisend={isMultisendTx(decodedData)} />
+          <SignForm {...props} safeTx={safeTx} isBatchable={isBatchable} />
         )}
       </TxCard>
     </>

diff --git a/src/hooks/useDecodeTx.ts b/src/hooks/useDecodeTx.ts
@@ -1,4 +1,4 @@
-import { type SafeTransaction } from '@safe-global/safe-core-sdk-types'
+import { OperationType, type SafeTransaction } from '@safe-global/safe-core-sdk-types'
 import { type DecodedDataResponse, getDecodedData } from '@safe-global/safe-gateway-typescript-sdk'
 import { getNativeTransferData } from '@/services/tx/tokenTransferParams'
 import { isEmptyHexData } from '@/utils/hex'
@@ -25,4 +25,8 @@ export const isMultisendTx = (decodedData?: DecodedDataResponse): boolean => {
   return !!decodedData?.parameters?.[0]?.valueDecoded
 }
 
+export const isDelegateCall = (safeTx: SafeTransaction): boolean => {
+  return safeTx.data.operation === OperationType.DelegateCall
+}
+
 export default useDecodeTx
diff --git a/src/hooks/useDraftBatch.ts b/src/hooks/useDraftBatch.ts
@@ -14,20 +14,20 @@ export const useUpdateBatch = () => {
   const dispatch = useAppDispatch()
 
   const onAdd = useCallback(
-    (id: string) => {
-      getTransactionDetails(chainId, id).then((tx) => {
-        dispatch(
-          addTx({
-            chainId,
-            safeAddress,
-            txDetails: tx,
-          }),
-        )
+    async (id: string): Promise<void> => {
+      const txDetails = await getTransactionDetails(chainId, id)
+
+      dispatch(
+        addTx({
+          chainId,
+          safeAddress,
+          txDetails,
+        }),
+      )
 
-        txDispatch(TxEvent.BATCH_ADD, { txId: id })
+      txDispatch(TxEvent.BATCH_ADD, { txId: id })
 
-        trackEvent({ ...BATCH_EVENTS.BATCH_TX_APPENDED, label: tx.txInfo.type })
-      })
+      trackEvent({ ...BATCH_EVENTS.BATCH_TX_APPENDED, label: txDetails.txInfo.type })
     },
     [dispatch, chainId, safeAddress],
   )

diff --git a/src/services/tx/tx-sender/__tests__/ts-sender.test.ts b/src/services/tx/tx-sender/__tests__/ts-sender.test.ts
@@ -1,6 +1,6 @@
 import { setSafeSDK } from '@/hooks/coreSDK/safeCoreSDK'
 import type Safe from '@safe-global/safe-core-sdk'
-import type { SafeSignature, SafeTransaction, TransactionResult } from '@safe-global/safe-core-sdk-types'
+import type { TransactionResult } from '@safe-global/safe-core-sdk-types'
 import { type TransactionDetails } from '@safe-global/safe-gateway-typescript-sdk'
 import { getTransactionDetails } from '@safe-global/safe-gateway-typescript-sdk'
 import extractTxInfo from '../../extractTxInfo'
@@ -31,29 +31,7 @@ const setupFetchStub = (data: any) => (_url: string) => {
 import type { EIP1193Provider, OnboardAPI, WalletState, AppState } from '@web3-onboard/core'
 import { hexZeroPad } from 'ethers/lib/utils'
 import { generatePreValidatedSignature } from '@safe-global/safe-core-sdk/dist/src/utils/signatures'
-
-const createSafeTx = (data = '0x'): SafeTransaction => {
-  return {
-    data: {
-      to: '0x0000000000000000000000000000000000000000',
-      value: '0x0',
-      data,
-      operation: 0,
-      nonce: 100,
-    },
-    signatures: new Map([]),
-    addSignature: function (sig: SafeSignature): void {
-      this.signatures.set(sig.signer, sig)
-    },
-    encodedSignatures: function (): string {
-      return Array.from(this.signatures)
-        .map(([, sig]) => {
-          return [sig.signer, sig.data].join(' = ')
-        })
-        .join('; ')
-    },
-  } as SafeTransaction
-}
+import { createMockSafeTransaction } from '@/tests/transactions'
 
 // Mock getTransactionDetails
 jest.mock('@safe-global/safe-gateway-typescript-sdk', () => ({
@@ -237,7 +215,10 @@ describe('txSender', () => {
     })
 
     it('should dispatch a PROPOSED event if tx is signed and has no id', async () => {
-      const tx = createSafeTx()
+      const tx = createMockSafeTransaction({
+        to: '0x123',
+        data: '0x0',
+      })
       tx.addSignature(generatePreValidatedSignature('0x1234567890123456789012345678901234567890'))
 
       const proposedTx = await dispatchTxProposal({ chainId: '4', safeAddress: '0x123', sender: '0x456', safeTx: tx })
@@ -249,7 +230,10 @@ describe('txSender', () => {
     })
 
     it('should dispatch a SIGNATURE_PROPOSED event if tx has signatures and an id', async () => {
-      const tx = createSafeTx()
+      const tx = createMockSafeTransaction({
+        to: '0x123',
+        data: '0x0',
+      })
       tx.addSignature(generatePreValidatedSignature('0x1234567890123456789012345678901234567890'))
 
       const proposedTx = await dispatchTxProposal({

diff --git a/src/store/batchSlice.ts b/src/store/batchSlice.ts
@@ -31,7 +31,7 @@ export const batchSlice = createSlice({
     ) => {
       const { chainId, safeAddress, items } = action.payload
       state[chainId] = state[chainId] || {}
-      // @ts-ignore
+      // @ts-expect-error
       state[chainId][safeAddress] = items
     },
 
@@ -47,7 +47,6 @@ export const batchSlice = createSlice({
       const { chainId, safeAddress, txDetails } = action.payload
       state[chainId] = state[chainId] || {}
       state[chainId][safeAddress] = state[chainId][safeAddress] || []
-      // @ts-ignore
       state[chainId][safeAddress].push({
         id: Math.random().toString(36).slice(2),
         timestamp: Date.now(),