Skip to content

fix(l2geth): do not throw an error when estimating gas for txs with nil data#940

Merged
gakonst merged 2 commits intodevelopfrom
fix/eth_estimateGas
May 24, 2021
Merged

fix(l2geth): do not throw an error when estimating gas for txs with nil data#940
gakonst merged 2 commits intodevelopfrom
fix/eth_estimateGas

Conversation

@gakonst
Copy link
Copy Markdown
Contributor

@gakonst gakonst commented May 23, 2021

Fixes #930

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented May 23, 2021

🦋 Changeset detected

Latest commit: f347c34

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@eth-optimism/l2geth Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #940 (f347c34) into develop (ef2fba1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #940   +/-   ##
========================================
  Coverage    82.21%   82.21%           
========================================
  Files           48       48           
  Lines         1895     1895           
  Branches       303      303           
========================================
  Hits          1558     1558           
  Misses         337      337           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef2fba1...f347c34. Read the comment docs.

to: DEFAULT_TRANSACTION.to,
value: 0,
})
expect(estimate).to.be.eq(21000)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do transactions with empty data always return a gas estimate of 21000? Why? Can this be abused?

Copy link
Copy Markdown
Contributor Author

@gakonst gakonst May 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was implemented in #695, it's not unique to empty data txs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to never let the value be lower than 21000 because that breaks an expectation that Metamask had. With #906 that line is removed

@gakonst gakonst merged commit c880043 into develop May 24, 2021
@gakonst gakonst deleted the fix/eth_estimateGas branch May 24, 2021 18:10
InoMurko pushed a commit to omgnetwork/optimism that referenced this pull request May 25, 2021
…il data (ethereum-optimism#940)

* fix(l2geth): do not throw an error when estimating gas for txs with nil data

* chore: add changeset
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.

cannot estimateGas on ETH value sends

5 participants