Fix: conditions to display "Execute transaction" #3257

DiogoSoaress · 2022-01-06T13:08:30Z

What it solves

Resolves #2942

How this PR fixes it

Extracted to a new hook useCanTxExecute the logic to check if a tx can execute.
Display the "Execute transaction" checkbox based on with this logic.

The variables were renamed as:

canTxExecute: for hook return
shouldExecute: for checkbox state
willExecute: for instances where the above two are combined

How to test it

In a single owner Safe, "Execute transaction" checkbox shall display when creating a transaction and

There is no tx in the next queue and the tx nonce is equal to the safe nonce

In a multi owner Safe, "Execute transaction" checkbox shall display when:

Approving transaction and the number of confirmations meets the Safe threshold

Screenshots

github-actions · 2022-01-06T13:08:49Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2022-01-06T13:12:29Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	2	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2022-01-06T13:16:02Z

Deployment links

🟠 Safe Rinkeby	⚪ Safe Mainnet	🟣 Safe Polygon	🟡 Safe BSC	⚫ Safe Arbitrum	🟢 Safe xDai

coveralls · 2022-01-06T13:17:15Z

Pull Request Test Coverage Report for Build 1693532428

47 of 141 (33.33%) changed or added relevant lines in 22 files are covered.
6 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.05%) to 32.493%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/routes/safe/components/Settings/UpdateSafeModal/index.tsx	0	1	0.0%
src/routes/safe/components/Transactions/TxList/modals/RejectTxModal.tsx	0	1	0.0%
src/logic/hooks/useGetRecommendedNonce.tsx	13	16	81.25%
src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/ReviewCustomTx/index.tsx	0	3	0.0%
src/routes/safe/components/Transactions/TxList/modals/ApproveTxModal.tsx	0	3	0.0%
src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx	4	8	50.0%
src/routes/safe/components/Apps/components/SignMessageModal/ReviewMessage.tsx	2	6	33.33%
src/routes/safe/components/Settings/Advanced/RemoveGuardModal.tsx	2	6	33.33%
src/logic/hooks/useEstimateTransactionGas.tsx	0	5	0.0%
src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx	0	6	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/logic/hooks/useEstimateTransactionGas.tsx	1	15.69%
src/logic/safe/transactions/gas.ts	1	17.31%
src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/ReviewCustomTx/index.tsx	1	0%
src/routes/safe/components/Balances/SendModal/screens/ReviewCollectible/index.tsx	1	0%
src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/Review/index.tsx	2	0%

Totals
Change from base Build 1692167562:	0.05%
Covered Lines:	3163
Relevant Lines:	8685

💛 - Coveralls

github-actions · 2022-01-06T13:22:02Z

E2E Tests Failed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1693566450

Failed tests:

❌ Safe Apps List Safe Apps List
❌ Read-only transaction creation and review Read-only transaction creation and review

src/logic/hooks/useCanTxExecute.tsx

iamacook · 2022-01-08T15:56:13Z

src/logic/hooks/useCanTxExecute.tsx

+}
+
+// Review default values
+const useCanTxExecute: UseCanTxExecuteType = (preApprovingOwner = '', txConfirmations = 0) => {


Make sure to test this. I am pretty sure we have react-hooks-testing-library installed.

iamacook · 2022-01-08T15:56:52Z

src/logic/hooks/useCanTxExecute.tsx

+  const [canTxExecute, setCanTxExecute] = useState(false)
+  const nextQueuedTx = useSelector(nextTransactions)
+  const hasQueueNextTx = nextQueuedTx && Object.keys(nextQueuedTx).length > 0
+  const { threshold = 1 } = useSelector(currentSafe)


Why would the threshold be undefined?

src/logic/hooks/useCanTxExecute.tsx

src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx

src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/Review/index.tsx

...utes/safe/components/Balances/SendModal/screens/ContractInteraction/ReviewCustomTx/index.tsx

src/routes/safe/components/Balances/SendModal/screens/ReviewCollectible/index.tsx

src/routes/safe/components/Balances/SendModal/screens/ReviewSendFundsTx/index.tsx

…and icon

iamacook

Looking good, but why do you now have a mix-match of isExecution and canExecute since my last review?

src/logic/hooks/__tests__/useCanTxExecute.test.ts

src/logic/hooks/useCanTxExecute.tsx

src/routes/safe/components/Transactions/TxList/TxCollapsedActions.tsx

iamacook

All looks good but I still find the variable names confusing as it is not consistent. There is a mix match between: canExecute, canTxExecute and isExecution for what you return from useCanTxExecute(). I would name them all canTxExecute as that mirrors the name of the hook.

I think shouldExecute is a good choice for when the execution relies not only on what is returned from useCanTxExecute() but also something else.

  const canTxExecute = useCanTxExecute()
  const shouldExecute = otherFlag && canTxExecute

For clarification: canExecute is used for showing/hiding or enabling/disabling the confirm/reject buttons and isExecution is used extensively in process/createTransaction.

iamacook · 2022-01-10T16:46:04Z

src/logic/hooks/useCanTxExecute.tsx

+  const { threshold } = useSelector(currentSafe)
+
+  const safeAddress = extractSafeAddress()
+  const recommendedNonce = useGetRecommendedNonce(safeAddress) // to be changed. should be txNonce


Can you now remove this comment?

src/logic/hooks/useEstimateTransactionGas.tsx

src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx

src/routes/safe/components/Apps/components/SignMessageModal/ReviewMessage.tsx

src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/Review/index.tsx

src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx

src/routes/safe/components/Settings/SpendingLimit/NewLimitModal/Review.tsx

src/routes/safe/components/Settings/SpendingLimit/RemoveLimitModal.tsx

src/routes/safe/components/Settings/ThresholdSettings/ChangeThreshold/index.tsx

src/routes/safe/components/Settings/UpdateSafeModal/index.tsx

src/routes/safe/components/Transactions/TxList/modals/RejectTxModal.tsx

src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx

usame-algan

🚀

iamacook

LGTM! 🚀

francovenica · 2022-01-12T15:13:24Z

Issues:

1 - Gas limit is not being calculated for tx with higher nonce. Even tho gas limit is not needed for tx that will not be executed right away, the problem is that if the user decides to change the nonce to the current one he will submit the tx with a gas limit of 0. Currently in dev the gas limit is calculated for all tx, even those with a higher nonce than the current one

2 - Switching from future nonce to current nonce and back causes the warning message of "tx might fail" error message to popUp.
Create a tx and leave it in queue
Start the creation of a new tx, it will have current nonce + 1, so it cannot be executed
Edit that nonce to be the current one, the execute button shows up just fine
Edit it once again to be current + 1.
Now a message of "tx might fail" will show up and won't go away

3 - The confirm icon was replaced with the execute one:
Create a tx in a safe of 3 out of x owners and see the tx

This is the Icon that should show there:

…ling

iamacook · 2022-01-13T13:01:54Z

src/components/TransactionsFees/index.tsx

  txEstimationExecutionStatus,
 }: TransactionFailTextProps): React.ReactElement | null => {
+  const { currentVersion: safeVersion = '' } = useSelector(currentSafe)


You should not default the Safe version otherwise it won't calculate the possibility of off chain signing correctly. You should get it from the Safe instance.

github-actions · 2022-01-13T15:47:09Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	2	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

DiogoSoaress · 2022-01-13T15:53:59Z

Issues:

1 - Gas limit is not being calculated for tx with higher nonce. Even tho gas limit is not needed for tx that will not be executed right away, the problem is that if the user decides to change the nonce to the current one he will submit the tx with a gas limit of 0. Currently in dev the gas limit is calculated for all tx, even those with a higher nonce than the current one

2 - Switching from future nonce to current nonce and back causes the warning message of "tx might fail" error message to popUp. Create a tx and leave it in queue Start the creation of a new tx, it will have current nonce + 1, so it cannot be executed Edit that nonce to be the current one, the execute button shows up just fine Edit it once again to be current + 1. Now a message of "tx might fail" will show up and won't go away

3 - The confirm icon was replaced with the execute one: Create a tx in a safe of 3 out of x owners and see the tx

This is the Icon that should show there:

@francovenica I've tackled points 1. and 2. the point 3. is fixed in other PR #3283

Please give it another round 🙏

francovenica · 2022-01-13T19:56:26Z

I have a estrange behavior with a tx I want to sign, the tx is ready for execution, but it triggers a multisig first and then the proper execution modal.
This is the safe, you can try to execute the tx with a non-owner now, and you should see the same behavior
https://pr3257--safereact.review-safe.gnosisdev.com/app/rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac/transactions/queue

Gif:

DiogoSoaress · 2022-01-13T23:35:06Z

Hey Franco, I am having the same behaviour in DEV and I think we should address it asap. However if it is not related with this issue I suggest it shouldn't block this ticket unless it impedes QAing properly. Let me know

iamacook · 2022-01-14T08:13:36Z

I have a estrange behavior with a tx I want to sign, the tx is ready for execution, but it triggers a multisig first and then the proper execution modal.

This seems like quite a major bug that's worthwhile tackling this sprint (as another issue ofc), no?

francovenica · 2022-01-14T14:20:17Z

Ok, since the issue is in dev we can ignore the issue at least for this PR.

The issues reported were fixed. The gas is estimated for tx creation and playing around with the nonce is not causing the issue with the warning message
Also the icon is now fixed here, so probably the fix was merged, so that's a plus.

Diogo Soares added 6 commits January 6, 2022 12:06

Build: create a hook for getting the recommended nonce

a216e28

Build: extract "can execute transaction" to a hook

0266a1e

Build: remove unused return type

b084b55

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

f0fb4cf

Refactor: use ComponentProps to get component types

ab24bd3

Fix: remove from tests logic extracted to hook

240478a

Diogo Soares added 3 commits January 7, 2022 18:41

refactor: refactor the useCanExecuteType to test the logic

ffc1bfc

refactor: exclude the spending limit logic from useCanTxExecute

d56093b

refactor: use useCanTxExecute in other review modals

57612cf

usame-algan reviewed Jan 8, 2022

View reviewed changes

src/logic/hooks/useCanTxExecute.tsx Outdated Show resolved Hide resolved

iamacook reviewed Jan 8, 2022

View reviewed changes

Diogo Soares added 7 commits January 8, 2022 20:52

fix: improve useCanTxExecute types and logic

a989041

refactor: do not return isExecution from useEstimateTransactionGas

1fa2d51

fix: approve modal logic

e1c7de6

build: include PENDING_FAILED transactions in awaiting for execution

93ef7c2

build: flag execution to calculate tx gas

674c8a8

build: include PENDING_FAILED tx status in showing execution tooltip …

5cca7a9

…and icon

fix: adjust tests to reflect latest logic

31f0efb

DiogoSoaress marked this pull request as ready for review January 10, 2022 09:30

Diogo Soares added 2 commits January 10, 2022 10:36

fix: add missing hook dependency

01ac45e

fix: avoid race conditions when fetching from the backend

144a35c

DiogoSoaress requested review from dasanra, katspaugh and iamacook January 10, 2022 11:04

iamacook reviewed Jan 10, 2022

View reviewed changes

src/logic/hooks/__tests__/useCanTxExecute.test.ts Show resolved Hide resolved

src/logic/hooks/useCanTxExecute.tsx Outdated Show resolved Hide resolved

src/routes/safe/components/Transactions/TxList/TxCollapsedActions.tsx Outdated Show resolved Hide resolved

Diogo Soares added 2 commits January 10, 2022 17:12

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

738e7a2

fix: update tests to take manualSafeNonce

9b622dd

DiogoSoaress requested a review from usame-algan January 10, 2022 16:29

iamacook approved these changes Jan 10, 2022

View reviewed changes

fix: remove wrong check

be54a48

usame-algan reviewed Jan 11, 2022

View reviewed changes

refactor: rename execution related variables

f501dde

DiogoSoaress requested a review from usame-algan January 11, 2022 13:34

usame-algan reviewed Jan 11, 2022

View reviewed changes

src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx Outdated Show resolved Hide resolved

fix: rename variable

669f8e4

DiogoSoaress requested a review from usame-algan January 11, 2022 15:45

usame-algan approved these changes Jan 11, 2022

View reviewed changes

iamacook approved these changes Jan 11, 2022

View reviewed changes

Diogo Soares and others added 3 commits January 13, 2022 13:04

Obtain isOffChainSignature by calling a function instead of prop dril…

3f26613

…ling

Calculate tx gas even for off chain transactions

fa26e8d

Merge branch 'dev' into hide-execute-checkbox-conditions

572c8f8

DiogoSoaress requested a review from iamacook January 13, 2022 12:47

iamacook reviewed Jan 13, 2022

View reviewed changes

fix TransactionFees rendering

7e8d179

katspaugh mentioned this pull request Jan 13, 2022

Transactions that cannot be immediately executed still show execution parameters #2943

Closed

DiogoSoaress merged commit 42c2ee6 into dev Jan 14, 2022

DiogoSoaress deleted the hide-execute-checkbox-conditions branch January 14, 2022 16:14

github-actions bot locked and limited conversation to collaborators Jan 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: conditions to display "Execute transaction" #3257

Fix: conditions to display "Execute transaction" #3257

DiogoSoaress commented Jan 6, 2022 •

edited

Loading

github-actions bot commented Jan 6, 2022

github-actions bot commented Jan 6, 2022 •

edited

Loading

github-actions bot commented Jan 6, 2022

coveralls commented Jan 6, 2022 •

edited

Loading

github-actions bot commented Jan 6, 2022 •

edited

Loading

iamacook Jan 8, 2022

iamacook Jan 8, 2022

iamacook left a comment

iamacook left a comment •

edited

Loading

iamacook Jan 10, 2022

usame-algan left a comment

iamacook left a comment

francovenica commented Jan 12, 2022

iamacook Jan 13, 2022

github-actions bot commented Jan 13, 2022

DiogoSoaress commented Jan 13, 2022

francovenica commented Jan 13, 2022

DiogoSoaress commented Jan 13, 2022

iamacook commented Jan 14, 2022

francovenica commented Jan 14, 2022

Fix: conditions to display "Execute transaction" #3257

Fix: conditions to display "Execute transaction" #3257

Conversation

DiogoSoaress commented Jan 6, 2022 • edited Loading

What it solves

How this PR fixes it

How to test it

Screenshots

github-actions bot commented Jan 6, 2022

github-actions bot commented Jan 6, 2022 • edited Loading

ESLint Summary View Full Report

github-actions bot commented Jan 6, 2022

Deployment links

coveralls commented Jan 6, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1693532428

💛 - Coveralls

github-actions bot commented Jan 6, 2022 • edited Loading

iamacook Jan 8, 2022

Choose a reason for hiding this comment

iamacook Jan 8, 2022

Choose a reason for hiding this comment

iamacook left a comment

Choose a reason for hiding this comment

iamacook left a comment • edited Loading

Choose a reason for hiding this comment

iamacook Jan 10, 2022

Choose a reason for hiding this comment

usame-algan left a comment

Choose a reason for hiding this comment

iamacook left a comment

Choose a reason for hiding this comment

francovenica commented Jan 12, 2022

iamacook Jan 13, 2022

Choose a reason for hiding this comment

github-actions bot commented Jan 13, 2022

ESLint Summary View Full Report

DiogoSoaress commented Jan 13, 2022

francovenica commented Jan 13, 2022

DiogoSoaress commented Jan 13, 2022

iamacook commented Jan 14, 2022

francovenica commented Jan 14, 2022

DiogoSoaress commented Jan 6, 2022 •

edited

Loading

github-actions bot commented Jan 6, 2022 •

edited

Loading

coveralls commented Jan 6, 2022 •

edited

Loading

github-actions bot commented Jan 6, 2022 •

edited

Loading

iamacook left a comment •

edited

Loading