banteg / brownie-safe

gnosis safe tx builder
http://safe.ape.tax/
271 stars 70 forks source link

Error when estimating gas #3

Open jo-tud opened 3 years ago

jo-tud commented 3 years ago

I'm receive the following error, when trying the to estimate the gas for any safe tx:

>>> safe.estimate_gas(safe_tx)
  File "<console>", line 1, in <module>
  File "ape_safe.py", line 137, in estimate_gas
    return self.estimate_tx_gas(safe_tx.to, safe_tx.value, safe_tx.data, safe_tx.operation)
  File "gnosis/safe/safe.py", line 530, in estimate_tx_gas
    return self.estimate_tx_gas_with_web3(to, value, data) + ADDITIONAL_GAS + WEB3_ESTIMATION_OFFSET
  File "gnosis/safe/safe.py", line 469, in estimate_tx_gas_with_web3
    raise CannotEstimateGas('Cannot estimate gas with `eth_estimateGas`') from exc
CannotEstimateGas: Cannot estimate gas with `eth_estimateGas`

No idea why that would be happening. safe.preview(safe_tx, call_trace=True) works without flaw and shows the gas used...

Tritium-VLK commented 3 years ago

I am also having this problem.

gosuto-inzasheru commented 3 years ago

Seems to stem from a ValueError caught here: https://github.com/gnosis/gnosis-py/blob/af806a39ada2d47cdf368218a98abb3a8b3f322b/gnosis/safe/safe.py#L513

Tritium-VLK commented 3 years ago

@banteg We at badger would love to fund a gitcoin bounty for the next round to solve this problem. Are you already working on it and/or would it be helpful to have someone fork this repo and sort that out?

gosuto-inzasheru commented 3 years ago

some form of this problem persists after #11:

    safe.estimate_gas(safe_tx)
  File "./ape_safe.py", line 151, in estimate_gas
    return self.estimate_tx_gas(safe_tx.to, safe_tx.value, safe_tx.data, safe_tx.operation)
  File "gnosis/safe/safe.py", line 577, in estimate_tx_gas
    return self.estimate_tx_gas_with_web3(to, value, data) + ADDITIONAL_GAS + WEB3_ESTIMATION_OFFSET
  File "gnosis/safe/safe.py", line 514, in estimate_tx_gas_with_web3
    raise CannotEstimateGas(f'Cannot estimate gas with `eth_estimateGas`: {exc}') from exc
CannotEstimateGas: Cannot estimate gas with `eth_estimateGas`: execution reverted: VM Exception while processing transaction: revert MultiSend should only be called via delegatecall

ape-safe v0.2.1 gnosis-py v3.2.2 eth-brownie v1.16.3 macos v11.6

mmv08 commented 3 years ago

some form of this problem persists after #11:

    safe.estimate_gas(safe_tx)
  File "./ape_safe.py", line 151, in estimate_gas
    return self.estimate_tx_gas(safe_tx.to, safe_tx.value, safe_tx.data, safe_tx.operation)
  File "gnosis/safe/safe.py", line 577, in estimate_tx_gas
    return self.estimate_tx_gas_with_web3(to, value, data) + ADDITIONAL_GAS + WEB3_ESTIMATION_OFFSET
  File "gnosis/safe/safe.py", line 514, in estimate_tx_gas_with_web3
    raise CannotEstimateGas(f'Cannot estimate gas with `eth_estimateGas`: {exc}') from exc
CannotEstimateGas: Cannot estimate gas with `eth_estimateGas`: execution reverted: VM Exception while processing transaction: revert MultiSend should only be called via delegatecall

ape-safe v0.2.1 gnosis-py v3.2.2 eth-brownie v1.16.3 macos v11.6

This is not related to the initial error; you tried to estimate a transaction that calls multicall contract via the CALL opcode, and it should be DELEGATECALL instead. If you use multisend_from_receipts it should use delegate call by default. Could you share the script?

gosuto-inzasheru commented 3 years ago

really want to get to the bottom of this, i think passing good gas values to the final tx is important.

i create a new env, with nothing else but ape-safe==0.2.2 installed. i then run one single tx from a safe to a wallet.

this seems to fail in the same way @jo-tud reports in the beginning of this issue:

  File "./scripts/example.py", line 12, in main
    safe.estimate_gas(safe_tx)
  File "ape_safe.py", line 161, in estimate_gas
    return self.estimate_tx_gas(safe_tx.to, safe_tx.value, safe_tx.data, safe_tx.operation)
  File "gnosis/safe/safe.py", line 577, in estimate_tx_gas
    return self.estimate_tx_gas_with_web3(to, value, data) + ADDITIONAL_GAS + WEB3_ESTIMATION_OFFSET
  File "gnosis/safe/safe.py", line 514, in estimate_tx_gas_with_web3
    raise CannotEstimateGas(f'Cannot estimate gas with `eth_estimateGas`: {exc}') from exc
CannotEstimateGas: Cannot estimate gas with `eth_estimateGas`: execution reverted: VM Exception while processing transaction: revert

someone suggested to use the --noVMErrorsOnRPCResponse flag for ganache. this helps a little bit, in that we get some more output (before failing in the same way):

Safe=0xFEB4acf3df3cDEA7399794D0869ef76A6EfAff52 Problem estimating gas, returned value without gas limit set is 0x for tx={[...]}

now if i run the script again, but this time with amount anything else but the full balance of the safe, it works! somehow doing

amount = usdc.balanceOf(safe.address)
usdc.transfer(wallet_addr, amount)

breaks .estimate_gas, but transferring amount / 2 or 0 give a successful output for .estimate_gas.

however, its output

recommended gas limit: 96060
estimate gas: 59614

differs a lot from the recommended gas limit. also, in all the of erroneous cases above, .preview was always able to output a (seemingly) proper recommended gas limit.

and tbh, all i care for is that that recommended gas limit ends up in safe_tx.safe_tx_gas and in safe_tx.base_gas before posting.

so hereby a feature/fix request: let there be a function which grabs recommended gas and sets safe_tx.safe_tx_gas and safe_tx.base_gas accordingly, preferably with some adjustable safety margin (either in percentages or absolute gas value).

gosuto-inzasheru commented 3 years ago

my understanding is that this issue is related to using gnosis safes <v.1.3.0. the implementation of https://github.com/gnosis/safe-contracts/issues/274 in v.1.3.0 makes setting safe_tx_gas obsolete, and makes the estimate_gas function work as expected.

for those still on <v1.3.0, @Uxio0's pr #15 is a workaround that removes dependency of web3's estimate_gas, and grabs it from self.preview().gas_used instead (with some additional margin calcs).

Uxio0 commented 3 years ago

Yes, in Safes < v1.3.0 safeTxGas needs to be set to prevent web3 providers underestimating. In v1.3.0 this is not needed, leaving safeTxGas=0 will lead to a proper estimation