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

Commit

Permalink
Fix: conditions to display "Execute transaction" (#3257)
Browse files Browse the repository at this point in the history
* Build: create a hook for getting the recommended nonce

* Build: extract "can execute transaction" to a hook

* Build: remove unused return type

* Build: use "can execute" hook to retrieve condition to show the checkbox

* Refactor: use ComponentProps to get component types

* Fix: remove from tests logic extracted to hook

* refactor: refactor the useCanExecuteType to test the logic

* refactor: exclude the spending limit logic from useCanTxExecute

* refactor: use useCanTxExecute in other review modals

* fix: improve useCanTxExecute types and logic

* refactor: do not return isExecution from useEstimateTransactionGas

* fix: approve modal logic

* build: include PENDING_FAILED transactions in awaiting for execution

* build: flag execution to calculate tx gas

* build: include PENDING_FAILED tx status in showing execution tooltip and icon

* fix: adjust tests to reflect latest logic

* fix: add missing hook dependency

* fix: avoid race conditions when fetching from the backend

* fix: implement minor PR comments

* build: display the Execute checkbox if safe nonce is edited to be next

* fix: update tests to take manualSafeNonce

* fix: remove wrong check

* refactor: rename execution related variables

* fix: rename variable

* Obtain isOffChainSignature by calling a function instead of prop drilling

* Calculate tx gas even for off chain transactions

* fix TransactionFees rendering
  • Loading branch information
DiogoSoaress committed Jan 14, 2022
1 parent 88f93ab commit 42c2ee6
Show file tree
Hide file tree
Showing 27 changed files with 555 additions and 241 deletions.
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', () => {
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

0 comments on commit 42c2ee6

Please sign in to comment.