feat[geth]: add gas pricer for congestion fees #1103
Conversation
🦋 Changeset detectedLatest commit: c718bcd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
karlfloersch
force-pushed
the
feat/gas-pricer
branch
from
102dced to
cc7e6d7
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1103 +/- ##
========================================
Coverage 86.05% 86.05%
========================================
Files 49 49
Lines 1936 1936
Branches 307 307
========================================
Hits 1666 1666
Misses 270 270
Continue to review full report at Codecov.
|
|
Good catch @tynes updated it! |
|
@tynes sorry I didn't notice that you had integrated the gas price oracle service into this PR. That's my bad. Do you think it makes sense to split up these PRs so we can get the business logic reviewed first and then get the service reviewed? |
I can split up the PRs, basing it on the same branch was a lot easier to get it working with your changes. My main concern about doing the split of the code is that there are loglines in the library itself which is kind of an antipattern. This means that we could work on better abstractions for the service to consume. All of the loglines should be happening in the service itself. Edit: @karlfloersch I'd prefer to not break this up unless part of breaking it up involved refactoring the interface such that we didn't need to have loglines inside of the library itself |
|
So now that I see we are in the rebase and merge workflow, I will force push this with a clean git history |
There was a problem hiding this comment.
This is awesome. I left a bunch of comments all over the place, most important notes were around the config.
That said, I love the way you tested this API -- the logic inside start & loop are boilerplate, so it feels less critical to add intensive testing there, but the core logic which connects the GasPriceUpdater/GasPricer to the chain are nicely tested. I have high hopes for this package being quite stable!
Can anyone think of other high leverage unit tests that we can add? I really want to hone our abilities to write testable code & I think this service is a good example of a step in that direction. Very curious if there are more tests folks can think of!
go/gas-oracle/README.md
Outdated
| --transaction-gas-price value (default: 0) [$GAS_PRICE_ORACLE_TRANSACTION_GAS_PRICE] | ||
| --loglevel value log level to emit to the screen (default: 3) | ||
| --floor-price value gas price floor (default: 0) | ||
| --target-gas-per-second value target gas per second (default: 0) |
There was a problem hiding this comment.
This default value is not something that can be used in practice. Maybe just make this a required flag?
There was a problem hiding this comment.
If we do not specify a default then it prints the default as the zero value, https://tour.golang.org/basics/12
go/gas-oracle/README.md
Outdated
| --target-gas-per-second value target gas per second (default: 0) | ||
| --max-percent-change-per-epoch value max percent change of gas price per second (default: 0) | ||
| --average-block-gas-limit-per-epoch value average block gas limit per epoch (default: 1.1e+07) | ||
| --epoch-length-seconds value length of epochs in seconds (default: 0) |
There was a problem hiding this comment.
Maybe make required or set to a reasonable value, maybe 30?
There was a problem hiding this comment.
I didn't see this until after I updated it to 10. I'm happy to update all of them in one go to reasonable values if you know what the most reasonable values are for them
| @@ -0,0 +1,99 @@ | |||
| { | |||
There was a problem hiding this comment.
Make symbolic link to the contracts
There was a problem hiding this comment.
I'm not going to make this a symbolic link because changes could cause the program to not compile and the binding is committed into the repo and building it is not part of the build step so if this is a symlink then it can get out of date with the binding. Updating it should be explicit and the only time it should change in the near future is for #1190
| log.Trace("fetched L2 tx.gasPrice", "gas-price", gasPrice) | ||
| opts.GasPrice = gasPrice | ||
| } else { | ||
| // Allow a configurable gas price to be set |
There was a problem hiding this comment.
TODO: add a polling cache to the sync service that allows through 0 gas price txs iff it is the owner of the OVM_GasPriceOracle
tynes
force-pushed
the
feat/gas-pricer
branch
5 times, most recently
from
55a2f69 to
7922229
Compare
tynes
force-pushed
the
feat/gas-pricer
branch
2 times, most recently
from
569e734 to
3839d6a
Compare
The `gasprices` package implements the logic that updates L2 gasprices on the Optimistic Ethereum Network. Co-authored-by: Karl Floersch <[email protected]>
This commit adds the `gas-oracle` which is an offchain entity that sends transactions to L2 to update the gas price. It must be configured with a private key as the `OVM_GasPriceOracle` is owned. The `gas-oracle` is added to the changesets setup. Tests are included as well as CI. Dockerizing will happen in a follow up PR.
tynes
force-pushed
the
feat/gas-pricer
branch
3 times, most recently
from
552ef46 to
c718bcd
Compare
Adds a dockerfile for the `gas-oracle` as well as adding it as a service in the `docker-compose.yaml`. It is not enabled by default due to memory issues in CI already happening occasionally where the integration tests are oom killed. The `gas-oracle` is configured with a key that owns the `OVM_GasPriceOracle`. This PR adds the `gas-oracle` to the Github Actions workflow that is responsible for publishing the docker images.
Description
Adds a new Go package to the monorepo,
go/gas-oraclethat includes the gas pricing algorithms in thegaspricespackage and a service to automatically update gas prices in theoraclepackage.The
gaspricespackage has unit tests written by @karlfloersch and theoraclepackage has unit tests written by @tynes. These tests run in CI and the code is also linted in CI with 2 new Github workflows added.The
main.gofile is the entrypoint to thegas-oracleservice, that polls the sequencer to see how much gas is being used and then it sends transactions to the sequencer to update the L2 gas price. This means that the service must be configured with theOVM_GasPriceOracleOwner key. As gas is used over time, the algorithms in thegaspricespackage will return values for thegas-oracleto use when updating the gas price.It will only update the L2 gas price when the change is significant enough - ie only update the gas price if it changes more than 5%, this is to save the need to send additional transactions to the chain. The significance factor can be configured via CLI. All config options are documented in the
README.md.A follow up PR will add a docker build/tag/publish Github workflow
Fixes OP-874