Skip to content

Commit

Permalink
Fix: warn if tx was already signed by connected wallet (#2322)
Browse files Browse the repository at this point in the history
* 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]>
  • Loading branch information
3 people authored Aug 2, 2023
1 parent 2658166 commit 657da0f
Show file tree
Hide file tree
Showing 12 changed files with 100 additions and 47 deletions.
4 changes: 2 additions & 2 deletions cypress/e2e/smoke/create_tx.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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', () => {
Expand Down
2 changes: 1 addition & 1 deletion cypress/e2e/smoke/nfts.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
})
})
})
7 changes: 2 additions & 5 deletions src/components/tx-flow/common/TxLayout/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -147,9 +146,7 @@ const TxLayout = ({
)}

<Box className={css.sticky}>
<RedefineMessage />

<TxSimulationMessage />
<SecurityWarnings />
</Box>
</Grid>
</Grid>
Expand Down
2 changes: 1 addition & 1 deletion src/components/tx-flow/common/TxLayout/styles.module.css
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
.container {
margin-top: 42px;
margin-top: 10px;
}

.header {
Expand Down
4 changes: 0 additions & 4 deletions src/components/tx-flow/flows/NewTx/styles.module.css
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
.container {
margin-top: 50px;
}

.chain {
align-self: flex-end;
margin-bottom: var(--space-2);
Expand Down
40 changes: 20 additions & 20 deletions src/components/tx-flow/flows/SuccessScreen/index.tsx
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand All @@ -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>
)
Expand Down
2 changes: 1 addition & 1 deletion src/components/tx/SignOrExecuteForm/ExecuteForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ const ExecuteForm = ({
<CheckWallet allowNonOwner={onlyExecute}>
{(isOk) => (
<Button variant="contained" type="submit" disabled={!isOk || submitDisabled}>
Submit
Execute
</Button>
)}
</CheckWallet>
Expand Down
7 changes: 5 additions & 2 deletions src/components/tx/SignOrExecuteForm/SignForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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) => {
Expand Down Expand Up @@ -64,6 +65,8 @@ const SignForm = ({

return (
<form onSubmit={handleSubmit}>
{hasSigned && <ErrorMessage level="warning">You have already signed this transaction.</ErrorMessage>}

{cannotPropose ? (
<NonOwnerError />
) : (
Expand All @@ -79,7 +82,7 @@ const SignForm = ({
<CheckWallet>
{(isOk) => (
<Button variant="contained" type="submit" disabled={!isOk || submitDisabled}>
Submit
Sign
</Button>
)}
</CheckWallet>
Expand Down
41 changes: 40 additions & 1 deletion src/components/tx/SignOrExecuteForm/hooks.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
})
})
7 changes: 7 additions & 0 deletions src/components/tx/SignOrExecuteForm/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
11 changes: 11 additions & 0 deletions src/components/tx/security/SecurityWarnings.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { RedefineMessage } from './redefine'
import { TxSimulationMessage } from './tenderly'

const SecurityWarnings = () => (
<>
<RedefineMessage />
<TxSimulationMessage />
</>
)

export default SecurityWarnings
20 changes: 10 additions & 10 deletions src/hooks/useTxNotifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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()
Expand Down Expand Up @@ -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,
}),
)
}),
Expand Down

0 comments on commit 657da0f

Please sign in to comment.