Feature: allow editing EIP-1559 max priority fee + refactor tx modals #3297

katspaugh · 2022-01-13T10:15:27Z

What it solves

Resolves #3185

How this PR fixes it

Adds an editable Max Prio field in the "editable tx params" component, and passes it to the wallet. Defaults to 2.5 GWei.
Refactored tx modals to not repeat this logic in each of the 20ish modals

How to test it

Make transactions on EIP-1559 networks (Mainnet, Rinkeby, xDai)
Make txns on non-1559 networks
Single owner skipping tx execution - Settings transactions not skippable #2935

Affected modals

Issues Found:

No new tickets, just listing the comments with the reported issues. All the issues apply to rinkeby, xdai, eth

Gas price/gas limit is set to default values when unchecking the "execute now" tx (technically fixed in fix: Dont estimate gas in ApproveTxModal if checkbox is not checked #3363, we should merge that ticket in this one)
Max priority fee field can be left empty + Error message too long

issues tested and fixed

Wrong type of tx show when executing a tx (fixed)
Warning message when unchecking the "execute now" checkbox
Policies allow to set the same policies already present
Tx in policies execute even when the "Execute tx" checkbox is not checked

Safes used so far:
rin:0x9913B9180C20C6b0F21B6480c84422F6ebc4B808
rin:0x51c125bb0D2FB3394129e4AAa97BbF91e737d9b2
rin:0xFfDC1BcdeC18b1196e7FA04246295DE3A17972Ac
rin:0x37A53791b3667c803318D083D339C236781D57D1
gno:0xB8d760a90a5ed54D3c2b3EFC231277e99188642A

github-actions · 2022-01-13T10:15:44Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2022-01-13T10:19:14Z

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	1	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

coveralls · 2022-01-13T10:21:37Z

Pull Request Test Coverage Report for Build 1744348605

55 of 148 (37.16%) changed or added relevant lines in 31 files are covered.
16 unchanged lines in 12 files lost coverage.
Overall coverage increased (+1.3%) to 33.635%

Changes Missing Coverage	Changed/Added Lines	%
src/logic/safe/store/actions/createTransaction.ts	1	0.0%
src/routes/CreateSafePage/components/SafeCreationProcess.tsx	1	0.0%
src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx	1	0.0%
src/routes/safe/components/Settings/ManageOwners/AddOwnerModal/screens/Review/index.tsx	1	0.0%
src/routes/safe/components/Settings/ManageOwners/RemoveOwnerModal/index.tsx	1	0.0%
src/routes/safe/components/Settings/ManageOwners/RemoveOwnerModal/screens/Review/index.tsx	1	0.0%
src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/index.tsx	1	0.0%
src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/screens/Review/index.tsx	1	0.0%
src/routes/safe/components/Settings/SpendingLimit/RemoveLimitModal.tsx	1	0.0%
src/routes/safe/components/Settings/UpdateSafeModal/index.tsx	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/components/TransactionsFees/index.tsx	1	75.0%
src/logic/hooks/useEstimateTransactionGas.tsx	1	20.54%
src/routes/CreateSafePage/components/SafeCreationProcess.tsx	1	4.76%
src/routes/safe/components/Apps/components/SignMessageModal/ReviewMessage.tsx	1	76.19%
src/routes/safe/components/Settings/Advanced/RemoveGuardModal.tsx	1	70.0%
src/routes/safe/components/Settings/ManageOwners/AddOwnerModal/index.tsx	1	0%
src/routes/safe/components/Transactions/helpers/EditTxParametersForm/index.tsx	1	18.52%
src/routes/safe/components/Transactions/TxList/modals/ApproveTxModal.tsx	1	2.69%
src/routes/safe/components/Balances/SendModal/screens/ContractInteraction/Review/index.tsx	2	0%
src/routes/safe/components/Settings/SpendingLimit/RemoveLimitModal.tsx	2	0%

Totals
Change from base Build 1740925373:	1.3%
Covered Lines:	3178
Relevant Lines:	8459

💛 - Coveralls

github-actions · 2022-01-13T10:23:22Z

Deployment links

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

github-actions · 2022-01-13T10:30:42Z

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

Failed tests:

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

iamacook

Looking good guys but your recent push didn't let me complete my review. I will continue now.

src/logic/hooks/useEstimateTransactionGas.tsx

src/logic/safe/store/actions/createTransaction.ts

src/logic/safe/store/actions/processTransaction.ts

src/logic/safe/transactions/__tests__/gas.test.ts

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

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

iamacook

Looking good boys!

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

src/routes/safe/components/Transactions/helpers/EditTxParametersForm/index.tsx

src/routes/safe/components/Transactions/helpers/EditableTxParameters.tsx

src/routes/safe/components/Transactions/helpers/TxParamsState/index.tsx

src/routes/safe/container/hooks/useTransactionParameters.ts

src/routes/safe/components/Transactions/helpers/TxParamsState/index.tsx

…eview modals

francovenica · 2022-01-21T18:14:26Z

This PR still has the issue of the wrong type of tx signature being shown:

Updated to the latest code, it should be good now.

Yes, is fixed now. Thanks

francovenica · 2022-01-22T07:40:40Z

In the "change policies" modal, in a safe 1/x, unchecking the "Execute tx" checkbox does nothing, the tx executes no matter what (you cannot queue this tx)
See in this gif how the checkbox is un marked as the tx is submitted, but the MM pop up is a onchain type of tx. submitting that executes the tx.

francovenica · 2022-01-24T00:36:10Z

Ignore this. The field is shown for trezor and ledger as well in rinkeby
Question: For trezor, it seems that Trezor T should be able to support EIP1559 according to this
I don't have a trezor T to check if it works, but assuming it doesn't, should we deal with it separately right?

DiogoSoaress · 2022-01-24T08:57:51Z

@gnosis/safe-web By unifying the modals we are displaying in every modal the option to “Execute Transaction”. However, this option is not available in all the PROD modals, for example it does not exist in the Safe owner management ones (add owner, ...).
Shall we keep the same UX and not enable these transaction types to delay execution or should we enable the option to delay execution in all the modals?

katspaugh · 2022-01-24T09:00:24Z

All modals should behave the same wrt execution/gas info.

katspaugh · 2022-01-24T09:06:22Z

It looks like delayExection isn't passed to createTransaction.

It should be passed like this.

@DiogoSoaress please check that all the modals are passing it.

DiogoSoaress · 2022-01-24T09:15:19Z

It looks like delayExection isn't passed to createTransaction.

It should be passed like this.

@DiogoSoaress please check that all the modals are passing it.

@katspaugh Please review the last commit. Addresses all the modals that didn't have the flag in the onSubmit callback

katspaugh · 2022-01-24T09:22:48Z

Perfect, thank you 👍

DiogoSoaress · 2022-01-24T11:00:03Z

This bug is already present in DEV. I've created a new issue for it #3358 .

iamacook · 2022-01-24T11:01:06Z

This bug is already present in DEV. I've created a new issue for it #3358 .

Probably makese sense to include it in the reafctor of the approval window, no?

DiogoSoaress · 2022-01-24T11:06:08Z

I say we should fix it in the current sprint @johannesmoormann

I've created a separate bug for it as it is already in dev and thus not tightly related with this PR

iamacook · 2022-01-24T11:09:58Z

I've created a separate bug for it as it is already in dev and thus not tightly related with this PR

Are you not refactoring the approval modals in the sprint as well?

katspaugh · 2022-01-24T16:44:55Z

Warning message when unchecking the "execute now" checkbox
Gas price/gas limit is set to default values when unchecking the "execute now" tx

This'll be fixed in #3363.

…3363) * fix: Dont estimate gas in ApproveTxModal if checkbox is not checked * fix: revert to using custom hook for canTxExecute * fix: Remove local state from useCanTxExecute custom hook

DiogoSoaress · 2022-01-25T14:07:42Z

@francovenica this issue #2935 is solved by this PR 🎉

francovenica · 2022-01-25T19:05:03Z

All these issues have been fix so far
Wrong type of tx show when executing a tx (fixed)
Warning message when unchecking the "execute now" checkbox
Policies allow to set the same policies already present
Tx in policies execute even when the "Execute tx" checkbox is not checked

New issue:

The max priority fee can be left empty. MM still receives the default value of 2.5 if this happens
The max priority fee has an error message that is too long when a value <0 is set. The problem is that the error message won't allow the user to click the field to correct the value (unless it clicks almost at end bottom of the input)

DiogoSoaress · 2022-01-26T09:00:18Z

Hey @francovenica , regarding your last comments:

The max priority fee can be left empty. MM still receives the default value of 2.5 if this happens

We are sending 2.5 as the default value. I will add this information to the PR description.

The max priority fee has an error message that is too long when a value <0 is set. The problem is that the error message won't allow the user to click the field to correct the value (unless it clicks almost at end bottom of the input)

This issue occurs in all the other modal inputs. I suggest to open a ticket to address it.

katspaugh and others added 11 commits January 12, 2022 11:28

Feature: display max fee and max prio fee

e2c72a5

Add default ethMaxPrioFee and pass it to the form

3c4cffc

Refactor tx params

6867693

Implement util to create send params

ea4a26d

Pass an object

fbfa341

Sum maxFee and maxPrioFee

25871b9

Refactor Owner and Guard modals

ac27a8d

Merge branch 'dev' of github.com:gnosis/safe-react into max-fee

f2f4eca

Rm console.log

a5465e7

Refactor replace and remove owner modals

e898729

refactor ChangeThreshold and ReviewCollectible modals

5e39473

katspaugh marked this pull request as draft January 13, 2022 10:15

katspaugh requested review from iamacook and usame-algan January 13, 2022 10:15

Merge with dev

dbd5d85

iamacook reviewed Jan 13, 2022

View reviewed changes

usame-algan reviewed Jan 13, 2022

View reviewed changes

src/routes/safe/components/Transactions/helpers/TxParamsState/index.tsx Outdated Show resolved Hide resolved

Diogo Soares added 7 commits January 14, 2022 19:05

add tests to createSendParams util function

df00c89

change getDefaultGasEstimation params type to be an object

9413cbf

refactor isSpendingLimit to an util function

eb603e2

address comments from the Review

ea0e676

refactor UpdateSafeModal and ReviewCustomTx modals

40309ad

refactor RemoveModuleModal and RemoveLimitModal modals

2734596

refactor ReviewMessage, ReviewSpendingLimits and ContractInteractionR…

69e54c0

…eview modals

Add delay execution to the onSubmit callback in all the unifiyed modals

d227a3b

Disable submit while threshold is not changed

f80751f

DiogoSoaress mentioned this pull request Jan 24, 2022

Advanced options values change if Approve Transaction unchecks "Execute transaction" checkbox #3358

Closed

katspaugh added 5 commits January 24, 2022 14:53

Merge branch 'dev' of github.com:gnosis/safe-react into max-fee

2a9049c

Fix formatted gas cost

0a26326

Fix formatted gas cost in ApproveTxModal

1d28c4e

canTxExecute -> doExecute

c3ff9a5

Merge branch 'dev' of github.com:gnosis/safe-react into max-fee

58de740

fix: Dont estimate gas in ApproveTxModal if checkbox is not checked (#…

ef03cb1

…3363) * fix: Dont estimate gas in ApproveTxModal if checkbox is not checked * fix: revert to using custom hook for canTxExecute * fix: Remove local state from useCanTxExecute custom hook

katspaugh mentioned this pull request Jan 25, 2022

refactor: use TxModalWrapper in ApproveTxModal #3369

Merged

usame-algan mentioned this pull request Jan 26, 2022

feat: Redesign Tx flow modals #3368

Closed

18 tasks

DiogoSoaress merged commit f1c0c49 into dev Jan 26, 2022

DiogoSoaress deleted the max-fee branch January 26, 2022 10:15

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: allow editing EIP-1559 max priority fee + refactor tx modals #3297

Feature: allow editing EIP-1559 max priority fee + refactor tx modals #3297

katspaugh commented Jan 13, 2022 •

edited by DiogoSoaress

github-actions bot commented Jan 13, 2022

github-actions bot commented Jan 13, 2022 •

edited

coveralls commented Jan 13, 2022 •

edited

github-actions bot commented Jan 13, 2022

github-actions bot commented Jan 13, 2022 •

edited

iamacook left a comment

iamacook left a comment

francovenica commented Jan 21, 2022

francovenica commented Jan 22, 2022

francovenica commented Jan 24, 2022 •

edited

DiogoSoaress commented Jan 24, 2022

katspaugh commented Jan 24, 2022

katspaugh commented Jan 24, 2022 •

edited

DiogoSoaress commented Jan 24, 2022

katspaugh commented Jan 24, 2022

DiogoSoaress commented Jan 24, 2022 •

edited

iamacook commented Jan 24, 2022

DiogoSoaress commented Jan 24, 2022 •

edited

iamacook commented Jan 24, 2022

katspaugh commented Jan 24, 2022 •

edited

DiogoSoaress commented Jan 25, 2022

francovenica commented Jan 25, 2022

DiogoSoaress commented Jan 26, 2022

Feature: allow editing EIP-1559 max priority fee + refactor tx modals #3297

Feature: allow editing EIP-1559 max priority fee + refactor tx modals #3297

Conversation

katspaugh commented Jan 13, 2022 • edited by DiogoSoaress

What it solves

How this PR fixes it

How to test it

Affected modals

Issues Found:

issues tested and fixed

github-actions bot commented Jan 13, 2022

github-actions bot commented Jan 13, 2022 • edited

ESLint Summary View Full Report

coveralls commented Jan 13, 2022 • edited

Pull Request Test Coverage Report for Build 1744348605

💛 - Coveralls

github-actions bot commented Jan 13, 2022

Deployment links

github-actions bot commented Jan 13, 2022 • edited

iamacook left a comment

Choose a reason for hiding this comment

iamacook left a comment

Choose a reason for hiding this comment

francovenica commented Jan 21, 2022

francovenica commented Jan 22, 2022

francovenica commented Jan 24, 2022 • edited

DiogoSoaress commented Jan 24, 2022

katspaugh commented Jan 24, 2022

katspaugh commented Jan 24, 2022 • edited

DiogoSoaress commented Jan 24, 2022

katspaugh commented Jan 24, 2022

DiogoSoaress commented Jan 24, 2022 • edited

iamacook commented Jan 24, 2022

DiogoSoaress commented Jan 24, 2022 • edited

iamacook commented Jan 24, 2022

katspaugh commented Jan 24, 2022 • edited

DiogoSoaress commented Jan 25, 2022

francovenica commented Jan 25, 2022

DiogoSoaress commented Jan 26, 2022

katspaugh commented Jan 13, 2022 •

edited by DiogoSoaress

github-actions bot commented Jan 13, 2022 •

edited

coveralls commented Jan 13, 2022 •

edited

github-actions bot commented Jan 13, 2022 •

edited

francovenica commented Jan 24, 2022 •

edited

katspaugh commented Jan 24, 2022 •

edited

DiogoSoaress commented Jan 24, 2022 •

edited

DiogoSoaress commented Jan 24, 2022 •

edited

katspaugh commented Jan 24, 2022 •

edited