Update L1 gas price calculations for Optimism Ecotone and Fjord upgrades #13661
Conversation
amit-momin
force-pushed
the
BCI-3516-Update-Optimism-L1-Oracle-for-Fjord-Hard-Fork
branch
from
e4e8072 to
492c6f7
Compare
There was a problem hiding this comment.
Moving the errors inside the constructor seems like an anti-pattern and perhaps it would be better if we did the initialization during Start. I guess for now we can leave it as it is, it's no big deal.
| OPStackGasOracle_l1BaseFee = "l1BaseFee" | ||
| // OPStackGasOracle_getL1Fee fetches the l1 fee for given tx bytes | ||
| // OPStackGasOracle_getL1Fee is a hex encoded call to: | ||
| upgradePollingPeriod = 14400 |
There was a problem hiding this comment.
nit: perhaps we could change this to a more meaningful 4 * time.Hour type and use builtin time.Since method when checking for last update. Same goes for upgradeCheckTs. It's better if we change it to: upgradeCheckTs time.Time
| // OPStackGasOracle_getL1GasUsed fetches the l1 gas used for given tx bytes | ||
| OPStackGasOracle_getL1GasUsed = "getL1GasUsed" | ||
| // OPStackGasOracle_isEcotonePollingPeriod is the interval to poll if chain has upgraded to Ecotone | ||
| // isUpgradedPollingPeriod is the interval to poll if chain has been upgraded |
There was a problem hiding this comment.
nit outdated comment?
There was a problem hiding this comment.
I've updated it in the latest commit
| func (o *optimismL1Oracle) checkForUpgrade(ctx context.Context) error { | ||
| // if chain is already Fjord (the latest upgrade), NOOP | ||
| // need to continue to check if not on latest upgrade | ||
| if o.isFjord { |
There was a problem hiding this comment.
Don't we want to check for both?
| if o.isFjord { | |
| if o.isFjord || o.isEcotone { |
There was a problem hiding this comment.
If we checked for both, we'd exit out early if the Ecotone upgrade is live. We wouldn't want to do this since we'd stop checking for the Fjord upgrade which is after. So we only want to exit early if the latest upgrade has already occurred.
There was a problem hiding this comment.
But Ecotone and Fjord have the same calculation formula, don't they?
There was a problem hiding this comment.
In that case, I don't think we'd even need the isFjord flag. We would never make the RPC call to update it with this change except during startup after the upgrade. My worry would be if we later need to add some Fjord specific logic this would not be as obvious for whoever makes the change if they trust checkForUpgrade to work as expected.
|
|
||
| l1GasUsed := new(big.Int).SetBytes(l1GasUsedBytes) | ||
| l1Fee := new(big.Int).SetBytes(l1FeeBytes) | ||
| // Gas price = scaled gas price / (16 * 10 ^ decimals) |
There was a problem hiding this comment.
nit: Can you add links for both Ecotone and Fjord that will verify this formula?
There was a problem hiding this comment.
I've added the links and some context in the latest commit
…timism-L1-Oracle-for-Fjord-Hard-Fork
|
dimriou
deleted the
BCI-3516-Update-Optimism-L1-Oracle-for-Fjord-Hard-Fork
branch
Description
Updates the L1 gas price calculations in our Optimism L1 oracle component for the Ecotone and Fjord upgrade. The Fjord upgrade deprecates the
getL1GasUsedmethod that is currently being used. It was identified that the gas price can be calculated directly without relying on this method for both Ecotone and Fjord. It now uses the following formula:(l1BaseFee * 16 * baseFeeScalar + blobBaseFee * blobBaseFeeScalar) / (16 * 10 ^ decimals)Ticket
https://smartcontract-it.atlassian.net/browse/BCI-3516