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

api: Add block param to eth_estimateGas #11462

Merged
merged 4 commits into from
Nov 29, 2023
Merged

api: Add block param to eth_estimateGas #11462

merged 4 commits into from
Nov 29, 2023

Conversation

fridrik01
Copy link
Contributor

@fridrik01 fridrik01 commented Nov 28, 2023

Related Issues

#11411

Proposed Changes

Geth implementation has added paramters to eth_estimateGas RPC endpoint and from reports from users on slack (thread1, thread2, thread3) it seems that Remix has been updated to use those extra parameters, causing RPC errors in Lotus since it does not recognize these parameters.

This PR adds support for a second ethtypes.EthBlockNumberOrHash parameter which if set will use the block specified when calculating estimateGas. Note that Geth supports an optional 3rd parameter which this PR does not implement as supporting that would require considerable effort and is left for future updates in case its needed.

Additional Info

Test Plan

  • I compiled lotus on calibnet, downloaded and imported calibnet snapshots
  • Started lotus
  • Switched Metamask to use my local lotus node
  • Went to https://www.glif.io/ and connected my wallet, then translated my address into a "t address"
  • Deposited some tFIL to my MM address using https://faucet.calibration.fildev.network/funds.html and using that "t address"
  • Went to https://remix.ethereum.org/ and set Metamask as the provider
  • Set a breakpoint in EthEstimateGas in lotus and attached my vscode to the running lotus instance
  • Deployed the contract from remix
  • Stepped through the EthEstimateGas in the debugger and observed that it went through successfully
  • Observed success from remix in deploying the contract (see screenshot):

image

  • Also tested deploying a contract using the "Compile and Run script" in the "Solidity Compiler" tab in Remix UI which apparently has not been updated with the extra param as it only sends one parameter. Confirmed that it successfully deployed the contract as well.

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@fridrik01 fridrik01 marked this pull request as ready for review November 28, 2023 17:34
@fridrik01 fridrik01 requested a review from a team as a code owner November 28, 2023 17:34
CHANGELOG.md Outdated Show resolved Hide resolved
gateway/proxy_eth.go Outdated Show resolved Hide resolved
@fridrik01 fridrik01 marked this pull request as draft November 28, 2023 19:43
@fridrik01
Copy link
Contributor Author

Moving back to draft until I make changes to support both 1 and 2 params

@fridrik01 fridrik01 marked this pull request as ready for review November 29, 2023 10:52
@fridrik01 fridrik01 requested a review from a team November 29, 2023 10:53
@Stebalien
Copy link
Member

Can we add a test or two by specifying a block param in a normal test.

@fridrik01 fridrik01 merged commit 813d133 into master Nov 29, 2023
86 of 87 checks passed
@fridrik01 fridrik01 deleted the update-estimate-gas branch November 29, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants