fix: Disable submit button if there is a high or critical risk (#2974)

* fix: Disable submit button if there is a high or critical risk * test: Add tests for SignForm * test: Add more tests for SignForm * test: Add happy path tests for ExecuteForm * test: Add more tests for ExecuteForm * refactor: Extract default props in SignForm test * refactor: Only render ApprovalEditor once * test: Add initial SignOrExecuteForm test * test: Add more SignOrExecuteForm tests * test: Implement missing tests, add TODO
safe-global · Dec 11, 2023 · a34c408 · a34c408
1 parent f162175
commit a34c408
Show file tree

Hide file tree

Showing 13 changed files with 671 additions and 68 deletions.
diff --git a/src/components/tx-flow/flows/SafeAppsTx/ReviewSafeAppsTx.tsx b/src/components/tx-flow/flows/SafeAppsTx/ReviewSafeAppsTx.tsx
@@ -1,6 +1,5 @@
-import { useContext, useEffect, useMemo, useState } from 'react'
+import { useContext, useEffect, useMemo } from 'react'
 import type { ReactElement } from 'react'
-import { ErrorBoundary } from '@sentry/react'
 import type { SafeTransaction } from '@safe-global/safe-core-sdk-types'
 import SendToBlock from '@/components/tx-flow/flows/TokenTransfer/SendToBlock'
 import SignOrExecuteForm from '@/components/tx/SignOrExecuteForm'
@@ -13,7 +12,6 @@ import useOnboard from '@/hooks/wallets/useOnboard'
 import useSafeInfo from '@/hooks/useSafeInfo'
 import useHighlightHiddenTab from '@/hooks/useHighlightHiddenTab'
 import { SafeTxContext } from '@/components/tx-flow/SafeTxProvider'
-import ApprovalEditor from '@/components/tx/ApprovalEditor'
 import { getInteractionTitle, isTxValid } from '@/components/safe-apps/utils'
 import ErrorMessage from '@/components/tx/ErrorMessage'
 import { asError } from '@/services/exceptions/utils'
@@ -33,15 +31,14 @@ const ReviewSafeAppsTx = ({
   const { safe } = useSafeInfo()
   const onboard = useOnboard()
   const chain = useCurrentChain()
-  const [txList, setTxList] = useState(txs)
   const { safeTx, setSafeTx, safeTxError, setSafeTxError } = useContext(SafeTxContext)
 
   useHighlightHiddenTab()
 
   useEffect(() => {
     const createSafeTx = async (): Promise<SafeTransaction> => {
-      const isMultiSend = txList.length > 1
-      const tx = isMultiSend ? await createMultiSendCallOnlyTx(txList) : await createTx(txList[0])
+      const isMultiSend = txs.length > 1
+      const tx = isMultiSend ? await createMultiSendCallOnlyTx(txs) : await createTx(txs[0])
 
       if (params?.safeTxGas) {
         // FIXME: do it properly via the Core SDK
@@ -53,7 +50,7 @@ const ReviewSafeAppsTx = ({
     }
 
     createSafeTx().then(setSafeTx).catch(setSafeTxError)
-  }, [txList, setSafeTx, setSafeTxError, params])
+  }, [txs, setSafeTx, setSafeTxError, params])
 
   const handleSubmit = async (txId: string) => {
     if (!safeTx || !onboard) return
@@ -82,10 +79,6 @@ const ReviewSafeAppsTx = ({
 
   return (
     <SignOrExecuteForm onSubmit={handleSubmit} origin={origin}>
-      <ErrorBoundary fallback={<div>Error parsing data</div>}>
-        <ApprovalEditor safeTransaction={safeTx} updateTransaction={setTxList} />
-      </ErrorBoundary>
-
       {safeTx ? (
         <SendToBlock address={safeTx.data.to} title={getInteractionTitle(safeTx.data.value || '', chain)} />
       ) : error ? (

diff --git a/src/components/tx/ApprovalEditor/ApprovalEditor.test.tsx b/src/components/tx/ApprovalEditor/ApprovalEditor.test.tsx
@@ -1,3 +1,4 @@
+import { safeSignatureBuilder, safeTxBuilder } from '@/tests/builders/safeTx'
 import { render } from '@/tests/test-utils'
 import ApprovalEditor from '.'
 import { TokenType } from '@safe-global/safe-gateway-typescript-sdk'
@@ -44,7 +45,7 @@ describe('ApprovalEditor', () => {
     expect(await result.queryByTestId('approval-editor-loading')).toBeInTheDocument()
   })
 
-  it('renders a read-only view if there is no update callback', async () => {
+  it('renders a read-only view if the transaction contains signatures', async () => {
     const mockApprovalInfo = {
       tokenInfo: { symbol: 'TST', decimals: 18, address: '0x3', type: TokenType.ERC20 },
       tokenAddress: '0x1',
@@ -53,7 +54,11 @@ describe('ApprovalEditor', () => {
       amountFormatted: '420.0',
     }
     jest.spyOn(approvalInfos, 'useApprovalInfos').mockReturnValue([[mockApprovalInfo], undefined, false])
-    const mockSafeTx = createMockSafeTransaction({ to: '0x1', data: '0x', operation: OperationType.DelegateCall })
+    const mockSafeTx = safeTxBuilder()
+      .with({
+        signatures: new Map().set('0x1', safeSignatureBuilder().build()),
+      })
+      .build()
 
     const result = render(<ApprovalEditor safeTransaction={mockSafeTx} />)
 
@@ -65,7 +70,7 @@ describe('ApprovalEditor', () => {
     expect(result.getByText('0x2'))
   })
 
-  it('renders a form if there is an update callback', async () => {
+  it('renders a form if there are no signatures', async () => {
     const mockApprovalInfo = {
       tokenInfo: { symbol: 'TST', decimals: 18, address: '0x3', type: TokenType.ERC20 },
       tokenAddress: '0x1',
@@ -76,7 +81,7 @@ describe('ApprovalEditor', () => {
     jest.spyOn(approvalInfos, 'useApprovalInfos').mockReturnValue([[mockApprovalInfo], undefined, false])
     const mockSafeTx = createMockSafeTransaction({ to: '0x1', data: '0x', operation: OperationType.DelegateCall })
 
-    const result = render(<ApprovalEditor safeTransaction={mockSafeTx} updateTransaction={jest.fn} />)
+    const result = render(<ApprovalEditor safeTransaction={mockSafeTx} />)
 
     const amountInput = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
 

diff --git a/src/components/tx/ApprovalEditor/index.tsx b/src/components/tx/ApprovalEditor/index.tsx
@@ -1,5 +1,8 @@
+import { SafeTxContext } from '@/components/tx-flow/SafeTxProvider'
+import { createMultiSendCallOnlyTx, createTx } from '@/services/tx/tx-sender'
 import { Alert, Box, Divider, Skeleton, SvgIcon, Typography } from '@mui/material'
-import { type MetaTransactionData, type SafeTransaction } from '@safe-global/safe-core-sdk-types'
+import { type SafeTransaction } from '@safe-global/safe-core-sdk-types'
+import { useContext } from 'react'
 import css from './styles.module.css'
 import { ApprovalEditorForm } from './ApprovalEditorForm'
 import { updateApprovalTxs } from './utils/approvals'
@@ -25,29 +28,27 @@ const Title = () => {
   )
 }
 
-export const ApprovalEditor = ({
-  safeTransaction,
-  updateTransaction,
-}: {
-  safeTransaction: SafeTransaction | undefined
-  updateTransaction?: (txs: MetaTransactionData[]) => void
-}) => {
+export const ApprovalEditor = ({ safeTransaction }: { safeTransaction: SafeTransaction | undefined }) => {
+  const { setSafeTx, setSafeTxError } = useContext(SafeTxContext)
   const [readableApprovals, error, loading] = useApprovalInfos(safeTransaction)
 
   if (readableApprovals?.length === 0 || !safeTransaction) {
     return null
   }
 
   const updateApprovals = (approvals: string[]) => {
-    if (!updateTransaction) return
-
     const extractedTxs = decodeSafeTxToBaseTransactions(safeTransaction)
-
     const updatedTxs = updateApprovalTxs(approvals, readableApprovals, extractedTxs)
-    updateTransaction(updatedTxs)
+
+    const createSafeTx = async (): Promise<SafeTransaction> => {
+      const isMultiSend = updatedTxs.length > 1
+      return isMultiSend ? createMultiSendCallOnlyTx(updatedTxs) : createTx(updatedTxs[0])
+    }
+
+    createSafeTx().then(setSafeTx).catch(setSafeTxError)
   }
 
-  const isReadOnly = updateTransaction === undefined
+  const isReadOnly = safeTransaction.signatures.size > 0
 
   return (
     <Box display="flex" flexDirection="column" gap={2} mb={3}>

diff --git a/src/components/tx/ExecuteCheckbox/index.tsx b/src/components/tx/ExecuteCheckbox/index.tsx
@@ -31,8 +31,15 @@ const ExecuteCheckbox = ({ onChange }: { onChange: (checked: boolean) => void })
           }
           control={<Radio />}
           className={css.radio}
+          data-testid="execute-checkbox"
+        />
+        <FormControlLabel
+          value="false"
+          label={<>No, later</>}
+          control={<Radio />}
+          className={css.radio}
+          data-testid="sign-checkbox"
         />
-        <FormControlLabel value="false" label={<>No, later</>} control={<Radio />} className={css.radio} />
       </RadioGroup>
     </>
   )

diff --git a/src/components/tx/SignOrExecuteForm/ExecuteForm.tsx b/src/components/tx/SignOrExecuteForm/ExecuteForm.tsx
@@ -1,4 +1,5 @@
 import BalanceInfo from '@/components/tx/BalanceInfo'
+import madProps from '@/utils/mad-props'
 import { type ReactElement, type SyntheticEvent, useContext, useState } from 'react'
 import { CircularProgress, Box, Button, CardActions, Divider } from '@mui/material'
 import classNames from 'classnames'
@@ -28,31 +29,36 @@ import { TxSecurityContext } from '../security/shared/TxSecurityContext'
 import useIsSafeOwner from '@/hooks/useIsSafeOwner'
 import NonOwnerError from '@/components/tx/SignOrExecuteForm/NonOwnerError'
 
-const ExecuteForm = ({
+export const ExecuteForm = ({
   safeTx,
   txId,
   onSubmit,
   disableSubmit = false,
   origin,
   onlyExecute,
   isCreation,
+  isOwner,
+  isExecutionLoop,
+  relays,
+  txActions,
+  txSecurity,
 }: SignOrExecuteProps & {
+  isOwner: ReturnType<typeof useIsSafeOwner>
+  isExecutionLoop: ReturnType<typeof useIsExecutionLoop>
+  relays: ReturnType<typeof useRelaysBySafe>
+  txActions: ReturnType<typeof useTxActions>
+  txSecurity: ReturnType<typeof useTxSecurityContext>
   safeTx?: SafeTransaction
 }): ReactElement => {
   // Form state
   const [isSubmittable, setIsSubmittable] = useState<boolean>(true)
   const [submitError, setSubmitError] = useState<Error | undefined>()
 
   // Hooks
-  const isOwner = useIsSafeOwner()
   const currentChain = useCurrentChain()
-  const { executeTx } = useTxActions()
-  const [relays] = useRelaysBySafe()
+  const { executeTx } = txActions
   const { setTxFlow } = useContext(TxModalContext)
-  const { needsRiskConfirmation, isRiskConfirmed, setIsRiskIgnored } = useContext(TxSecurityContext)
-
-  // Check that the transaction is executable
-  const isExecutionLoop = useIsExecutionLoop()
+  const { needsRiskConfirmation, isRiskConfirmed, setIsRiskIgnored } = txSecurity
 
   // We default to relay, but the option is only shown if we canRelay
   const [executionMethod, setExecutionMethod] = useState(ExecutionMethod.RELAY)
@@ -61,7 +67,7 @@ const ExecuteForm = ({
   const [walletCanRelay] = useWalletCanRelay(safeTx)
 
   // The transaction can/will be relayed
-  const canRelay = walletCanRelay && hasRemainingRelays(relays)
+  const canRelay = walletCanRelay && hasRemainingRelays(relays[0])
   const willRelay = canRelay && executionMethod === ExecutionMethod.RELAY
 
   // Estimate gas limit
@@ -102,7 +108,13 @@ const ExecuteForm = ({
   }
 
   const cannotPropose = !isOwner && !onlyExecute
-  const submitDisabled = !safeTx || !isSubmittable || disableSubmit || isExecutionLoop || cannotPropose
+  const submitDisabled =
+    !safeTx ||
+    !isSubmittable ||
+    disableSubmit ||
+    isExecutionLoop ||
+    cannotPropose ||
+    (needsRiskConfirmation && !isRiskConfirmed)
 
   return (
     <>
@@ -123,7 +135,7 @@ const ExecuteForm = ({
               <ExecutionMethodSelector
                 executionMethod={executionMethod}
                 setExecutionMethod={setExecutionMethod}
-                relays={relays}
+                relays={relays[0]}
               />
             </div>
           )}
@@ -168,4 +180,12 @@ const ExecuteForm = ({
   )
 }
 
-export default ExecuteForm
+const useTxSecurityContext = () => useContext(TxSecurityContext)
+
+export default madProps(ExecuteForm, {
+  isOwner: useIsSafeOwner,
+  isExecutionLoop: useIsExecutionLoop,
+  relays: useRelaysBySafe,
+  txActions: useTxActions,
+  txSecurity: useTxSecurityContext,
+})
diff --git a/src/components/tx/SignOrExecuteForm/SignForm.tsx b/src/components/tx/SignOrExecuteForm/SignForm.tsx
@@ -1,3 +1,4 @@
+import madProps from '@/utils/mad-props'
 import { type ReactElement, type SyntheticEvent, useContext, useState } from 'react'
 import { CircularProgress, Box, Button, CardActions, Divider } from '@mui/material'
 
@@ -15,7 +16,7 @@ import { TxSecurityContext } from '../security/shared/TxSecurityContext'
 import NonOwnerError from '@/components/tx/SignOrExecuteForm/NonOwnerError'
 import BatchButton from './BatchButton'
 
-const SignForm = ({
+export const SignForm = ({
   safeTx,
   txId,
   onSubmit,
@@ -24,18 +25,23 @@ const SignForm = ({
   isBatch,
   isBatchable,
   isCreation,
+  isOwner,
+  txActions,
+  txSecurity,
 }: SignOrExecuteProps & {
+  isOwner: ReturnType<typeof useIsSafeOwner>
+  txActions: ReturnType<typeof useTxActions>
+  txSecurity: ReturnType<typeof useTxSecurityContext>
   safeTx?: SafeTransaction
 }): ReactElement => {
   // Form state
   const [isSubmittable, setIsSubmittable] = useState<boolean>(true)
   const [submitError, setSubmitError] = useState<Error | undefined>()
 
   // Hooks
-  const isOwner = useIsSafeOwner()
-  const { signTx, addToBatch } = useTxActions()
+  const { signTx, addToBatch } = txActions
   const { setTxFlow } = useContext(TxModalContext)
-  const { needsRiskConfirmation, isRiskConfirmed, setIsRiskIgnored } = useContext(TxSecurityContext)
+  const { needsRiskConfirmation, isRiskConfirmed, setIsRiskIgnored } = txSecurity
   const hasSigned = useAlreadySigned(safeTx)
 
   // On modal submit
@@ -76,7 +82,8 @@ const SignForm = ({
   }
 
   const cannotPropose = !isOwner
-  const submitDisabled = !safeTx || !isSubmittable || disableSubmit || cannotPropose
+  const submitDisabled =
+    !safeTx || !isSubmittable || disableSubmit || cannotPropose || (needsRiskConfirmation && !isRiskConfirmed)
 
   return (
     <form onSubmit={handleSubmit}>
@@ -123,4 +130,10 @@ const SignForm = ({
   )
 }
 
-export default SignForm
+const useTxSecurityContext = () => useContext(TxSecurityContext)
+
+export default madProps(SignForm, {
+  isOwner: useIsSafeOwner,
+  txActions: useTxActions,
+  txSecurity: useTxSecurityContext,
+})