MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
12.08k stars 4.93k forks source link

[Bug]: gasLimit auto increased when sending erc20 #19318

Open shunjizhan opened 1 year ago

shunjizhan commented 1 year ago

Describe the bug

when I send erc20 with MM, it uses gasLimit which 50% greater than eth_estimateGas return. I did some grep around and it seems to be a "feature". https://github.com/MetaMask/metamask-extension/issues/8771#issuecomment-759243828

However this become a bug in our evm network, where gasLimit has some specific encoding, so can't be altered randomly. I hope to make MM use exactly what eth_estimateGas return.

This 50% increase is a very opinionated operation that's only required for networks that has the gas underestimation issue. So I think there should either be a way to turn it off (make it default behaviour, but optional), or force enable it ONLY for certain networks that really need it.

So my question is:

Thanks!

Steps to reproduce

  1. start an evm network with ETH RPC locally
  2. connect MM to localhost, try send erc20
  3. local RPC logs show that eth_estimateGas returns X
  4. on MM UI we can see X * 1.5 as gasLimit

Error messages or log output

No response

Version

10.30.4

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

anaamolnar commented 1 year ago

Hello, @shunjizhan. Thanks for reporting! I will pass this on to the team.

shunjizhan commented 1 year ago

thank you!

shunjizhan commented 1 year ago

From the codebase I do see this feature disabled for optimism.

I think it's better to be "enabled only to certain networks", instead of current "enabled globally, but only disabled for specific networks".

bschorchit commented 1 year ago

Thank you for reporting and for your investigation into this. We'll investigate wether this buffer is still needed (it has been introduced very early on when estimations were less accurate) and if not needed, we'll communicate it and remove it across networks. If it's still needed, we'll research a permissionless way for networks to workaround/opt out of this feature.