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

Sending eth to contract fallback fails with default gas #7115

Closed
miguelmota opened this issue Sep 5, 2019 · 6 comments · Fixed by #7252
Closed

Sending eth to contract fallback fails with default gas #7115

miguelmota opened this issue Sep 5, 2019 · 6 comments · Fixed by #7252
Assignees

Comments

@miguelmota
Copy link

miguelmota commented Sep 5, 2019

MetaMask is setting the gas limit of 21000 by default for eth sends even if it's to a contract which will always fail. An improvement would be to add additional gas padding if the the receiving address is a contract.

Example failed tx

@jennypollack
Copy link
Contributor

+1
I started noticing this recently on gnosis branch but it doesn't happen every time
if we detect a contract address that is a valid recipient we should do an estimate_gas call instead of using the gas defaults set in the frontend and the gasLimitSpecified flag

@rstormsf
Copy link

rstormsf commented Oct 3, 2019

@danfinlay
100% agree. Metamask could make 1 extra call to see if it's an account or contract

const bytecode = await web3.eth.getCode('REGULAR_ADDRESS')
console.log(bytecode) // returns 0x for regular accounts

or always try to call estimate_gas regardless if it's contract or not
contracts with fallbacks needs a bit more gas than 21000

@danfinlay
Copy link
Contributor

Yeah that's what I thought we did, so this is definitely a bug in the meanwhile. Avoiding estimate_gas when not a contract was one strategy for improving time to estimate gas, which I guess is a little surprising, since you'd think any node would be quick to estimate 21000 when it was not a contract and a simple ether send.

@danfinlay
Copy link
Contributor

Oh man, this bug is so bad now blog posts are talking about how to educate users around it. We need this fixed soon. This must have been introduced recently, there's no way this has been sitting around.
https://medium.com/wearekickback/integrating-a-contract-based-wallet-into-your-dapp-1721c1a1148b

@danfinlay
Copy link
Contributor

Here’s the line that claims to be doing this. Does look like a regression.
https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/controllers/transactions/tx-gas-utils.js

@robertmagier
Copy link

It seems that issues is back in version 7.7.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants