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

(Feature) - #1048 Tx will fail warning#1675

Merged
dasanra merged 111 commits intodevelopmentfrom
feature/1048-tx-will-fail-warning
Jan 13, 2021
Merged

(Feature) - #1048 Tx will fail warning#1675
dasanra merged 111 commits intodevelopmentfrom
feature/1048-tx-will-fail-warning

Conversation

@Agupane
Copy link
Contributor

@Agupane Agupane commented Nov 30, 2020

Closes #1048 by:

  • Using the function checkIfTxWillFail to show/hide a red warning on the approveTxModal
  • Refactoring the modals and the transaction gas functions
  • Creating a new useCheckIfTransactionWillFail hook that will receive txData, safeAddress and txRecipient and will try to estimate the gas for executing it, in case the returned gas is bellow 0, it will return true

Note: this PR won't add the new signing flow

Looks like this now:

image

@Agupane Agupane self-assigned this Nov 30, 2020
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Nov 30, 2020

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

@ghost
Copy link

ghost commented Nov 30, 2020

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Nov 30, 2020

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/volta/app

@Agupane Agupane marked this pull request as ready for review December 1, 2020 13:20
@Agupane Agupane requested review from dasanra, fernandomg and francovenica and removed request for fernandomg December 1, 2020 13:20
@Agupane Agupane added the WIP label Dec 1, 2020
@Agupane Agupane marked this pull request as draft December 1, 2020 15:08
@ghost
Copy link

ghost commented Dec 2, 2020

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Dec 2, 2020

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Dec 2, 2020

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/volta/app

@Agupane
Copy link
Contributor Author

Agupane commented Jan 11, 2021

(1) I only got the alert when executing the transaction, i.e. not when just signing or creating a tx for a Safe with threshold > 1. Is is not possible to show the alert there?

Yes, it's possible, but not here, we are doing that on this pr. Until now we didn't differentiate between creation and execution of the transactions in order to estimate the gas or see if the tx will fail based on the returned gas value (<= 0). We just had the difference between tx approval and tx execution. You should be able to check that on that pr. In this pr you would receive a warning just when executing, or in case that you need to do an on-chain signature, a warning in case that sign process will fail.

(2) I always got the error when performing Safe app transactions, i.e. multisends. Why is that? That was for ultimately non-failing txs.

I didn't found a case in which this happens, could you send steps?

@tschubotz

@ghost
Copy link

ghost commented Jan 11, 2021

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/rinkeby/app

@tschubotz
Copy link
Contributor

I didn't found a case in which this happens, could you send steps?

@Agupane What I did is:

  1. Open Safe (threshold 2): https://pr1675--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0xaE3c91c89153DEaC332Ab7BBd167164978638c30/
  2. Go to Safe apps
  3. Open tx builder
  4. Target: 0x5592EC0cfb4dbc12D3aB100b257153436a1f0FEa
  5. Choose approve method
  6. Enter Safe as spender (0xaE3c91c89153DEaC332Ab7BBd167164978638c30)
  7. Enter 1 as value
  8. Add tx
  9. Send tx
  10. Sign with owner 1
  11. execute with owner 2 -> In the popup I see the alert.
  12. I execute
  13. Success, here's the tx: https://rinkeby.etherscan.io/tx/0x642972cac5fe601d5dded3159bc3514eee2db46e02916815e43c1a68b07bd3b0

The same with the Compound Safe app when supplying 1 Rinkeby day to compound https://rinkeby.etherscan.io/tx/0xd5696dc1d44548b3e74def6adacb78fb3bf80a561da80561cacb39fc32f1c810

@ghost
Copy link

ghost commented Jan 11, 2021

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/volta/app

@Agupane
Copy link
Contributor Author

Agupane commented Jan 11, 2021

Should be fixed now @tschubotz

…r-gas-calculation

# Conflicts:
#	src/routes/safe/components/Transactions/TxsTable/ExpandedTx/ApproveTxModal/index.tsx
@ghost
Copy link

ghost commented Jan 11, 2021

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 11, 2021

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/volta/app

@dasanra dasanra requested a review from francovenica January 11, 2021 19:10
@dasanra
Copy link
Collaborator

dasanra commented Jan 11, 2021

@francovenica we will need another check in this PR
We updated the estimation algorithm. You could also check if the issues found by @tschubotz were solved

@ghost
Copy link

ghost commented Jan 11, 2021

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 12, 2021

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Jan 12, 2021

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 12, 2021

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/volta/app

@tschubotz
Copy link
Contributor

The cases from before work now for me 👏

@francovenica
Copy link
Contributor

Safe used: https://pr1675--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

I wasn't able to use the hash Tobi provided, but I used one from an ERC20 I've created
0xb4d0593cec06d78db3723ae442f6fe2662103928

I transfer some of those tokens to the safe.
I was able to transfer tokens from the safe to another account with no issues
I was able to use the ´approve´ method Tobi mentioned and it worked fine https://rinkeby.etherscan.io/tx/0x71d514e1eef753a59a22af5648ca7f5e85db8e703612bbaaccdf56cdaeb339b2
I tried to transfer more tokens than I had and I got the warning message. When I gave more tokens to the safe the warning message was shown no more.
Every time I got the message the tx end up failing
Every time I did NOT got the message, the tx went through fine

@francovenica
Copy link
Contributor

francovenica commented Jan 12, 2021

CC to @Agupane @fernandomg
Regarding the gas estimation, is hard to test in rinkeby since every tx is really cheap, so all of them show it will cost < 0.001 ETH.

The issue I found is that the message is not working properly with the "Spending limit" feature
I have a safe that has a 2 of 2 owners needed to sign, but the spending limit type of tx is an on-chain tx and is expected, in the review step, to show me how much gas I will use to sign the tx, but it seems the system doesn't understand this and takes the tx as an off-chain one, by no showing me that I'll spend any gas:

So steps would be:
Have a safe with a 2 out of 2 owners needed
Set the spending limit for an owner with any token
Attempt to create a "Send funds" with that token and select "Spending limit"

Is expected: The review step shows how much gas this tx will need
What happens: It shows that no gas will be spent, like if this was an off-chain tx

image.png

Safe used: https://pr1675--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

@ghost
Copy link

ghost commented Jan 12, 2021

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 12, 2021

Travis automatic deployment:
https://pr1675--safereact.review.gnosisdev.com/volta/app

@dasanra dasanra merged commit 8a774f2 into development Jan 13, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2021
@dasanra dasanra deleted the feature/1048-tx-will-fail-warning branch January 14, 2021 10:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

There should be a warning if we know that a Safe tx will fail

8 participants