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
contracts-bedrock: ecotone gas config support #10226
Conversation
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
71055ac
to
8a685a9
Compare
WalkthroughWalkthroughThe recent updates involve significant changes across various files to transition away from deprecated fields Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional comments not posted (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
@sebastianst I have addressed all open comments on this PR |
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.
LGTM, a few non-blocking possible improvements and cleanups
787b4dd
to
e4b0d84
Compare
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.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
packages/contracts-bedrock/src/L1/SystemConfig.sol (2)
88-88
: Deprecated comment should be updated to reflect the new context.Consider updating the comment to clarify that
overhead
is deprecated and should not be used in new configurations.
92-93
: Clarify the usage of the most significant byte inscalar
.The comment could be expanded to explain how the most significant byte is used to determine the version. This will help maintainers understand the versioning mechanism better.
Seeing failures in the |
8162dce
to
24f217e
Compare
I have rebased |
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.
Actionable comments posted: 1
8fb8a56
to
78ee64e
Compare
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.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
packages/contracts-bedrock/src/L1/SystemConfig.sol (2)
93-93
: Deprecatedoverhead
should be clearly documented.Consider adding a
@deprecated
tag in the comment foroverhead
to clearly indicate its deprecation status in the documentation.
97-98
: Clarify versioning inscalar
documentation.The comment on
scalar
mentions versioning but does not explain how versions are determined. Consider expanding this comment to include details on how versions are encoded and decoded.packages/contracts-bedrock/test/L1/SystemConfig.t.sol (1)
234-235
: Use realistic values for basefee and blobbasefee scalars in tests.Consider using more realistic values for
basefeeScalar
andblobbasefeeScalar
in thecleanStorageAndInit
function to better reflect production scenarios.
78ee64e
to
69dc585
Compare
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.
Actionable comments posted: 3
Out of diff range and nitpick comments (8)
op-chain-ops/upgrades/l1.go (8)
Line range hint
2-2
: Ensure consistent package naming conventions.The package name
upgrades
is very generic. Consider renaming it to something more specific to its functionality, likecontractUpgrades
orl1Upgrades
, to improve clarity and avoid potential conflicts.
Line range hint
10-10
: Consider using a more descriptive variable name.The variable
method
is used to represent the method namesetBytes32
. A more descriptive name likestorageSetterMethod
could improve readability and maintainability.
Line range hint
18-18
: Consider handling potential nil pointer dereferences.In the function
L1
, there is no null check for the parametersconfig
andchainConfig
before they are used. This could lead to runtime panics ifnil
is passed. Consider adding checks and returning an appropriate error if they arenil
.
Line range hint
20-20
: Optimize error handling for better clarity.The error messages in the
L1
function could be more descriptive. Instead of just stating the contract name, include more context about what specifically failed, such as the operation being performed.
Line range hint
22-22
: Ensure error handling consistency.The function
L1CrossDomainMessenger
and other similar functions do not handle errors consistently. Some errors are returned directly while others are wrapped with additional context. It's important to maintain consistency in how errors are handled and reported.
Line range hint
24-24
: Avoid hard-coded values in production code.The use of hard-coded addresses and values, such as in the
storageSetterAddr
, can lead to maintenance issues and less flexibility. Consider fetching these values from a configuration file or environment variables.
Line range hint
28-28
: Ensure proper resource cleanup and error handling.In the
SystemConfig
function, there is a potential resource leak wherestorageSetterABI
is fetched but not used in some error paths. Ensure that all resources are properly released or handled in all code paths.
Line range hint
30-30
: Consider adding more comprehensive logging.The functions lack detailed logging which could help in debugging issues during the upgrade process. Consider adding detailed logs that capture key steps and variable values.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #10226 +/- ##
============================================
- Coverage 42.36% 29.22% -13.15%
============================================
Files 73 31 -42
Lines 4836 2898 -1938
Branches 766 614 -152
============================================
- Hits 2049 847 -1202
+ Misses 2680 1976 -704
+ Partials 107 75 -32
Flags with carried forward coverage won't be shown. Click here to find out more. |
673f1b9
to
1ceb1ba
Compare
I have addressed merge conflicts with a rebase |
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/contracts-bedrock/test/kontrol/proofs/utils/DeploymentSummaryFaultProofs.sol (1)
Line range hint
248-403
: Review the extensive use ofvm.store
for potential optimization.The function
recreateDeployment
uses multiplevm.store
calls with similar patterns. Consider refactoring to reduce redundancy and improve maintainability, possibly by creating helper functions or loops for repetitive tasks.
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.
Actionable comments posted: 4
Outside diff range and nitpick comments (2)
packages/contracts-bedrock/test/L1/SystemConfig.t.sol (2)
34-42
: Ensure consistent use ofbasefeeScalar
andblobbasefeeScalar
across all tests.The setup function initializes
basefeeScalar
andblobbasefeeScalar
which are used across various tests. It's crucial to ensure that these values are consistently used in all relevant test cases to maintain the integrity of the tests.
290-291
: Review the use of default scalar values in resource configuration tests.In the
_initializeWithResourceConfig
function, the use of default scalar values (_basefeeScalar: 0, _blobbasefeeScalar: 0
) might not fully test the behavior under realistic configurations. Consider using more representative values to enhance the test's effectiveness.
Description
Updates the gas config on the
SystemConfig
to be ecotone first.This resolves chain operator ux issues with setting the gas config,
rendering the additional tooling for calculating the encoded
scalar version obsolete. This version of the system config
is technically backwards compatible with pre-ecotone, but it
requires a follow up tx to call the old
setGasConfig
method.The old gas config method was updated such that it cannot
be used to set ecotone style config to prevent footguns.
A new method
setGasConfigEcotone
should be used instead.