Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
shunjizhan opened this issue May 27, 2023 · 4 comments
Open

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

shunjizhan opened this issue May 27, 2023 · 4 comments
Labels
Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations-system DEPRECATED: please use "team-confirmations" label instead team-transactions Transactions team type-enhancement

Comments

@shunjizhan
Copy link

shunjizhan commented May 27, 2023

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". #8771 (comment)

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:

  • is there a way to turn this feature off? If not, it's a feature request.
  • can someone please clarify when does MM auto increase gasLimit by 50%? Is it only for token transfer, or is it for all contract calls? If it's only for token transfers, I can probably do some workaround in our network, such as "divide gasLimit by 1.5 before eth_estimateGas return when sending erc20"

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 anaamolnar added Sev2-normal Normal severity; minor loss of service or inconvenience. team-wallet-api-platform labels May 29, 2023
@anaamolnar
Copy link

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

@anaamolnar anaamolnar added team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead type-enhancement Sev3-low Low severity; minimal to no impact upon users and removed team-wallet-api-platform Sev2-normal Normal severity; minor loss of service or inconvenience. labels May 29, 2023
@shunjizhan
Copy link
Author

thank you!

@shunjizhan
Copy link
Author

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".

@anaamolnar anaamolnar removed Sev3-low Low severity; minimal to no impact upon users type-bug labels May 30, 2023
@bschorchit bschorchit added team-confirmations-planning (only for internal use within Confirmations team) and removed team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Jun 5, 2023
@bschorchit
Copy link

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.

@bschorchit bschorchit added the Sev2-normal Normal severity; minor loss of service or inconvenience. label Aug 29, 2023
@bschorchit bschorchit added the team-confirmations-system DEPRECATED: please use "team-confirmations" label instead label Jan 23, 2024
@cryptotavares cryptotavares added the team-confirmations Push issues to confirmations team label Apr 24, 2024
@bschorchit bschorchit added team-transactions Transactions team and removed team-confirmations-planning (only for internal use within Confirmations team) team-confirmations Push issues to confirmations team labels May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations-system DEPRECATED: please use "team-confirmations" label instead team-transactions Transactions team type-enhancement
Projects
None yet
Development

No branches or pull requests

6 participants
@cryptotavares @shunjizhan @bschorchit @anaamolnar and others