-
Notifications
You must be signed in to change notification settings - Fork 1
CircleCI: fix op-program-compat #40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like upstream doesn't have such changes, why is it not an issue for them? |
Because the |
|
|
||
| isOsaka := config.IsOsaka(config.LondonBlock, headTimestamp) | ||
| bcfg := latestBlobConfig(config, headTimestamp) | ||
| if bcfg == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to distinguish whether L2Blob is enabled. If enabled, bcfg must be non-nil.
| blobConfig := latestBlobConfig(config, header.Time) | ||
| if blobConfig == nil { | ||
| panic("calculating blob fee on unsupported fork") | ||
| return minBlobGasPrice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to distinguish whether L2Blob is enabled. If enabled, we should panic here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth checking here that L2 blob is not enabled. We don't necessarily need to modify latestBlobConfig above. Just ensure that these hardcoded values only works for compatibility with upstream, but we don't actually use these hardcoded values.
Fix CircleCI error in op-program-compat
Can be verified locally on the optimism repo: