Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Fix: conditions to display "Execute transaction" #3257

Merged
merged 28 commits into from
Jan 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
a216e28
Build: create a hook for getting the recommended nonce
Jan 6, 2022
0266a1e
Build: extract "can execute transaction" to a hook
Jan 6, 2022
b084b55
Build: remove unused return type
Jan 6, 2022
f0fb4cf
Build: use "can execute" hook to retrieve condition to show the checkbox
Jan 6, 2022
ab24bd3
Refactor: use ComponentProps to get component types
Jan 6, 2022
240478a
Fix: remove from tests logic extracted to hook
Jan 6, 2022
ffc1bfc
refactor: refactor the useCanExecuteType to test the logic
Jan 7, 2022
d56093b
refactor: exclude the spending limit logic from useCanTxExecute
Jan 8, 2022
57612cf
refactor: use useCanTxExecute in other review modals
Jan 8, 2022
a989041
fix: improve useCanTxExecute types and logic
Jan 8, 2022
1fa2d51
refactor: do not return isExecution from useEstimateTransactionGas
Jan 8, 2022
e1c7de6
fix: approve modal logic
Jan 9, 2022
93ef7c2
build: include PENDING_FAILED transactions in awaiting for execution
Jan 9, 2022
674c8a8
build: flag execution to calculate tx gas
Jan 9, 2022
5cca7a9
build: include PENDING_FAILED tx status in showing execution tooltip …
Jan 9, 2022
31f0efb
fix: adjust tests to reflect latest logic
Jan 9, 2022
01ac45e
fix: add missing hook dependency
Jan 10, 2022
144a35c
fix: avoid race conditions when fetching from the backend
Jan 10, 2022
2befeb9
fix: implement minor PR comments
Jan 10, 2022
738e7a2
build: display the Execute checkbox if safe nonce is edited to be next
Jan 10, 2022
9b622dd
fix: update tests to take manualSafeNonce
Jan 10, 2022
be54a48
fix: remove wrong check
Jan 10, 2022
f501dde
refactor: rename execution related variables
Jan 11, 2022
669f8e4
fix: rename variable
Jan 11, 2022
3f26613
Obtain isOffChainSignature by calling a function instead of prop dril…
Jan 13, 2022
fa26e8d
Calculate tx gas even for off chain transactions
Jan 13, 2022
572c8f8
Merge branch 'dev' into hide-execute-checkbox-conditions
DiogoSoaress Jan 13, 2022
7e8d179
fix TransactionFees rendering
Jan 13, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/ExecuteCheckbox/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ interface ExecuteCheckboxProps {
onChange: (val: boolean) => unknown
}

const ExecuteCheckbox = ({ onChange }: ExecuteCheckboxProps): ReactElement | null => {
const ExecuteCheckbox = ({ onChange }: ExecuteCheckboxProps): ReactElement => {
const handleChange = (e: React.ChangeEvent<HTMLInputElement>): void => {
onChange(e.target.checked)
}
Expand Down
6 changes: 2 additions & 4 deletions src/components/ReviewInfoText/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ import { lg, sm } from 'src/theme/variables'
import { TransactionFees } from '../TransactionsFees'
import { getRecommendedNonce } from 'src/logic/safe/api/fetchSafeTxGasEstimation'
import { extractSafeAddress } from 'src/routes/routes'
import { useEffect, useState } from 'react'
import { ComponentProps, useEffect, useState } from 'react'

type CustomReviewInfoTextProps = {
safeNonce?: string
testId?: string
}

type ReviewInfoTextProps = Parameters<typeof TransactionFees>[0] & CustomReviewInfoTextProps
type ReviewInfoTextProps = ComponentProps<typeof TransactionFees> & CustomReviewInfoTextProps

const ReviewInfoTextWrapper = styled.div`
background-color: ${({ theme }) => theme.colors.background};
Expand All @@ -27,7 +27,6 @@ export const ReviewInfoText = ({
gasCostFormatted,
isCreation,
isExecution,
isOffChainSignature,
safeNonce: txParamsSafeNonce = '',
testId,
txEstimationExecutionStatus,
Expand Down Expand Up @@ -88,7 +87,6 @@ export const ReviewInfoText = ({
gasCostFormatted={gasCostFormatted}
isCreation={isCreation}
isExecution={isExecution}
isOffChainSignature={isOffChainSignature}
txEstimationExecutionStatus={txEstimationExecutionStatus}
/>
)}
Expand Down
12 changes: 10 additions & 2 deletions src/components/TransactionsFees/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,30 @@ import Paragraph from 'src/components/layout/Paragraph'
import { getNativeCurrency } from 'src/config'
import { TransactionFailText } from 'src/components/TransactionFailText'
import { Text } from '@gnosis.pm/safe-react-components'
import useCanTxExecute from 'src/logic/hooks/useCanTxExecute'
import { providerSelector } from 'src/logic/wallets/store/selectors'
import { useSelector } from 'react-redux'
import { currentSafe } from 'src/logic/safe/store/selectors'
import { checkIfOffChainSignatureIsPossible } from 'src/logic/safe/safeTxSigner'

type TransactionFailTextProps = {
txEstimationExecutionStatus: EstimationStatus
gasCostFormatted?: string
isExecution: boolean
isCreation: boolean
isOffChainSignature: boolean
}

export const TransactionFees = ({
gasCostFormatted,
isExecution,
isCreation,
isOffChainSignature,
txEstimationExecutionStatus,
}: TransactionFailTextProps): React.ReactElement | null => {
const { currentVersion: safeVersion } = useSelector(currentSafe)
const { smartContractWallet } = useSelector(providerSelector)
const canTxExecute = useCanTxExecute(isExecution)
const isOffChainSignature = checkIfOffChainSignatureIsPossible(canTxExecute, smartContractWallet, safeVersion)

const nativeCurrency = getNativeCurrency()
let transactionAction
if (txEstimationExecutionStatus === EstimationStatus.LOADING) {
Expand Down
191 changes: 191 additions & 0 deletions src/logic/hooks/__tests__/useCanTxExecute.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
import { calculateCanTxExecute } from '../useCanTxExecute'

describe('useCanTxExecute tests', () => {
iamacook marked this conversation as resolved.
Show resolved Hide resolved
describe('calculateCanTxExecute tests', () => {
beforeEach(() => {
threshold = 1
isExecution = false
currentSafeNonce = 8
recommendedNonce = 8
txConfirmations = 0
preApprovingOwner = ''
manualSafeNonce = recommendedNonce
})
// to be overriden as necessary
let threshold
let preApprovingOwner
let txConfirmations
let currentSafeNonce
let recommendedNonce
let isExecution
let manualSafeNonce
it(`should return true if isExecution`, () => {
// given
isExecution = true

// when
const result = calculateCanTxExecute(
currentSafeNonce,
preApprovingOwner,
threshold,
txConfirmations,
recommendedNonce,
isExecution,
)

// then
expect(result).toBe(true)
})
it(`should return true if single owner and edited nonce is same as safeNonce`, () => {
// given
threshold = 1
currentSafeNonce = 8
recommendedNonce = 12
manualSafeNonce = 8

// when
const result = calculateCanTxExecute(
currentSafeNonce,
preApprovingOwner,
threshold,
txConfirmations,
recommendedNonce,
undefined,
manualSafeNonce,
)

// then
expect(result).toBe(true)
})
it(`should return false if single owner and edited nonce is different than safeNonce`, () => {
// given
threshold = 1
currentSafeNonce = 8
recommendedNonce = 8
manualSafeNonce = 20

// when
const result = calculateCanTxExecute(
currentSafeNonce,
preApprovingOwner,
threshold,
txConfirmations,
recommendedNonce,
undefined,
manualSafeNonce,
)

// then
expect(result).toBe(false)
})
it(`should return true if single owner and recommendedNonce is same as safeNonce`, () => {
// given
threshold = 1
currentSafeNonce = 8
recommendedNonce = 8

// when
const result = calculateCanTxExecute(
currentSafeNonce,
preApprovingOwner,
threshold,
txConfirmations,
recommendedNonce,
)

// then
expect(result).toBe(true)
})
it(`should return false if single owner and recommendedNonce is greater than safeNonce and no edited nonce`, () => {
// given
threshold = 1
currentSafeNonce = 8
recommendedNonce = 11
manualSafeNonce = undefined

// when
const result = calculateCanTxExecute(
currentSafeNonce,
preApprovingOwner,
threshold,
txConfirmations,
recommendedNonce,
undefined,
manualSafeNonce,
)

// then
expect(result).toBe(false)
})
it(`should return false if single owner and recommendedNonce is different than safeNonce`, () => {
// given
threshold = 1
currentSafeNonce = 8
recommendedNonce = 12

// when
const result = calculateCanTxExecute(
currentSafeNonce,
preApprovingOwner,
threshold,
txConfirmations,
recommendedNonce,
)

// then
expect(result).toBe(false)
})
it(`should return true if the safe threshold is reached for the transaction`, () => {
// given
threshold = 3
txConfirmations = 3

// when
const result = calculateCanTxExecute(
currentSafeNonce,
preApprovingOwner,
threshold,
txConfirmations,
recommendedNonce,
)

// then
expect(result).toBe(true)
})
it(`should return false if the number of confirmations does not meet the threshold and there is no preApprovingOwner`, () => {
// given
threshold = 5
txConfirmations = 4

// when
const result = calculateCanTxExecute(
currentSafeNonce,
preApprovingOwner,
threshold,
txConfirmations,
recommendedNonce,
)

// then
expect(result).toBe(false)
})
it(`should return true if the number of confirmations is one bellow the threshold but there is a preApprovingOwner`, () => {
// given
threshold = 5
preApprovingOwner = '0x29B1b813b6e84654Ca698ef5d7808E154364900B'
txConfirmations = 4

// when
const result = calculateCanTxExecute(
currentSafeNonce,
preApprovingOwner,
threshold,
txConfirmations,
recommendedNonce,
)

// then
expect(result).toBe(true)
})
})
})
74 changes: 1 addition & 73 deletions src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts
Original file line number Diff line number Diff line change
@@ -1,77 +1,5 @@
import {
checkIfTxIsApproveAndExecution,
checkIfTxIsCreation,
checkIfTxIsExecution,
} from 'src/logic/hooks/useEstimateTransactionGas'
import { checkIfTxIsApproveAndExecution, checkIfTxIsCreation } from 'src/logic/hooks/useEstimateTransactionGas'

describe('checkIfTxIsExecution', () => {
const mockedEthAccount = '0x29B1b813b6e84654Ca698ef5d7808E154364900B'
it(`should return true if the safe threshold is 1`, () => {
// given
const threshold = 1
const preApprovingOwner = undefined
const transactionConfirmations = 0
const transactionType = ''

// when
const result = checkIfTxIsExecution(threshold, preApprovingOwner, transactionConfirmations, transactionType)

// then
expect(result).toBe(true)
})
it(`should return true if the safe threshold is reached for the transaction`, () => {
// given
const threshold = 3
const preApprovingOwner = mockedEthAccount
const transactionConfirmations = 3
const transactionType = ''

// when
const result = checkIfTxIsExecution(threshold, preApprovingOwner, transactionConfirmations, transactionType)

// then
expect(result).toBe(true)
})
it(`should return true if the transaction is spendingLimit`, () => {
// given
const threshold = 5
const preApprovingOwner = undefined
const transactionConfirmations = 0
const transactionType = 'spendingLimit'

// when
const result = checkIfTxIsExecution(threshold, preApprovingOwner, transactionConfirmations, transactionType)

// then
expect(result).toBe(true)
})
it(`should return true if the number of confirmations is one bellow the threshold but there is a preApprovingOwner`, () => {
// given
const threshold = 5
const preApprovingOwner = mockedEthAccount
const transactionConfirmations = 4
const transactionType = undefined

// when
const result = checkIfTxIsExecution(threshold, preApprovingOwner, transactionConfirmations, transactionType)

// then
expect(result).toBe(true)
})
it(`should return false if the number of confirmations is one bellow the threshold and there is no preApprovingOwner`, () => {
// given
const threshold = 5
const preApprovingOwner = undefined
const transactionConfirmations = 4
const transactionType = undefined

// when
const result = checkIfTxIsExecution(threshold, preApprovingOwner, transactionConfirmations, transactionType)

// then
expect(result).toBe(false)
})
})
describe('checkIfTxIsCreation', () => {
it(`should return true if there are no confirmations for the transaction and the transaction is not spendingLimit`, () => {
// given
Expand Down
Loading