Safe-react: Displayed tx fee wrong

[tschubotz]

Title/Description

Dispalyed tx fee is wrong on the tx modal.

Seems like only safeTxGas is taken into account, not the Ethereum gas limit.

Environment

• Browser: Firefox
• Safe: https://rinkeby.gnosis-safe.io/app/#/safes/0xaE3c91c89153DEaC332Ab7BBd167164978638c30/transactions
• Wallet: MM
• App Version: 2.19.2
• Environment:
  
  
  
  • production (rinkeby)
  
  

Steps to reproduce

1. Create a tx (I did an ERC20 transfer)
2. Collect signatures
3. Execute
4. Check advanced params before executing.

Expected result

Ethereum gas limit * gas price (in GWei) / 1e9 = Tx fee i.e. 0.025828 in my example.

Obtained result

0.01081 ETH is displayed.

It seems like only the safeTxGas is taken into account here. However it should really the Ethereum gas limit that's taken into account.
The Metamask calculation is correct. I expect that Metamask calculation to be in sync with our tx fee calculation.

Screenshots

_Rating this as major since it's about displaying how much funds are spent, max._

[dasanra]

@tschubotz which is the final spending of the TX if it's correctly executed? The estimation message is showing the amount you will pay if transaction is successful. Metamask is showing worst case (transaction fails and all gas is used up to gasLimit)

[tschubotz]

which is the final spending of the TX if it's correctly executed?

Depends on the tx, but it the case above it was well above the 0.01081 that we displayed.

The estimation message is showing the amount you will pay if transaction is successful

So you are saying we can estimate it better, right? Why don't we then send this better estimation to Metamask so there the user doesn't need to sign something higher? :)

Metamask is showing worst case (transaction fails and all gas is used up to gasLimit)

We set the Metamask gas price and gas limit, so Metamask shows what we set, right? And the user needs the ~0.025 ETH in their account in order to submit the tx. That's why I think we should just be honest in our popup and display that value.

I don't understand why we just use safeTxGas for calculating the displayed fee in our popup. Why is that?

[nicosampler]

I followed the same example as Tobi.

MM is estimating what we are sending as gasLimit but our UI is estimation gasLimit/2.

Safe estimation was

MM estimation

BUT...
the tx execution took a little more than the one suggested by the safe:

Not sure what should we do here, because as far as I understand:

• MM is using the gasLimit we are providing and converting it to Eth, that's why it suggests 0.013Eth.
• Safe UI is doing the same without multiplying gasLimit *2 (*2 is added to estimate the worst scenario where the tx will fail and the miners can take their fee)

If I change our code to take the gas cost using gasLimit * 2 both estimations are almost identical between MM and safe UI.

@dasanra @tschubotz

[nicosampler]

So.. our UI is trying to show the most accurate estimation, but MM is estimating what we are sending to it (gasLimit)

[tschubotz]

Thanks for checking this our. So one of the recent gas estimation fixes, seems to have changed it in a way that not only safeTxGas is taken into account for the fee that we display in our interface - nice!

• MM is using the gasLimit we are providing and converting it to Eth, that's why it suggests 0.013Eth.

• Safe UI is doing the same without multiplying gasLimit *2 (*2 is added to estimate the worst scenario where the tx will fail and the miners can take their fee)

What I don't understand is the following: We set the (ETH) gas limit, right? So if we know better, i.e. we can estimate it better, then why do we not straight set it to just gasLimit instead of 2* gasLimit?

cc @rmeissner

Either way, I think the number in our interface and Metamask should definitely match up (no matter if 1x or 2x) since we write "make sure you have xxx in your wallet". Users aren't able to execute the tx with just the value we display. They would have to manually lower the gas limit in metamask. no one really does that. Hence the numbers should be the same.

[dasanra]

I think the solution here is to show the estimation message using gasLimit value, as it's the same we send to Metamask and like this we should show the same estimate value.

Using gasLimit tells the user the necessary funds amount to guarantee that transaction would go through

[nicosampler]

What I don't understand is the following: We set the (ETH) gas limit, right? So if we know better, i.e. we can estimate it better, then why do we not straight set it to just gasLimit instead of 2* gasLimit?

2x Is required, because if tx fails, miners need to be sure they will receive all the gas fee otherwise the tx won't be eligible by them. So we increase that amount to make sure enough gas is provided for the worst scenario.

I agreed with Dani, the solution should be to show in our UI the gas required using gasLimit*2.

[tschubotz]

2x Is required, because if tx fails, miners need to be sure they will receive all the gas fee otherwise the tx won't be eligible by them. So we increase that amount to make sure enough gas is provided for the worst scenario.

I don't understand this. Miners don't care if your tx fails or runs out of gas or whatever in my opinion. They get the fee payment anyway, even if a tx fails.

I agreed with Dani, the solution should be to show in our UI the gas required using gasLimit*2.

Making sure that the numbers match up is important imo. 👍