Skip to content

l2geth: gaslimit encoding state polling#927

Merged
tynes merged 4 commits intofeat/gaslimit-encodingfrom
feat/gaslimit-encoding-state-polling
May 20, 2021
Merged

l2geth: gaslimit encoding state polling#927
tynes merged 4 commits intofeat/gaslimit-encodingfrom
feat/gaslimit-encoding-state-polling

Conversation

@tynes
Copy link
Copy Markdown
Contributor

@tynes tynes commented May 20, 2021

Description
This adds in L2 state polling in the sequence and verify loops to pull out the L2 gas price that is set inside of the OVM_GasPriceOracle being worked on here: #912

This is fully backwards compatible with the config option EnableL2GasPolling as it defaults to false. The config option GasPriceOracleAddress will set the address that it reads from, this should default to the 0x42.. address that the contract lives at. Having this address be configurable will let us deploy it to L2 and then update the network with the correct address configured and have it start working. Note that infrastructure providers will also need to reconfigure with the address that the contract ends up being deployed to.

Additional context
This is opened up as a PR to #906 to prevent it from growing too large without review

This still needs config parsing and to pass through the config options to the rollup.Config

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented May 20, 2021

⚠️ No Changeset found

Latest commit: 4fc9d65

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

@tynes tynes requested a review from gakonst May 20, 2021 05:22
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 20, 2021

Codecov Report

Merging #927 (f420f44) into feat/gaslimit-encoding (d09e81c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           feat/gaslimit-encoding     #927   +/-   ##
=======================================================
  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 d09e81c...f420f44. Read the comment docs.

Copy link
Copy Markdown
Contributor

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

This change LGTM. Basically updating the RollupGPO price with whatever's in the state once every polling interval. Would we not also need to store the data price somewhere?

@tynes
Copy link
Copy Markdown
Contributor Author

tynes commented May 20, 2021

I don't think we need to store the data price in the L2 state since we can just poll it from L1. There aren't PGAs that users can participate in by upping their fee so its in their best interest to just use whatever is returned. This is under the assumption that replicas will be able to estimate L1 gas prices the same as the sequencer, but this may not be the case depending on the version of the L1 node software used, see https://eth-gas.chainsafe.io/ how different nodes can have a different view of the network

@tynes tynes merged commit 7b1801e into feat/gaslimit-encoding May 20, 2021
@tynes tynes deleted the feat/gaslimit-encoding-state-polling branch May 20, 2021 20:21
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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants