Skip to content

feat: gaslimit encoding end to end#932

Merged
tynes merged 22 commits intofeat/gaslimit-encodingfrom
feat/gaslimit-encoding-end-to-end
May 24, 2021
Merged

feat: gaslimit encoding end to end#932
tynes merged 22 commits intofeat/gaslimit-encodingfrom
feat/gaslimit-encoding-end-to-end

Conversation

@tynes
Copy link
Copy Markdown
Contributor

@tynes tynes commented May 21, 2021

Description
This PR included end to end testing of the gasLimit encoding scheme described in #823 (comment)

A JS fees library was written that implements the scheme and is added to core-utils. It could use some more tests but currently passes the end to end tests. Will follow up with more tests as well as #823 (comment)

It has some additional implementation details, for example handling gasPrices set to 0

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented May 21, 2021

⚠️ No Changeset found

Latest commit: 6586690

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added 2-reviewers A-pkg-core-utils Area: packages/core-utils A-ops Area: ops labels May 21, 2021
@tynes tynes requested a review from maurelian as a code owner May 21, 2021 00:43
l2GasLimit: BigNumber,
l2GasPrice: BigNumber
): BigNumber {
const l1GasLimit = calculateL1GasLimit(data)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should throw an error if the values do not satisfy the correct equations, see #823 (comment)

}
expect(estimate).to.be.deep.eq(expected)
const estimate = await l2Provider.estimateGas(tx)
const l2Gaslimit = await l2Provider.send('eth_estimateExecutionGas', [
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This new RPC endpoint was added to be exactly what the L1 eth_estimateGas is

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 21, 2021

Codecov Report

Merging #932 (4fddd33) into feat/gaslimit-encoding (4c4df82) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           feat/gaslimit-encoding     #932   +/-   ##
=======================================================
  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 4c4df82...4fddd33. Read the comment docs.

@@ -0,0 +1,19 @@
import './setup'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This needs more tests but the e2e tests are passing using encode and decode

@tynes
Copy link
Copy Markdown
Contributor Author

tynes commented May 22, 2021

This implementation needs to use big.Ints for handling gas prices in geth

return err
}

if !s.enforceFees && tx.GasPrice().Cmp(common.Big0) == 0 {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should be able to be moved to the top

@tynes
Copy link
Copy Markdown
Contributor Author

tynes commented May 24, 2021

I have more commits coming for this branch soon

@smartcontracts smartcontracts marked this pull request as draft May 24, 2021 19:28
func RoundL2GasPrice(gasPrice uint64) uint64 {
if gasPrice == 0 {
return gasPrice
// VerifyL2GasPrice returns an error if the number is an invalid possible L2 gas
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

big.Ints are too verbose but we have to use them because we need the arbitrary precision

@tynes tynes marked this pull request as ready for review May 24, 2021 22:58
@tynes tynes merged commit ec21e03 into feat/gaslimit-encoding May 24, 2021
@tynes tynes deleted the feat/gaslimit-encoding-end-to-end branch May 24, 2021 22:59
tynes added a commit that referenced this pull request May 26, 2021
* l2geth: updated calculate rollup fee

* l2geth: implement the gasprice serialization

* lint: fix

* rollup-test: fix

* l2geth: gaslimit encoding state polling (#927)

* l2geth: rollup gas price oracle state polling

* l2geth: read l2 gasprice from the tip

* l2geth: add config options for L2 gas price

* l2geth: comment to remove code in future

* l2geth: handle 0 and 1 fees

* l2geth: enable 0 and 1 fee tests

* feat: gaslimit encoding end to end (#932)

* core-utils: add fees package

* integration-tests: refactor for new fees

* l2geth: end to end fee spec

* l2geth: use new env var

* deps: regenerate

* lint: fix

* l2geth: fee verification

* tests: update gas prices

* tests: update gas prices

* tests: cleanup

* l2geth: small cleanup

* l2geth: fix max

* feat: fix fee calculations with bigints

* tests: fix

* tests: lint

* core-utils: rename fees to L2GasLimit

* l2geth: fix comment

* l2geth: fix name of env var

* l2geth: delete extra print statement

* l2geth: fix logline

* tests: fix typo

* l2geth: improve readability

* chore: add changeset

* l2geth: fix compiler error

* feat: clean up and fix fees

* lint: fix

* core-utils: refactor api to be more friendly

* lint: fix

* comments: fix

* refactor: clean up style and common language

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ops Area: ops A-pkg-core-utils Area: packages/core-utils

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants