From a216e28101ff5f51a716f1580fec6cb7ccfb4463 Mon Sep 17 00:00:00 2001 From: Diogo Soares Date: Thu, 6 Jan 2022 12:06:23 +0100 Subject: [PATCH 01/27] Build: create a hook for getting the recommended nonce --- src/logic/hooks/useGetRecommendedNonce.tsx | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 src/logic/hooks/useGetRecommendedNonce.tsx diff --git a/src/logic/hooks/useGetRecommendedNonce.tsx b/src/logic/hooks/useGetRecommendedNonce.tsx new file mode 100644 index 0000000000..7396ed7789 --- /dev/null +++ b/src/logic/hooks/useGetRecommendedNonce.tsx @@ -0,0 +1,29 @@ +import { useEffect, useState } from 'react' +import { useSelector } from 'react-redux' +import { getRecommendedNonce } from '../safe/api/fetchSafeTxGasEstimation' +import { getLastTxNonce } from '../safe/store/selectors/gatewayTransactions' + +type UseGetRecommendedNonce = (safeAddress: string) => number | undefined + +const useGetRecommendedNonce: UseGetRecommendedNonce = (safeAddress) => { + const lastTxNonce = useSelector(getLastTxNonce) + const storeNextNonce = lastTxNonce ? lastTxNonce + 1 : undefined + + const [recommendedNonce, setRecommendedNonce] = useState(storeNextNonce) + + useEffect(() => { + const fetchRecommendedNonce = async () => { + try { + const recommendedNonce = await getRecommendedNonce(safeAddress) + setRecommendedNonce(recommendedNonce) + } catch (e) { + return + } + } + fetchRecommendedNonce() + }, [lastTxNonce, safeAddress]) + + return recommendedNonce +} + +export default useGetRecommendedNonce From 0266a1ef800e3c3ffaaecab0a08c2653f9353831 Mon Sep 17 00:00:00 2001 From: Diogo Soares Date: Thu, 6 Jan 2022 12:07:14 +0100 Subject: [PATCH 02/27] Build: extract "can execute transaction" to a hook --- src/logic/hooks/useCanTxExecute.tsx | 45 +++++++++++++++++++ src/logic/hooks/useEstimateTransactionGas.tsx | 27 +++-------- 2 files changed, 50 insertions(+), 22 deletions(-) create mode 100644 src/logic/hooks/useCanTxExecute.tsx diff --git a/src/logic/hooks/useCanTxExecute.tsx b/src/logic/hooks/useCanTxExecute.tsx new file mode 100644 index 0000000000..846ead4777 --- /dev/null +++ b/src/logic/hooks/useCanTxExecute.tsx @@ -0,0 +1,45 @@ +import { useSelector } from 'react-redux' +import { extractSafeAddress } from 'src/routes/routes' +import { sameString } from 'src/utils/strings' +import { currentSafe } from '../safe/store/selectors' +import { nextTransactions } from '../safe/store/selectors/gatewayTransactions' +import useGetRecommendedNonce from './useGetRecommendedNonce' + +type UseCanTxExecuteType = ( + threshold: number, + preApprovingOwner?: string, + txConfirmations?: number, + txType?: string, +) => boolean + +// Review default values +const useCanTxExecute: UseCanTxExecuteType = (threshold, preApprovingOwner = '', txConfirmations = 0, txType) => { + const nextQueuedTx = useSelector(nextTransactions) + const hasQueueNextTx = nextQueuedTx && Object.keys(nextQueuedTx).length > 0 + + const safeAddress = extractSafeAddress() + const recommendedNonce = useGetRecommendedNonce(safeAddress) + const { nonce: currentSafeNonce } = useSelector(currentSafe) + + // Do not display "Execute checkbox" if there's a tx in the next queue + if (hasQueueNextTx) return false + + // Single owner Safe and transaction is the next nonce + const isSingleOwnerNextTx = threshold === 1 && recommendedNonce === currentSafeNonce + + if ( + isSingleOwnerNextTx || + sameString(txType, 'spendingLimit') || + (txConfirmations !== undefined && txConfirmations >= threshold) + ) { + return true + } + + if (preApprovingOwner && txConfirmations) { + return txConfirmations + 1 === threshold + } + + return false +} + +export default useCanTxExecute diff --git a/src/logic/hooks/useEstimateTransactionGas.tsx b/src/logic/hooks/useEstimateTransactionGas.tsx index f305b52509..972c10e84b 100644 --- a/src/logic/hooks/useEstimateTransactionGas.tsx +++ b/src/logic/hooks/useEstimateTransactionGas.tsx @@ -19,6 +19,7 @@ import { Confirmation } from 'src/logic/safe/store/models/types/confirmation' import { checkIfOffChainSignatureIsPossible } from 'src/logic/safe/safeTxSigner' import { ZERO_ADDRESS } from 'src/logic/wallets/ethAddresses' import { sameString } from 'src/utils/strings' +import useCanTxExecute from './useCanTxExecute' export enum EstimationStatus { LOADING = 'LOADING', @@ -26,27 +27,6 @@ export enum EstimationStatus { SUCCESS = 'SUCCESS', } -export const checkIfTxIsExecution = ( - threshold: number, - preApprovingOwner?: string, - txConfirmations?: number, - txType?: string, -): boolean => { - if ( - threshold === 1 || - sameString(txType, 'spendingLimit') || - (txConfirmations !== undefined && txConfirmations >= threshold) - ) { - return true - } - - if (preApprovingOwner && txConfirmations) { - return txConfirmations + 1 === threshold - } - - return false -} - export const checkIfTxIsApproveAndExecution = ( threshold: number, txConfirmations: number, @@ -133,12 +113,14 @@ export const useEstimateTransactionGas = ({ const nativeCurrency = getNativeCurrency() const { address: safeAddress = '', threshold = 1, currentVersion: safeVersion = '' } = useSelector(currentSafe) ?? {} const { account: from, smartContractWallet, name: providerName } = useSelector(providerSelector) + + const isExecution = useCanTxExecute(threshold, preApprovingOwner, txConfirmations?.size, txType) + useEffect(() => { const estimateGas = async () => { if (!txData.length) { return } - const isExecution = checkIfTxIsExecution(Number(threshold), preApprovingOwner, txConfirmations?.size, txType) const isOffChainSignature = checkIfOffChainSignatureIsPossible(isExecution, smartContractWallet, safeVersion) const isCreation = checkIfTxIsCreation(txConfirmations?.size || 0, txType) @@ -257,6 +239,7 @@ export const useEstimateTransactionGas = ({ providerName, manualGasPrice, manualGasLimit, + isExecution, ]) return gasEstimation From b084b5500fdfe0e9cb2ad82bad6ca634b7d9981a Mon Sep 17 00:00:00 2001 From: Diogo Soares Date: Thu, 6 Jan 2022 12:08:54 +0100 Subject: [PATCH 03/27] Build: remove unused return type --- src/components/ExecuteCheckbox/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/ExecuteCheckbox/index.tsx b/src/components/ExecuteCheckbox/index.tsx index 127e832cec..3cdedee961 100644 --- a/src/components/ExecuteCheckbox/index.tsx +++ b/src/components/ExecuteCheckbox/index.tsx @@ -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): void => { onChange(e.target.checked) } From f0fb4cf4da59875d30a7262b6de764dd1a7ecc66 Mon Sep 17 00:00:00 2001 From: Diogo Soares Date: Thu, 6 Jan 2022 13:21:36 +0100 Subject: [PATCH 04/27] Build: use "can execute" hook to retrieve condition to show the checkbox --- .../SendModal/screens/ReviewSendFundsTx/index.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx b/src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx index e7d58cf2ca..376dff88d1 100644 --- a/src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx +++ b/src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx @@ -39,6 +39,8 @@ import { ModalHeader } from '../ModalHeader' import { extractSafeAddress } from 'src/routes/routes' import ExecuteCheckbox from 'src/components/ExecuteCheckbox' import { getNativeCurrencyAddress } from 'src/config/utils' +import { currentSafe } from 'src/logic/safe/store/selectors' +import useCanTxExecute from 'src/logic/hooks/useCanTxExecute' const useStyles = makeStyles(styles) @@ -108,7 +110,6 @@ const ReviewSendFundsTx = ({ onClose, onPrev, tx }: ReviewTxProps): React.ReactE gasLimit, gasEstimation, txEstimationExecutionStatus, - isExecution, isCreation, isOffChainSignature, } = useEstimateTransactionGas({ @@ -121,10 +122,13 @@ const ReviewSendFundsTx = ({ onClose, onPrev, tx }: ReviewTxProps): React.ReactE manualGasLimit, }) + const { threshold = 1 } = useSelector(currentSafe) + const canExecute = useCanTxExecute(threshold, tx.txType) + const [buttonStatus, setButtonStatus] = useEstimationStatus(txEstimationExecutionStatus) const isSpendingLimit = sameString(tx.txType, 'spendingLimit') const [executionApproved, setExecutionApproved] = useState(true) - const doExecute = isExecution && executionApproved + const doExecute = canExecute && executionApproved const submitTx = async (txParameters: TxParameters) => { setButtonStatus(ButtonStatus.LOADING) @@ -251,7 +255,7 @@ const ReviewSendFundsTx = ({ onClose, onPrev, tx }: ReviewTxProps): React.ReactE - {isExecution && !isSpendingLimit && } + {canExecute && !isSpendingLimit && } {/* Tx Parameters */} {/* FIXME TxParameters should be updated to be used with spending limits */} From ab24bd345d5d5b70b6747ddc7456cfd5c53c9d8b Mon Sep 17 00:00:00 2001 From: Diogo Soares Date: Thu, 6 Jan 2022 13:23:47 +0100 Subject: [PATCH 05/27] Refactor: use ComponentProps to get component types --- src/components/ReviewInfoText/index.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/ReviewInfoText/index.tsx b/src/components/ReviewInfoText/index.tsx index 2d7b57c1bc..7d69455609 100644 --- a/src/components/ReviewInfoText/index.tsx +++ b/src/components/ReviewInfoText/index.tsx @@ -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[0] & CustomReviewInfoTextProps +type ReviewInfoTextProps = ComponentProps & CustomReviewInfoTextProps const ReviewInfoTextWrapper = styled.div` background-color: ${({ theme }) => theme.colors.background}; From 240478a4f57b949e46472de4916a46e9d2403019 Mon Sep 17 00:00:00 2001 From: Diogo Soares Date: Thu, 6 Jan 2022 13:56:03 +0100 Subject: [PATCH 06/27] Fix: remove from tests logic extracted to hook --- .../useEstimateTransactionGas.test.ts | 74 +------------------ 1 file changed, 1 insertion(+), 73 deletions(-) diff --git a/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts b/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts index d7a613d7f3..5429e98497 100644 --- a/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts +++ b/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts @@ -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 From ffc1bfc60ea4eb094bb9226dfa354bdd78384543 Mon Sep 17 00:00:00 2001 From: Diogo Soares Date: Fri, 7 Jan 2022 18:41:27 +0100 Subject: [PATCH 07/27] refactor: refactor the useCanExecuteType to test the logic --- .../hooks/__tests__/useCanTxExecute.test.ts | 207 ++++++++++++++++++ src/logic/hooks/useCanTxExecute.tsx | 54 +++-- 2 files changed, 246 insertions(+), 15 deletions(-) create mode 100644 src/logic/hooks/__tests__/useCanTxExecute.test.ts diff --git a/src/logic/hooks/__tests__/useCanTxExecute.test.ts b/src/logic/hooks/__tests__/useCanTxExecute.test.ts new file mode 100644 index 0000000000..029d38d6b9 --- /dev/null +++ b/src/logic/hooks/__tests__/useCanTxExecute.test.ts @@ -0,0 +1,207 @@ +import { calculateCanTxExecute } from '../useCanTxExecute' + +describe('useCanTxExecute tests', () => { + describe('calculateCanTxExecute test', () => { + beforeEach(() => { + txType = '' + hasQueueNextTx = false + currentSafeNonce = 8 + recommendedNonce = 8 + }) + // to be overriden as necessary + let threshold + let preApprovingOwner + let txConfirmations + let txType + let currentSafeNonce + let recommendedNonce + let hasQueueNextTx + it(`should return false there are queued txs`, () => { + // given + hasQueueNextTx = true + + // when + const result = calculateCanTxExecute( + currentSafeNonce, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + recommendedNonce, + ) + + // then + expect(result).toBe(false) + }) + it(`should return true if the safe threshold is 1 and no queued txs`, () => { + // given + threshold = 1 + currentSafeNonce = 8 + recommendedNonce = 8 + hasQueueNextTx = false + + // when + const result = calculateCanTxExecute( + currentSafeNonce, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + recommendedNonce, + ) + + // then + expect(result).toBe(true) + }) + it(`should return false if the safe threshold is 1 and there are queued txs`, () => { + // given + threshold = 1 + currentSafeNonce = 8 + recommendedNonce = 8 + hasQueueNextTx = true + + // when + const result = calculateCanTxExecute( + currentSafeNonce, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + recommendedNonce, + ) + + // then + expect(result).toBe(false) + }) + + it(`should return false if the safe threshold is 1, there are no queued txs but recommendedNonce does not match safeNonce`, () => { + // given + threshold = 1 + currentSafeNonce = 8 + recommendedNonce = 10 + + // when + const result = calculateCanTxExecute( + currentSafeNonce, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + recommendedNonce, + ) + + // then + expect(result).toBe(false) + }) + it(`should return true if the safe threshold is reached for the transaction`, () => { + // given + threshold = 3 + txConfirmations = 3 + + console.log( + currentSafeNonce, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + recommendedNonce, + ) + // when + const result = calculateCanTxExecute( + currentSafeNonce, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + recommendedNonce, + ) + + // then + expect(result).toBe(true) + }) + it(`should return true if the transaction is spendingLimit`, () => { + // given + txType = 'spendingLimit' + + // when + const result = calculateCanTxExecute( + currentSafeNonce, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + recommendedNonce, + ) + + // then + expect(result).toBe(true) + }) + it.skip(`should return false if the number of confirmations meets the threshold but txNonce > safeNonce`, () => { + // given + threshold = 5 + txConfirmations = 5 + currentSafeNonce = 8 + recommendedNonce = 10 + + // when + const result = calculateCanTxExecute( + currentSafeNonce, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + recommendedNonce, + ) + + // then + expect(result).toBe(false) + }) + 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, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + 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, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + recommendedNonce, + ) + + // then + expect(result).toBe(true) + }) + }) +}) diff --git a/src/logic/hooks/useCanTxExecute.tsx b/src/logic/hooks/useCanTxExecute.tsx index 846ead4777..ef12136b15 100644 --- a/src/logic/hooks/useCanTxExecute.tsx +++ b/src/logic/hooks/useCanTxExecute.tsx @@ -1,3 +1,4 @@ +import { useEffect, useState } from 'react' import { useSelector } from 'react-redux' import { extractSafeAddress } from 'src/routes/routes' import { sameString } from 'src/utils/strings' @@ -12,29 +13,26 @@ type UseCanTxExecuteType = ( txType?: string, ) => boolean -// Review default values -const useCanTxExecute: UseCanTxExecuteType = (threshold, preApprovingOwner = '', txConfirmations = 0, txType) => { - const nextQueuedTx = useSelector(nextTransactions) - const hasQueueNextTx = nextQueuedTx && Object.keys(nextQueuedTx).length > 0 - - const safeAddress = extractSafeAddress() - const recommendedNonce = useGetRecommendedNonce(safeAddress) - const { nonce: currentSafeNonce } = useSelector(currentSafe) - +export const calculateCanTxExecute = ( + currentSafeNonce: number, + hasQueueNextTx = false, + preApprovingOwner = '', + threshold: number, + txConfirmations = 0, + txType = '', + recommendedNonce?: number, +): boolean => { // Do not display "Execute checkbox" if there's a tx in the next queue if (hasQueueNextTx) return false - // Single owner Safe and transaction is the next nonce + // Single owner Safe AND transaction is the next nonce const isSingleOwnerNextTx = threshold === 1 && recommendedNonce === currentSafeNonce - if ( - isSingleOwnerNextTx || - sameString(txType, 'spendingLimit') || - (txConfirmations !== undefined && txConfirmations >= threshold) - ) { + if (isSingleOwnerNextTx || sameString(txType, 'spendingLimit') || txConfirmations >= threshold) { return true } + // When having a preApprovingOwner it is needed one less confirmation to execute the tx if (preApprovingOwner && txConfirmations) { return txConfirmations + 1 === threshold } @@ -42,4 +40,30 @@ const useCanTxExecute: UseCanTxExecuteType = (threshold, preApprovingOwner = '', return false } +// Review default values +const useCanTxExecute: UseCanTxExecuteType = (threshold, preApprovingOwner = '', txConfirmations = 0, txType) => { + const [canTxExecute, setCanTxExecute] = useState(false) + const nextQueuedTx = useSelector(nextTransactions) + const hasQueueNextTx = nextQueuedTx && Object.keys(nextQueuedTx).length > 0 + + const safeAddress = extractSafeAddress() + const recommendedNonce = useGetRecommendedNonce(safeAddress) // to be changed. should be txNonce + const { nonce: currentSafeNonce } = useSelector(currentSafe) + + useEffect(() => { + const result = calculateCanTxExecute( + currentSafeNonce, + hasQueueNextTx, + preApprovingOwner, + threshold, + txConfirmations, + txType, + recommendedNonce, + ) + setCanTxExecute(result) + }, [currentSafeNonce, hasQueueNextTx, preApprovingOwner, recommendedNonce, threshold, txConfirmations, txType]) + + return canTxExecute +} + export default useCanTxExecute From d56093b99a15af4f60a903825fa00989a5e07f9a Mon Sep 17 00:00:00 2001 From: Diogo Soares Date: Sat, 8 Jan 2022 15:58:20 +0100 Subject: [PATCH 08/27] refactor: exclude the spending limit logic from useCanTxExecute --- .../hooks/__tests__/useCanTxExecute.test.ts | 21 +--------------- src/logic/hooks/useCanTxExecute.tsx | 25 ++++++++----------- src/logic/hooks/useEstimateTransactionGas.tsx | 2 +- .../screens/ReviewSendFundsTx/index.tsx | 10 +++----- 4 files changed, 16 insertions(+), 42 deletions(-) diff --git a/src/logic/hooks/__tests__/useCanTxExecute.test.ts b/src/logic/hooks/__tests__/useCanTxExecute.test.ts index 029d38d6b9..a7e3e11363 100644 --- a/src/logic/hooks/__tests__/useCanTxExecute.test.ts +++ b/src/logic/hooks/__tests__/useCanTxExecute.test.ts @@ -3,7 +3,6 @@ import { calculateCanTxExecute } from '../useCanTxExecute' describe('useCanTxExecute tests', () => { describe('calculateCanTxExecute test', () => { beforeEach(() => { - txType = '' hasQueueNextTx = false currentSafeNonce = 8 recommendedNonce = 8 @@ -27,7 +26,6 @@ describe('useCanTxExecute tests', () => { preApprovingOwner, threshold, txConfirmations, - txType, recommendedNonce, ) @@ -48,7 +46,6 @@ describe('useCanTxExecute tests', () => { preApprovingOwner, threshold, txConfirmations, - txType, recommendedNonce, ) @@ -69,7 +66,6 @@ describe('useCanTxExecute tests', () => { preApprovingOwner, threshold, txConfirmations, - txType, recommendedNonce, ) @@ -90,7 +86,6 @@ describe('useCanTxExecute tests', () => { preApprovingOwner, threshold, txConfirmations, - txType, recommendedNonce, ) @@ -102,15 +97,6 @@ describe('useCanTxExecute tests', () => { threshold = 3 txConfirmations = 3 - console.log( - currentSafeNonce, - hasQueueNextTx, - preApprovingOwner, - threshold, - txConfirmations, - txType, - recommendedNonce, - ) // when const result = calculateCanTxExecute( currentSafeNonce, @@ -118,14 +104,13 @@ describe('useCanTxExecute tests', () => { preApprovingOwner, threshold, txConfirmations, - txType, recommendedNonce, ) // then expect(result).toBe(true) }) - it(`should return true if the transaction is spendingLimit`, () => { + it.skip(`should return true if the transaction is spendingLimit`, () => { // given txType = 'spendingLimit' @@ -136,7 +121,6 @@ describe('useCanTxExecute tests', () => { preApprovingOwner, threshold, txConfirmations, - txType, recommendedNonce, ) @@ -157,7 +141,6 @@ describe('useCanTxExecute tests', () => { preApprovingOwner, threshold, txConfirmations, - txType, recommendedNonce, ) @@ -176,7 +159,6 @@ describe('useCanTxExecute tests', () => { preApprovingOwner, threshold, txConfirmations, - txType, recommendedNonce, ) @@ -196,7 +178,6 @@ describe('useCanTxExecute tests', () => { preApprovingOwner, threshold, txConfirmations, - txType, recommendedNonce, ) diff --git a/src/logic/hooks/useCanTxExecute.tsx b/src/logic/hooks/useCanTxExecute.tsx index ef12136b15..4c51ec7817 100644 --- a/src/logic/hooks/useCanTxExecute.tsx +++ b/src/logic/hooks/useCanTxExecute.tsx @@ -1,17 +1,11 @@ import { useEffect, useState } from 'react' import { useSelector } from 'react-redux' import { extractSafeAddress } from 'src/routes/routes' -import { sameString } from 'src/utils/strings' import { currentSafe } from '../safe/store/selectors' import { nextTransactions } from '../safe/store/selectors/gatewayTransactions' import useGetRecommendedNonce from './useGetRecommendedNonce' -type UseCanTxExecuteType = ( - threshold: number, - preApprovingOwner?: string, - txConfirmations?: number, - txType?: string, -) => boolean +type UseCanTxExecuteType = (preApprovingOwner?: string, txConfirmations?: number) => boolean export const calculateCanTxExecute = ( currentSafeNonce: number, @@ -19,16 +13,19 @@ export const calculateCanTxExecute = ( preApprovingOwner = '', threshold: number, txConfirmations = 0, - txType = '', recommendedNonce?: number, ): boolean => { // Do not display "Execute checkbox" if there's a tx in the next queue if (hasQueueNextTx) return false - // Single owner Safe AND transaction is the next nonce - const isSingleOwnerNextTx = threshold === 1 && recommendedNonce === currentSafeNonce + const isNextTx = recommendedNonce === currentSafeNonce + // Single owner + if (threshold === 1) { + // transaction is the next nonce + return isNextTx ? true : false + } - if (isSingleOwnerNextTx || sameString(txType, 'spendingLimit') || txConfirmations >= threshold) { + if (txConfirmations >= threshold) { return true } @@ -41,10 +38,11 @@ export const calculateCanTxExecute = ( } // Review default values -const useCanTxExecute: UseCanTxExecuteType = (threshold, preApprovingOwner = '', txConfirmations = 0, txType) => { +const useCanTxExecute: UseCanTxExecuteType = (preApprovingOwner = '', txConfirmations = 0) => { const [canTxExecute, setCanTxExecute] = useState(false) const nextQueuedTx = useSelector(nextTransactions) const hasQueueNextTx = nextQueuedTx && Object.keys(nextQueuedTx).length > 0 + const { threshold = 1 } = useSelector(currentSafe) const safeAddress = extractSafeAddress() const recommendedNonce = useGetRecommendedNonce(safeAddress) // to be changed. should be txNonce @@ -57,11 +55,10 @@ const useCanTxExecute: UseCanTxExecuteType = (threshold, preApprovingOwner = '', preApprovingOwner, threshold, txConfirmations, - txType, recommendedNonce, ) setCanTxExecute(result) - }, [currentSafeNonce, hasQueueNextTx, preApprovingOwner, recommendedNonce, threshold, txConfirmations, txType]) + }, [currentSafeNonce, hasQueueNextTx, preApprovingOwner, recommendedNonce, threshold, txConfirmations]) return canTxExecute } diff --git a/src/logic/hooks/useEstimateTransactionGas.tsx b/src/logic/hooks/useEstimateTransactionGas.tsx index 972c10e84b..4b330fde84 100644 --- a/src/logic/hooks/useEstimateTransactionGas.tsx +++ b/src/logic/hooks/useEstimateTransactionGas.tsx @@ -114,7 +114,7 @@ export const useEstimateTransactionGas = ({ const { address: safeAddress = '', threshold = 1, currentVersion: safeVersion = '' } = useSelector(currentSafe) ?? {} const { account: from, smartContractWallet, name: providerName } = useSelector(providerSelector) - const isExecution = useCanTxExecute(threshold, preApprovingOwner, txConfirmations?.size, txType) + const isExecution = useCanTxExecute(preApprovingOwner, txConfirmations?.size) useEffect(() => { const estimateGas = async () => { diff --git a/src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx b/src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx index 376dff88d1..0c28545af9 100644 --- a/src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx +++ b/src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx @@ -39,7 +39,6 @@ import { ModalHeader } from '../ModalHeader' import { extractSafeAddress } from 'src/routes/routes' import ExecuteCheckbox from 'src/components/ExecuteCheckbox' import { getNativeCurrencyAddress } from 'src/config/utils' -import { currentSafe } from 'src/logic/safe/store/selectors' import useCanTxExecute from 'src/logic/hooks/useCanTxExecute' const useStyles = makeStyles(styles) @@ -121,14 +120,11 @@ const ReviewSendFundsTx = ({ onClose, onPrev, tx }: ReviewTxProps): React.ReactE manualGasPrice, manualGasLimit, }) - - const { threshold = 1 } = useSelector(currentSafe) - const canExecute = useCanTxExecute(threshold, tx.txType) - const [buttonStatus, setButtonStatus] = useEstimationStatus(txEstimationExecutionStatus) const isSpendingLimit = sameString(tx.txType, 'spendingLimit') const [executionApproved, setExecutionApproved] = useState(true) - const doExecute = canExecute && executionApproved + const canTxExecute = useCanTxExecute() + const doExecute = canTxExecute && executionApproved const submitTx = async (txParameters: TxParameters) => { setButtonStatus(ButtonStatus.LOADING) @@ -255,7 +251,7 @@ const ReviewSendFundsTx = ({ onClose, onPrev, tx }: ReviewTxProps): React.ReactE - {canExecute && !isSpendingLimit && } + {canTxExecute && !isSpendingLimit && } {/* Tx Parameters */} {/* FIXME TxParameters should be updated to be used with spending limits */} From 57612cfd1b35e8974c028a33054c971ea1b94fe6 Mon Sep 17 00:00:00 2001 From: Diogo Soares Date: Sat, 8 Jan 2022 15:59:37 +0100 Subject: [PATCH 09/27] refactor: use useCanTxExecute in other review modals --- .../Apps/components/ConfirmTxModal/ReviewConfirm.tsx | 7 ++++--- .../screens/ContractInteraction/Review/index.tsx | 7 ++++--- .../screens/ContractInteraction/ReviewCustomTx/index.tsx | 7 ++++--- .../SendModal/screens/ReviewCollectible/index.tsx | 8 ++++---- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx b/src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx index 1d53f50c2e..6eba62a91a 100644 --- a/src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx +++ b/src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx @@ -31,6 +31,7 @@ import { ReviewInfoText } from 'src/components/ReviewInfoText' import { ConfirmTxModalProps, DecodedTxDetail } from '.' import { grantedSelector } from 'src/routes/safe/container/selector' import ExecuteCheckbox from 'src/components/ExecuteCheckbox' +import useCanTxExecute from 'src/logic/hooks/useCanTxExecute' const Container = styled.div` max-width: 480px; @@ -109,7 +110,6 @@ export const ReviewConfirm = ({ gasEstimation, isOffChainSignature, isCreation, - isExecution, gasCostFormatted, txEstimationExecutionStatus, } = useEstimateTransactionGas({ @@ -124,7 +124,8 @@ export const ReviewConfirm = ({ const [buttonStatus, setButtonStatus] = useEstimationStatus(txEstimationExecutionStatus) const [executionApproved, setExecutionApproved] = useState(true) - const doExecute = isExecution && executionApproved + const canTxExecute = useCanTxExecute() + const doExecute = canTxExecute && executionApproved // Decode tx data. useEffect(() => { @@ -225,7 +226,7 @@ export const ReviewConfirm = ({ {!isMultiSend && } - {isExecution && } + {canTxExecute && } {/* Tx Parameters */} { @@ -226,7 +227,7 @@ const ContractInteractionReview = ({ onClose, onPrev, tx }: Props): React.ReactE - {isExecution && } + {canTxExecute && } {/* Tx Parameters */} { gasPriceFormatted, gasCostFormatted, txEstimationExecutionStatus, - isExecution, isCreation, isOffChainSignature, } = useEstimateTransactionGas({ @@ -66,7 +66,8 @@ const ReviewCustomTx = ({ onClose, onPrev, tx }: Props): ReactElement => { txAmount: tx.value ? toTokenUnit(tx.value, nativeCurrency.decimals) : '0', }) - const doExecute = isExecution && executionApproved + const canTxExecute = useCanTxExecute() + const doExecute = canTxExecute && executionApproved const [buttonStatus] = useEstimationStatus(txEstimationExecutionStatus) const submitTx = (txParameters: TxParameters) => { @@ -151,7 +152,7 @@ const ReviewCustomTx = ({ onClose, onPrev, tx }: Props): ReactElement => { - {isExecution && } + {canTxExecute && } {/* Tx Parameters */} { @@ -194,7 +194,7 @@ const ReviewCollectible = ({ onClose, onPrev, tx }: Props): React.ReactElement = )} - {isExecution && } + {canTxExecute && } {/* Tx Parameters */} Date: Sat, 8 Jan 2022 20:52:51 +0100 Subject: [PATCH 10/27] fix: improve useCanTxExecute types and logic --- src/logic/hooks/useCanTxExecute.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/logic/hooks/useCanTxExecute.tsx b/src/logic/hooks/useCanTxExecute.tsx index 4c51ec7817..c9a2a941ee 100644 --- a/src/logic/hooks/useCanTxExecute.tsx +++ b/src/logic/hooks/useCanTxExecute.tsx @@ -5,14 +5,12 @@ import { currentSafe } from '../safe/store/selectors' import { nextTransactions } from '../safe/store/selectors/gatewayTransactions' import useGetRecommendedNonce from './useGetRecommendedNonce' -type UseCanTxExecuteType = (preApprovingOwner?: string, txConfirmations?: number) => boolean - export const calculateCanTxExecute = ( currentSafeNonce: number, hasQueueNextTx = false, - preApprovingOwner = '', + preApprovingOwner: string, threshold: number, - txConfirmations = 0, + txConfirmations: number, recommendedNonce?: number, ): boolean => { // Do not display "Execute checkbox" if there's a tx in the next queue @@ -22,7 +20,7 @@ export const calculateCanTxExecute = ( // Single owner if (threshold === 1) { // transaction is the next nonce - return isNextTx ? true : false + return isNextTx } if (txConfirmations >= threshold) { @@ -37,12 +35,14 @@ export const calculateCanTxExecute = ( return false } +type UseCanTxExecuteType = (preApprovingOwner?: string, txConfirmations?: number) => boolean + // Review default values const useCanTxExecute: UseCanTxExecuteType = (preApprovingOwner = '', txConfirmations = 0) => { const [canTxExecute, setCanTxExecute] = useState(false) const nextQueuedTx = useSelector(nextTransactions) const hasQueueNextTx = nextQueuedTx && Object.keys(nextQueuedTx).length > 0 - const { threshold = 1 } = useSelector(currentSafe) + const { threshold } = useSelector(currentSafe) const safeAddress = extractSafeAddress() const recommendedNonce = useGetRecommendedNonce(safeAddress) // to be changed. should be txNonce From 1fa2d510ed9cbf586a2e36d6c264a7e76bb81fed Mon Sep 17 00:00:00 2001 From: Diogo Soares Date: Sat, 8 Jan 2022 21:44:28 +0100 Subject: [PATCH 11/27] refactor: do not return isExecution from useEstimateTransactionGas --- src/logic/hooks/useEstimateTransactionGas.tsx | 8 +------- .../Apps/components/ConfirmTxModal/ReviewConfirm.tsx | 8 ++++---- .../Apps/components/SignMessageModal/ReviewMessage.tsx | 3 ++- .../components/Settings/Advanced/RemoveGuardModal.tsx | 3 ++- .../components/Settings/Advanced/RemoveModuleModal.tsx | 4 ++-- .../ManageOwners/AddOwnerModal/screens/Review/index.tsx | 3 ++- .../RemoveOwnerModal/screens/Review/index.tsx | 3 ++- .../ReplaceOwnerModal/screens/Review/index.tsx | 3 ++- .../Settings/SpendingLimit/NewLimitModal/Review.tsx | 3 ++- .../Settings/SpendingLimit/RemoveLimitModal.tsx | 3 ++- .../Settings/ThresholdSettings/ChangeThreshold/index.tsx | 3 ++- .../safe/components/Settings/UpdateSafeModal/index.tsx | 3 ++- .../Transactions/TxList/modals/ApproveTxModal.tsx | 3 ++- .../Transactions/TxList/modals/RejectTxModal.tsx | 3 ++- 14 files changed, 29 insertions(+), 24 deletions(-) diff --git a/src/logic/hooks/useEstimateTransactionGas.tsx b/src/logic/hooks/useEstimateTransactionGas.tsx index 4b330fde84..96bd1770c4 100644 --- a/src/logic/hooks/useEstimateTransactionGas.tsx +++ b/src/logic/hooks/useEstimateTransactionGas.tsx @@ -68,7 +68,6 @@ export type TransactionGasEstimationResult = { gasPrice: string // Current price of gas unit gasPriceFormatted: string // Current gas price formatted gasLimit: string // Minimum gas requited to execute the Tx - isExecution: boolean // Returns true if the user will execute the tx or false if it just signs it isCreation: boolean // Returns true if the transaction is a creation transaction isOffChainSignature: boolean // Returns true if offChainSignature is available } @@ -77,7 +76,6 @@ const getDefaultGasEstimation = ( txEstimationExecutionStatus: EstimationStatus, gasPrice: string, gasPriceFormatted: string, - isExecution = false, isCreation = false, isOffChainSignature = false, ): TransactionGasEstimationResult => { @@ -89,7 +87,6 @@ const getDefaultGasEstimation = ( gasPrice, gasPriceFormatted, gasLimit: '0', - isExecution, isCreation, isOffChainSignature, } @@ -125,9 +122,7 @@ export const useEstimateTransactionGas = ({ const isCreation = checkIfTxIsCreation(txConfirmations?.size || 0, txType) if (isOffChainSignature && !isCreation) { - setGasEstimation( - getDefaultGasEstimation(EstimationStatus.SUCCESS, '1', '1', isExecution, isCreation, isOffChainSignature), - ) + setGasEstimation(getDefaultGasEstimation(EstimationStatus.SUCCESS, '1', '1', isCreation, isOffChainSignature)) return } const approvalAndExecution = checkIfTxIsApproveAndExecution( @@ -209,7 +204,6 @@ export const useEstimateTransactionGas = ({ gasPrice, gasPriceFormatted, gasLimit, - isExecution, isCreation, isOffChainSignature, }) diff --git a/src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx b/src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx index 6eba62a91a..8751843d0f 100644 --- a/src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx +++ b/src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx @@ -125,7 +125,7 @@ export const ReviewConfirm = ({ const [buttonStatus, setButtonStatus] = useEstimationStatus(txEstimationExecutionStatus) const [executionApproved, setExecutionApproved] = useState(true) const canTxExecute = useCanTxExecute() - const doExecute = canTxExecute && executionApproved + const isExecution = canTxExecute && executionApproved // Decode tx data. useEffect(() => { @@ -195,7 +195,7 @@ export const ReviewConfirm = ({ safeTxGas={Math.max(parseInt(gasEstimation), params?.safeTxGas || 0).toString()} closeEditModalCallback={closeEditModalCallback} isOffChainSignature={isOffChainSignature} - isExecution={doExecute} + isExecution={isExecution} > {(txParameters, toggleEditMode) => (