Add L1 gas cost estimation feature to L1 gas oracle #11812
Conversation
|
I see that you haven't updated any README files. Would it make sense to do so? |
…ptimism, and Scroll
amit-momin
force-pushed
the
feature/l1-gas-cost-estimation
branch
from
b88192f to
3c38d6f
Compare
amit-momin
requested review from
samsondav and
prashantkumar1982
as code owners
amit-momin
requested review from
FelixFan1992 and
dimriou
and removed request for
samsondav and
prashantkumar1982
| precompile := common.HexToAddress(o.l1GasPriceAddress) | ||
| callData, err = o.l1GasPriceMethodAbi.Pack(o.gasPriceMethod) | ||
| if err != nil { | ||
| o.logger.Errorf("failed to pack calldata for %s L1 gas price method: %w", o.chainType, err) |
There was a problem hiding this comment.
I know this behavior is same as previously.
But in these cases of errors, could we set them somewhere, and let it be reportable via HealthReport()?
Currently I see HealthReport always reports Healthy.
There was a problem hiding this comment.
I updated the code to append these errors to the SvcErrBuffer which should return all of the errors joined together when Healthy() is called.
| PollPeriod = 12 * time.Second | ||
|
|
||
| // RPC call timeout | ||
| queryTimeout = 10 * time.Second |
There was a problem hiding this comment.
Not sure, but is it possible to use a configured RPC timeout like other components are doing?
There was a problem hiding this comment.
Ideally let's reuse something that already exists
There was a problem hiding this comment.
I tried to look through existing configs and didn't find one for RPC timeouts. Even in chains/evm/client, we have 10s hard coded which is used by ContextWithDefaultTimeout. It makes sense for this to be configurable but I think I'd have to add a new config for this.
There was a problem hiding this comment.
Actually I see RequestTimeout for Starknet which I could maybe leverage. But it may be misleading if the rest of our code isn't using this config for RPC timeouts.
There was a problem hiding this comment.
Yeah, QueryTimeout was the config I was thinking of. You can probably use evmclient.ContextWithDefaultTimeout() here instead of having a new config and let the client handle the timeout.
There was a problem hiding this comment.
I updated the code to use the client.QueryTimeout. I was avoiding using ContextWithDefaultTimeout since it just uses context.Background() under the hood and we have a ctx passed into the method we could use.
| KromaGasOracle_l1BaseFee = "l1BaseFee" | ||
|
|
||
| // Interval at which to poll for L1BaseFee. A good starting point is the L1 block time. | ||
| PollPeriod = 12 * time.Second |
There was a problem hiding this comment.
I know this is same as previously.
But 12 seconds could sometimes mean our price estimates can be 11-12 seconds stale.
Could we make this a configurable setting, or if not, atleast change the default to 6 seconds?
There was a problem hiding this comment.
| } | ||
| } | ||
|
|
||
| func (o *l1Oracle) refresh() (t *time.Timer) { |
There was a problem hiding this comment.
Maybe for a future PR.
But we ought to have some logic to start giving errors to all methods from the l1Oracle, if we fail to fetch fresh prices for last 12-20 seconds.
Otherwise with current logic, we keep using stale prices, which may be worse behavior for applications.
Perhaps if we just report errors in such cases, it may be better.
There was a problem hiding this comment.
I added tracking for when the price was refreshed so we can validate for stale prices on read. Is this what you had in mind?
There was a problem hiding this comment.
Is there a link where we can find these ABIs? It would be nice to add it here.
There was a problem hiding this comment.
Good idea. I just got them from arbiscan and optimistic.etherscan but I can link those in the comments.
|
SonarQube Quality Gate
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Added the ability to retrieve L1 gas cost estimation from the L1 gas oracle for Arbitrum, Optimism, and Scroll