search

livepeer / livepeer-monorepo

JavaScript tools and applications that interact with Livepeer's smart contracts and peer-to-peer network
https://livepeer.org
MIT License
167 stars 71 forks source link

remove manual gas estimation in explorer #1010

Closed adamsoffer closed 3 years ago

adamsoffer commented 3 years ago

What does this pull request do? Explain your changes. (required) This PR removes the explorer's manual gas estimation in favor of the gas estimation provided by a user's ethereum wallet. The reason for this change is two fold:

  1. The eth_estimateGas RPC call is changing in the upcoming London hardfork tomorrow and will no longer work if the new 1559 gas pricing isn't filled in.
  2. Ethereum wallets are typically more capable when it comes to gas estimation

How did you test each of these updates Tested affected transactions on Rinkeby

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

explorer-mainnet – ./packages/explorer-2.0

🔍 Inspect: https://vercel.com/livepeerorg/explorer-mainnet/Ao2patdRuXyrxKhNQvnfLysJ7Fq1
✅ Preview: https://explorer-mainnet-git-as-eip-updates-livepeerorg.vercel.app

explorer-rinkeby – ./packages/explorer-2.0

🔍 Inspect: https://vercel.com/livepeerorg/explorer-rinkeby/4B35JjeStssEqXyXGy4x2DseAQfC
✅ Preview: https://explorer-rinkeby-git-as-eip-updates-livepeerorg.vercel.app

yondonfu commented 3 years ago

The change look good in that removing the gas estimation calls will avoid any breakage caused by missing newly required arguments, but...

Would this change mean that Metamask may overestimate the transaction fee again which was addressed in https://github.com/livepeer/livepeerjs/pull/955 by relying on a manual gas estimation instead of Metamask's gas estimation?

adamsoffer commented 3 years ago

The change look good in that removing the gas estimation calls will avoid any breakage caused by missing newly required arguments, but...

Would this change mean that Metamask may overestimate the transaction fee again which was addressed in #955 by relying on a manual gas estimation instead of Metamask's gas estimation?

Ah, yeah thanks for bringing that up. Yeah, that might be the case. Although, I'm wondering if the issue of Metamask overestimating will still be the case post hardfork. Does the way by which gas is estimated change after the hardfork? If you anticipate overestimation will still be an issue, I can revert and add the extra argument to eth_estimateGas.

yondonfu commented 3 years ago

Although, I'm wondering if the issue of Metamask overestimating will still be the case post hardfork.

If Metamask was previously or currently returning a higher gas estimate than a manual eth_estimateGas call then I'd guess that its because Metamask adds a small buffer to their gas estimate as a safety precaution to ensure that txs that consume a variable amount of gas depending on the state of a contract don't run out of gas.

Does the way by which gas is estimated change after the hardfork?

Nope the actual gas estimation algorithm used by eth_estimateGas isn't changing.

If you anticipate overestimation will still be an issue, I can revert and add the extra argument to eth_estimateGas.

I'm comfortable with merging this PR as-is to make sure that the manual gas estimation logic doesn't break after the hardfork due to missing args especially given how soon the hardfork is happening.

I do think it'd be worthwhile to do another gas estimate comparison between Metamask and a manual eth_estimateGas call. If there is a substantial difference then we can consider populating the necessary args for manual eth_estimateGas calls similar to what MyCrypto does here.

adamsoffer commented 3 years ago

I do think it'd be worthwhile to do another gas estimate comparison between Metamask and a manual eth_estimateGas call. If there is a substantial difference then we can consider populating the necessary args for manual eth_estimateGas calls similar to what MyCrypto does here.

Cool, I'll merge then do a gas estimate comparison after the hardfork and see what the difference is.

adamsoffer commented 3 years ago

Just tested again now that London is live and metamask indeed still overestimates with ~15% more gas. I'll create a followup PR that adds back manual estimation and will use that MyCrypto link as a reference for the new eth_estimateGas call.