Fix: warn if tx was already signed by connected wallet (#2322)

* Fix: warn if wallet already signed a tx * Adjust TxLayout margin * Replace dashboard link with modal close * fix: align transaction links (#2332) * Fix: mobile pairing check (#2327) * Fix: mobile pairing check * Fix tests * fix: align transaction links --------- Co-authored-by: katspaugh <[email protected]> * Rm NewTxMenu margin * fix e2e create_tx * fix e2e nfts --------- Co-authored-by: Aaron Cook <[email protected]> Co-authored-by: Franco Venica <[email protected]>
safe-global · Aug 2, 2023 · 657da0f · 657da0f
1 parent 2658166
commit 657da0f
Show file tree

Hide file tree

Showing 12 changed files with 100 additions and 47 deletions.
diff --git a/cypress/e2e/smoke/create_tx.cy.js b/cypress/e2e/smoke/create_tx.cy.js
@@ -69,7 +69,7 @@ describe('Queue a transaction on 1/N', () => {
     cy.contains('Estimated fee').should('exist')
 
     // Asserting the sponsored info is present
-    cy.contains('Execute').should('be.visible')
+    cy.contains('Execute').scrollIntoView().should('be.visible')
 
     cy.get('span').contains('Estimated fee').next().should('have.css', 'text-decoration-line', 'line-through')
     cy.contains('Transactions per hour')
@@ -103,7 +103,7 @@ describe('Queue a transaction on 1/N', () => {
       .type(currentNonce + 10, { force: true })
       .type('{enter}', { force: true })
 
-    cy.contains('Submit').click()
+    cy.contains('Sign').click()
   })
 
   it('should click the notification and see the transaction queued', () => {

diff --git a/cypress/e2e/smoke/nfts.cy.js b/cypress/e2e/smoke/nfts.cy.js
@@ -84,7 +84,7 @@ describe('Assets > NFTs', () => {
       cy.contains('1')
       cy.contains('2')
       cy.get('b:contains("safeTransferFrom")').should('have.length', 2)
-      cy.contains('button:not([disabled])', 'Submit')
+      cy.contains('button:not([disabled])', 'Execute')
     })
   })
 })
diff --git a/src/components/tx-flow/common/TxLayout/index.tsx b/src/components/tx-flow/common/TxLayout/index.tsx
@@ -9,11 +9,10 @@ import { TxInfoProvider } from '@/components/tx-flow/TxInfoProvider'
 import TxNonce from '../TxNonce'
 import TxStatusWidget from '../TxStatusWidget'
 import css from './styles.module.css'
-import { TxSimulationMessage } from '@/components/tx/security/tenderly'
 import SafeLogo from '@/public/images/logo-no-text.svg'
-import { RedefineMessage } from '@/components/tx/security/redefine'
 import { TxSecurityProvider } from '@/components/tx/security/shared/TxSecurityContext'
 import ChainIndicator from '@/components/common/ChainIndicator'
+import SecurityWarnings from '@/components/tx/security/SecurityWarnings'
 
 const TxLayoutHeader = ({
   hideNonce,
@@ -147,9 +146,7 @@ const TxLayout = ({
                   )}
 
                   <Box className={css.sticky}>
-                    <RedefineMessage />
-
-                    <TxSimulationMessage />
+                    <SecurityWarnings />
                   </Box>
                 </Grid>
               </Grid>

diff --git a/src/components/tx-flow/common/TxLayout/styles.module.css b/src/components/tx-flow/common/TxLayout/styles.module.css
@@ -1,5 +1,5 @@
 .container {
-  margin-top: 42px;
+  margin-top: 10px;
 }
 
 .header {

diff --git a/src/components/tx-flow/flows/NewTx/styles.module.css b/src/components/tx-flow/flows/NewTx/styles.module.css
@@ -1,7 +1,3 @@
-.container {
-  margin-top: 50px;
-}
-
 .chain {
   align-self: flex-end;
   margin-bottom: var(--space-2);

diff --git a/src/components/tx-flow/flows/SuccessScreen/index.tsx b/src/components/tx-flow/flows/SuccessScreen/index.tsx
@@ -1,26 +1,27 @@
-import { useRouter } from 'next/router'
 import StatusMessage from './StatusMessage'
 import StatusStepper from './StatusStepper'
-import { AppRoutes } from '@/config/routes'
 import { Button, Container, Divider, Paper } from '@mui/material'
 import classnames from 'classnames'
 import Link from 'next/link'
-import { type UrlObject } from 'url'
 import css from './styles.module.css'
 import { useAppSelector } from '@/store'
 import { selectPendingTxById } from '@/store/pendingTxsSlice'
-import { useEffect, useState } from 'react'
-import { getBlockExplorerLink } from '@/utils/chains'
+import { useEffect, useState, useCallback, useContext } from 'react'
+import { getTxLink } from '@/hooks/useTxNotifications'
 import { useCurrentChain } from '@/hooks/useChains'
 import { TxEvent, txSubscribe } from '@/services/tx/txEvents'
+import useSafeInfo from '@/hooks/useSafeInfo'
+import { TxModalContext } from '../..'
 
 export const SuccessScreen = ({ txId }: { txId: string }) => {
   const [localTxHash, setLocalTxHash] = useState<string>()
   const [error, setError] = useState<Error>()
-  const router = useRouter()
+  const { setTxFlow } = useContext(TxModalContext)
   const chain = useCurrentChain()
   const pendingTx = useAppSelector((state) => selectPendingTxById(state, txId))
+  const { safeAddress } = useSafeInfo()
   const { txHash = '', status } = pendingTx || {}
+  const txLink = chain && getTxLink(txId, chain, safeAddress)
 
   useEffect(() => {
     if (!txHash) return
@@ -38,12 +39,9 @@ export const SuccessScreen = ({ txId }: { txId: string }) => {
     return () => unsubFns.forEach((unsubscribe) => unsubscribe())
   }, [txId])
 
-  const homeLink: UrlObject = {
-    pathname: AppRoutes.home,
-    query: { safe: router.query.safe },
-  }
-
-  const txLink = chain && localTxHash ? getBlockExplorerLink(chain, localTxHash) : undefined
+  const onFinishClick = useCallback(() => {
+    setTxFlow(undefined)
+  }, [setTxFlow])
 
   return (
     <Container
@@ -69,17 +67,19 @@ export const SuccessScreen = ({ txId }: { txId: string }) => {
       )}
 
       <Divider />
+
       <div className={classnames(css.row, css.buttons)}>
-        <Link href={homeLink} passHref>
-          <Button variant="outlined" size="small">
-            Back to dashboard
-          </Button>
-        </Link>
         {txLink && (
-          <Button href={txLink.href} target="_blank" rel="noreferrer" variant="outlined" size="small">
-            View transaction
-          </Button>
+          <Link {...txLink} passHref target="_blank" rel="noreferrer">
+            <Button variant="outlined" size="small">
+              View transaction
+            </Button>
+          </Link>
         )}
+
+        <Button variant="contained" size="small" onClick={onFinishClick}>
+          Finish
+        </Button>
       </div>
     </Container>
   )

diff --git a/src/components/tx/SignOrExecuteForm/ExecuteForm.tsx b/src/components/tx/SignOrExecuteForm/ExecuteForm.tsx
@@ -154,7 +154,7 @@ const ExecuteForm = ({
           <CheckWallet allowNonOwner={onlyExecute}>
             {(isOk) => (
               <Button variant="contained" type="submit" disabled={!isOk || submitDisabled}>
-                Submit
+                Execute
               </Button>
             )}
           </CheckWallet>

diff --git a/src/components/tx/SignOrExecuteForm/SignForm.tsx b/src/components/tx/SignOrExecuteForm/SignForm.tsx
@@ -5,7 +5,7 @@ import ErrorMessage from '@/components/tx/ErrorMessage'
 import { logError, Errors } from '@/services/exceptions'
 import useIsSafeOwner from '@/hooks/useIsSafeOwner'
 import CheckWallet from '@/components/common/CheckWallet'
-import { useTxActions } from './hooks'
+import { useAlreadySigned, useTxActions } from './hooks'
 import type { SignOrExecuteProps } from '.'
 import type { SafeTransaction } from '@safe-global/safe-core-sdk-types'
 import { TxModalContext } from '@/components/tx-flow'
@@ -32,6 +32,7 @@ const SignForm = ({
   const { signTx } = useTxActions()
   const { setTxFlow } = useContext(TxModalContext)
   const { needsRiskConfirmation, isRiskConfirmed, setIsRiskIgnored } = useContext(TxSecurityContext)
+  const hasSigned = useAlreadySigned(safeTx)
 
   // On modal submit
   const handleSubmit = async (e: SyntheticEvent) => {
@@ -64,6 +65,8 @@ const SignForm = ({
 
   return (
     <form onSubmit={handleSubmit}>
+      {hasSigned && <ErrorMessage level="warning">You have already signed this transaction.</ErrorMessage>}
+
       {cannotPropose ? (
         <NonOwnerError />
       ) : (
@@ -79,7 +82,7 @@ const SignForm = ({
         <CheckWallet>
           {(isOk) => (
             <Button variant="contained" type="submit" disabled={!isOk || submitDisabled}>
-              Submit
+              Sign
             </Button>
           )}
         </CheckWallet>

diff --git a/src/components/tx/SignOrExecuteForm/hooks.test.ts b/src/components/tx/SignOrExecuteForm/hooks.test.ts
@@ -10,7 +10,7 @@ import * as pending from '@/hooks/usePendingTxs'
 import * as txSender from '@/services/tx/tx-sender/dispatch'
 import * as onboardHooks from '@/hooks/wallets/useOnboard'
 import { type OnboardAPI } from '@web3-onboard/core'
-import { useImmediatelyExecutable, useIsExecutionLoop, useTxActions, useValidateNonce } from './hooks'
+import { useAlreadySigned, useImmediatelyExecutable, useIsExecutionLoop, useTxActions, useValidateNonce } from './hooks'
 
 const createSafeTx = (data = '0x'): SafeTransaction => {
   return {
@@ -542,4 +542,43 @@ describe('SignOrExecute hooks', () => {
       expect(relaySpy).not.toHaveBeenCalled()
     })
   })
+
+  describe('useAlreadySigned', () => {
+    it('should return true if wallet already signed a tx', () => {
+      // Wallet
+      jest.spyOn(wallet, 'default').mockReturnValue({
+        chainId: '1',
+        label: 'MetaMask',
+        address: '0x1234567890000000000000000000000000000000',
+      } as unknown as ConnectedWallet)
+
+      const tx = createSafeTx()
+      tx.addSignature({
+        signer: '0x1234567890000000000000000000000000000000',
+        data: '0x0001',
+        staticPart: () => '',
+        dynamicPart: () => '',
+      })
+      const { result } = renderHook(() => useAlreadySigned(tx))
+      expect(result.current).toEqual(true)
+    })
+  })
+  it('should return false if wallet has not signed a tx yet', () => {
+    // Wallet
+    jest.spyOn(wallet, 'default').mockReturnValue({
+      chainId: '1',
+      label: 'MetaMask',
+      address: '0x1234567890000000000000000000000000000000',
+    } as unknown as ConnectedWallet)
+
+    const tx = createSafeTx()
+    tx.addSignature({
+      signer: '0x00000000000000000000000000000000000000000',
+      data: '0x0001',
+      staticPart: () => '',
+      dynamicPart: () => '',
+    })
+    const { result } = renderHook(() => useAlreadySigned(tx))
+    expect(result.current).toEqual(false)
+  })
 })
diff --git a/src/components/tx/SignOrExecuteForm/hooks.ts b/src/components/tx/SignOrExecuteForm/hooks.ts
@@ -179,3 +179,10 @@ export const useSafeTxGas = (safeTx: SafeTransaction | undefined): number | unde
 
   return safeTxGas
 }
+
+export const useAlreadySigned = (safeTx: SafeTransaction | undefined): boolean => {
+  const wallet = useWallet()
+  const hasSigned =
+    safeTx && wallet && (safeTx.signatures.has(wallet.address.toLowerCase()) || safeTx.signatures.has(wallet.address))
+  return Boolean(hasSigned)
+}
diff --git a/src/components/tx/security/SecurityWarnings.tsx b/src/components/tx/security/SecurityWarnings.tsx
@@ -0,0 +1,11 @@
+import { RedefineMessage } from './redefine'
+import { TxSimulationMessage } from './tenderly'
+
+const SecurityWarnings = () => (
+  <>
+    <RedefineMessage />
+    <TxSimulationMessage />
+  </>
+)
+
+export default SecurityWarnings
diff --git a/src/hooks/useTxNotifications.ts b/src/hooks/useTxNotifications.ts
@@ -43,7 +43,11 @@ enum Variant {
 
 const successEvents = [TxEvent.PROPOSED, TxEvent.SIGNATURE_PROPOSED, TxEvent.ONCHAIN_SIGNATURE_SUCCESS, TxEvent.SUCCESS]
 
-const getTxLink = (txId: string, chain: ChainInfo, safeAddress: string): { href: LinkProps['href']; title: string } => {
+export const getTxLink = (
+  txId: string,
+  chain: ChainInfo,
+  safeAddress: string,
+): { href: LinkProps['href']; title: string } => {
   return {
     href: {
       pathname: AppRoutes.transactions.tx,
@@ -53,14 +57,6 @@ const getTxLink = (txId: string, chain: ChainInfo, safeAddress: string): { href:
   }
 }
 
-const getTxExplorerLink = (txHash: string, chain: ChainInfo): { href: LinkProps['href']; title: string } => {
-  const { href } = getExplorerLink(txHash, chain.blockExplorerUriTemplate)
-  return {
-    href,
-    title: 'View on explorer',
-  }
-}
-
 const useTxNotifications = (): void => {
   const dispatch = useAppDispatch()
   const chain = useCurrentChain()
@@ -90,7 +86,11 @@ const useTxNotifications = (): void => {
             detailedMessage: isError ? detail.error.message : undefined,
             groupKey,
             variant: isError ? Variant.ERROR : isSuccess ? Variant.SUCCESS : Variant.INFO,
-            link: txId ? getTxLink(txId, chain, safeAddress) : txHash ? getTxExplorerLink(txHash, chain) : undefined,
+            link: txId
+              ? getTxLink(txId, chain, safeAddress)
+              : txHash
+              ? getExplorerLink(txHash, chain.blockExplorerUriTemplate)
+              : undefined,
           }),
         )
       }),