From 4b4ba5732100b1022d80474578117c2cd7e6f2b8 Mon Sep 17 00:00:00 2001 From: Agustin Pane Date: Fri, 22 Jan 2021 15:50:33 -0300 Subject: [PATCH 1/5] Fix checkIfTxIsExecution method implementation --- src/logic/hooks/useEstimateTransactionGas.tsx | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/logic/hooks/useEstimateTransactionGas.tsx b/src/logic/hooks/useEstimateTransactionGas.tsx index f3c33747ed..499081390b 100644 --- a/src/logic/hooks/useEstimateTransactionGas.tsx +++ b/src/logic/hooks/useEstimateTransactionGas.tsx @@ -30,18 +30,26 @@ export enum EstimationStatus { SUCCESS = 'SUCCESS', } -const checkIfTxIsExecution = ( +export const checkIfTxIsExecution = ( threshold: number, preApprovingOwner?: string, txConfirmations?: number, txType?: string, -): boolean => - txConfirmations === threshold || !!preApprovingOwner || threshold === 1 || sameString(txType, 'spendingLimit') +): boolean => { + if (threshold === 1) return true + if (sameString(txType, 'spendingLimit')) return true + if (txConfirmations === threshold) return true + if (preApprovingOwner && txConfirmations) { + return txConfirmations + 1 === threshold + } + + return false +} -const checkIfTxIsApproveAndExecution = (threshold: number, txConfirmations: number, txType?: string): boolean => +export const checkIfTxIsApproveAndExecution = (threshold: number, txConfirmations: number, txType?: string): boolean => txConfirmations + 1 === threshold || sameString(txType, 'spendingLimit') -const checkIfTxIsCreation = (txConfirmations: number, txType?: string): boolean => +export const checkIfTxIsCreation = (txConfirmations: number, txType?: string): boolean => txConfirmations === 0 && !sameString(txType, 'spendingLimit') type TransactionEstimationProps = { @@ -173,7 +181,7 @@ export const useEstimateTransactionGas = ({ return null } - const isExecution = checkIfTxIsExecution(Number(threshold), preApprovingOwner, txConfirmations?.size, txType) + const isExecution = checkIfTxIsExecution(Number(threshold), undefined, txConfirmations?.size, txType) const isCreation = checkIfTxIsCreation(txConfirmations?.size || 0, txType) const approvalAndExecution = checkIfTxIsApproveAndExecution(Number(threshold), txConfirmations?.size || 0, txType) From 7280286cf8dcc31ad3702d0bf79cd83350bdf8e7 Mon Sep 17 00:00:00 2001 From: Agustin Pane Date: Fri, 22 Jan 2021 15:50:56 -0300 Subject: [PATCH 2/5] Add tests for checkIfTxIsExecution/checkIfTxIsCreation/checkIfTxIsApproveAndExecution/ --- .../useEstimateTransactionGas.test.ts | 161 ++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts diff --git a/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts b/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts new file mode 100644 index 0000000000..344bc780d0 --- /dev/null +++ b/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts @@ -0,0 +1,161 @@ +import { + checkIfTxIsApproveAndExecution, + checkIfTxIsCreation, + checkIfTxIsExecution, +} 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 + const transactionConfirmations = 0 + const transactionType = '' + + + // when + const result = checkIfTxIsCreation(transactionConfirmations, transactionType) + + // then + expect(result).toBe(true) + }) + it(`should return false if there are no confirmations for the transaction and the transaction is spendingLimit`, () => { + // given + const transactionConfirmations = 0 + const transactionType = 'spendingLimit' + + + // when + const result = checkIfTxIsCreation(transactionConfirmations, transactionType) + + // then + expect(result).toBe(false) + }) + it(`should return false if there are confirmations for the transaction`, () => { + // given + const transactionConfirmations = 2 + const transactionType = '' + + + // when + const result = checkIfTxIsCreation(transactionConfirmations, transactionType) + + // then + expect(result).toBe(false) + }) +}) + +describe('checkIfTxIsApproveAndExecution', () => { + it(`should return true if there is only one confirmation left to reach the safe threshold`, () => { + // given + const transactionConfirmations = 2 + const safeThreshold = 3 + const transactionType = '' + + + // when + const result = checkIfTxIsApproveAndExecution(safeThreshold, transactionConfirmations, transactionType) + + // then + expect(result).toBe(true) + }) + it(`should return true if the transaction is spendingLimit`, () => { + // given + const transactionConfirmations = 0 + const transactionType = 'spendingLimit' + const safeThreshold = 3 + + + // when + const result = checkIfTxIsApproveAndExecution(safeThreshold, transactionConfirmations, transactionType) + + // then + expect(result).toBe(true) + }) + it(`should return false if the are missing more than one confirmations to reach the safe threshold and the transaction is not spendingLimit`, () => { + // given + const transactionConfirmations = 0 + const transactionType = '' + const safeThreshold = 3 + + + // when + const result = checkIfTxIsApproveAndExecution(safeThreshold, transactionConfirmations, transactionType) + + // then + expect(result).toBe(false) + }) +}) + From 2f451156978ff20a55eae194033cead87eb26898 Mon Sep 17 00:00:00 2001 From: Agustin Pane Date: Mon, 25 Jan 2021 09:34:23 -0300 Subject: [PATCH 3/5] ESLINT issues fix --- .../__tests__/useEstimateTransactionGas.test.ts | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts b/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts index 344bc780d0..99b83f0322 100644 --- a/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts +++ b/src/logic/hooks/__tests__/useEstimateTransactionGas.test.ts @@ -4,9 +4,8 @@ import { checkIfTxIsExecution, } from 'src/logic/hooks/useEstimateTransactionGas' - describe('checkIfTxIsExecution', () => { - const mockedEthAccount = "0x29B1b813b6e84654Ca698ef5d7808E154364900B" + const mockedEthAccount = '0x29B1b813b6e84654Ca698ef5d7808E154364900B' it(`should return true if the safe threshold is 1`, () => { // given const threshold = 1 @@ -14,7 +13,6 @@ describe('checkIfTxIsExecution', () => { const transactionConfirmations = 0 const transactionType = '' - // when const result = checkIfTxIsExecution(threshold, preApprovingOwner, transactionConfirmations, transactionType) @@ -28,7 +26,6 @@ describe('checkIfTxIsExecution', () => { const transactionConfirmations = 3 const transactionType = '' - // when const result = checkIfTxIsExecution(threshold, preApprovingOwner, transactionConfirmations, transactionType) @@ -42,7 +39,6 @@ describe('checkIfTxIsExecution', () => { const transactionConfirmations = 0 const transactionType = 'spendingLimit' - // when const result = checkIfTxIsExecution(threshold, preApprovingOwner, transactionConfirmations, transactionType) @@ -56,7 +52,6 @@ describe('checkIfTxIsExecution', () => { const transactionConfirmations = 4 const transactionType = undefined - // when const result = checkIfTxIsExecution(threshold, preApprovingOwner, transactionConfirmations, transactionType) @@ -70,7 +65,6 @@ describe('checkIfTxIsExecution', () => { const transactionConfirmations = 4 const transactionType = undefined - // when const result = checkIfTxIsExecution(threshold, preApprovingOwner, transactionConfirmations, transactionType) @@ -84,7 +78,6 @@ describe('checkIfTxIsCreation', () => { const transactionConfirmations = 0 const transactionType = '' - // when const result = checkIfTxIsCreation(transactionConfirmations, transactionType) @@ -96,7 +89,6 @@ describe('checkIfTxIsCreation', () => { const transactionConfirmations = 0 const transactionType = 'spendingLimit' - // when const result = checkIfTxIsCreation(transactionConfirmations, transactionType) @@ -108,7 +100,6 @@ describe('checkIfTxIsCreation', () => { const transactionConfirmations = 2 const transactionType = '' - // when const result = checkIfTxIsCreation(transactionConfirmations, transactionType) @@ -124,7 +115,6 @@ describe('checkIfTxIsApproveAndExecution', () => { const safeThreshold = 3 const transactionType = '' - // when const result = checkIfTxIsApproveAndExecution(safeThreshold, transactionConfirmations, transactionType) @@ -137,7 +127,6 @@ describe('checkIfTxIsApproveAndExecution', () => { const transactionType = 'spendingLimit' const safeThreshold = 3 - // when const result = checkIfTxIsApproveAndExecution(safeThreshold, transactionConfirmations, transactionType) @@ -150,7 +139,6 @@ describe('checkIfTxIsApproveAndExecution', () => { const transactionType = '' const safeThreshold = 3 - // when const result = checkIfTxIsApproveAndExecution(safeThreshold, transactionConfirmations, transactionType) @@ -158,4 +146,3 @@ describe('checkIfTxIsApproveAndExecution', () => { expect(result).toBe(false) }) }) - From 842b4aadac57e9a26756846b16fb5605b5c7085c Mon Sep 17 00:00:00 2001 From: Agustin Pane Date: Mon, 25 Jan 2021 10:53:33 -0300 Subject: [PATCH 4/5] Fix missing param --- src/logic/hooks/useEstimateTransactionGas.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/logic/hooks/useEstimateTransactionGas.tsx b/src/logic/hooks/useEstimateTransactionGas.tsx index 499081390b..820be0fec5 100644 --- a/src/logic/hooks/useEstimateTransactionGas.tsx +++ b/src/logic/hooks/useEstimateTransactionGas.tsx @@ -181,7 +181,7 @@ export const useEstimateTransactionGas = ({ return null } - const isExecution = checkIfTxIsExecution(Number(threshold), undefined, txConfirmations?.size, txType) + const isExecution = checkIfTxIsExecution(Number(threshold), preApprovingOwner, txConfirmations?.size, txType) const isCreation = checkIfTxIsCreation(txConfirmations?.size || 0, txType) const approvalAndExecution = checkIfTxIsApproveAndExecution(Number(threshold), txConfirmations?.size || 0, txType) From d6e29b0ab6cd4c56d029be21d945be79da2be775 Mon Sep 17 00:00:00 2001 From: Daniel Sanchez Date: Tue, 26 Jan 2021 09:30:37 +0100 Subject: [PATCH 5/5] Minimiza number of ifs with same result --- src/logic/hooks/useEstimateTransactionGas.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/logic/hooks/useEstimateTransactionGas.tsx b/src/logic/hooks/useEstimateTransactionGas.tsx index 820be0fec5..fa5cc94ef2 100644 --- a/src/logic/hooks/useEstimateTransactionGas.tsx +++ b/src/logic/hooks/useEstimateTransactionGas.tsx @@ -36,9 +36,10 @@ export const checkIfTxIsExecution = ( txConfirmations?: number, txType?: string, ): boolean => { - if (threshold === 1) return true - if (sameString(txType, 'spendingLimit')) return true - if (txConfirmations === threshold) return true + if (threshold === 1 || sameString(txType, 'spendingLimit') || txConfirmations === threshold) { + return true + } + if (preApprovingOwner && txConfirmations) { return txConfirmations + 1 === threshold }