refactor(gas-oracle): remove outdated logic #1560
Conversation
colinlyguo
requested review from
zimpha,
georgehao,
Thegaram and
0xmountaintop
WalkthroughThe changes in this pull request include an update to the version number in Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
rollup/internal/controller/relayer/l1_relayer.go (3)
131-132: Enhance base fee validationConsider adding specific validation thresholds and more descriptive error messages to better handle edge cases.
- if block.BaseFee == 0 || block.BlobBaseFee == 0 { + if block.BaseFee == 0 { + log.Error("Invalid base fee", "block.Hash", block.Hash, "block.Height", block.Number, "block.BaseFee", block.BaseFee) + return + } + if block.BlobBaseFee == 0 { + log.Error("Invalid blob base fee", "block.Hash", block.Hash, "block.Height", block.Number, "block.BlobBaseFee", block.BlobBaseFee) + return + }
179-182: Improve error handling for better debuggingConsider adding more context to the error message to aid in debugging.
- log.Error("Failed to pack setL1BaseFeeAndBlobBaseFee", "block.Hash", block.Hash, "block.Height", block.Number, "block.BaseFee", baseFee, "block.BlobBaseFee", blobBaseFee, "err", err) + log.Error("Failed to pack setL1BaseFeeAndBlobBaseFee", "block.Hash", block.Hash, "block.Height", block.Number, "block.BaseFee", baseFee, "block.BlobBaseFee", blobBaseFee, "lastBaseFee", r.lastBaseFee, "lastBlobBaseFee", r.lastBlobBaseFee, "err", err)
Line range hint
250-267: Enhance logging for gas oracle updatesConsider adding more detailed logging to track the decision-making process.
func (r *Layer1Relayer) shouldUpdateGasOracle(baseFee uint64, blobBaseFee uint64) bool { // Right after restarting. if r.lastBaseFee == 0 { - log.Info("First time to update gas oracle after restarting", "baseFee", baseFee, "blobBaseFee", blobBaseFee) + log.Info("First time to update gas oracle after restarting", "baseFee", baseFee, "blobBaseFee", blobBaseFee, "minGasPrice", r.minGasPrice) return true } expectedBaseFeeDelta := r.lastBaseFee*r.gasPriceDiff/gasPriceDiffPrecision + 1 if baseFee >= r.minGasPrice && (baseFee >= r.lastBaseFee+expectedBaseFeeDelta || baseFee+expectedBaseFeeDelta <= r.lastBaseFee) { + log.Debug("Updating gas oracle due to base fee change", "baseFee", baseFee, "lastBaseFee", r.lastBaseFee, "expectedDelta", expectedBaseFeeDelta) return true } expectedBlobBaseFeeDelta := r.lastBlobBaseFee * r.gasPriceDiff / gasPriceDiffPrecision // Plus a minimum of 0.01 gwei, since the blob base fee is usually low, preventing short-time flunctuation. expectedBlobBaseFeeDelta += 10000000 if blobBaseFee >= r.minGasPrice && (blobBaseFee >= r.lastBlobBaseFee+expectedBlobBaseFeeDelta || blobBaseFee+expectedBlobBaseFeeDelta <= r.lastBlobBaseFee) { + log.Debug("Updating gas oracle due to blob base fee change", "blobBaseFee", blobBaseFee, "lastBlobBaseFee", r.lastBlobBaseFee, "expectedDelta", expectedBlobBaseFeeDelta) return true } return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
common/version/version.go(1 hunks)rollup/cmd/gas_oracle/app/app.go(2 hunks)rollup/cmd/rollup_relayer/app/app.go(2 hunks)rollup/conf/config.json(0 hunks)rollup/internal/config/relayer.go(0 hunks)rollup/internal/controller/relayer/l1_relayer.go(7 hunks)
💤 Files with no reviewable changes (2)
- rollup/conf/config.json
- rollup/internal/config/relayer.go
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🔇 Additional comments (6)
rollup/cmd/gas_oracle/app/app.go (2)
101-101: LGTM: Function call updated correctly
The function call has been properly updated to use the new rutils alias. The logic remains unchanged, maintaining the important safety measure of using number - 1 to prevent frequent reorgs.
25-25: LGTM: Import alias update
The import alias change from butils to rutils is consistent with the refactoring mentioned in the PR objectives.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
Import alias change is consistent across the codebase
The verification results show:
- No instances of
butilsalias remain in the codebase - The new
rutilsalias is consistently used across 6 files that import the utils package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all imports of the utils package use the new 'rutils' alias
# Check for any remaining 'butils' imports
echo "Checking for any remaining 'butils' imports:"
rg 'butils.*"scroll-tech/rollup/internal/utils"'
# Check for consistent usage of 'rutils'
echo "Checking for 'rutils' imports:"
rg 'rutils.*"scroll-tech/rollup/internal/utils"'
Length of output: 798
rollup/cmd/rollup_relayer/app/app.go (2)
23-23: LGTM: Import alias refactoring
The import alias change from butils to rutils is consistent with the refactoring objectives.
95-95: LGTM: Function call updated correctly
The function call has been properly updated to use the new import alias while maintaining the same functionality and error handling.
✅ Verification successful
Based on the results, I can generate the final response as we have clear evidence that:
- There are no remaining references to
butilsin the codebase - The
rutils.GetLatestConfirmedBlockNumberis consistently used in both occurrences
Function call and import alias updated consistently
The verification confirms that:
- No legacy
butilsreferences remain in the codebase rutils.GetLatestConfirmedBlockNumberis used consistently in both the gas oracle and rollup relayer components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of the new import alias across the codebase
# Expect: All occurrences should use 'rutils', not 'butils'
echo "Checking for any remaining 'butils' references:"
rg "butils\." --type go
echo "Verifying consistent usage of 'rutils':"
rg "rutils\.GetLatestConfirmedBlockNumber" --type go
Length of output: 553
rollup/internal/controller/relayer/l1_relayer.go (2)
39-42: LGTM: Field declarations are well-structured
The removal of weight-related fields simplifies the gas price tracking logic while maintaining essential fields for base fees, thresholds, and price differences.
92-93: LGTM: Constructor initialization is consistent
The initialization properly sets up the essential gas price parameters from the configuration.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1560 +/- ##
===========================================
- Coverage 52.78% 52.53% -0.25%
===========================================
Files 157 157
Lines 12652 12627 -25
===========================================
- Hits 6678 6634 -44
- Misses 5392 5421 +29
+ Partials 582 572 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
colinlyguo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
rollup/abi/bridge_abi_test.go (1)
62-63: Consider using a more meaningful test value for blobBaseFeeThe test uses a placeholder value of 1 for
blobBaseFeewhilebaseFeeuses a more meaningful value (2333). Consider using a realistic value that better represents the expected blob base fee in production.rollup/abi/bridge_abi.go (1)
37-37: Consider updating documentation for EIP-4844 supportThe ABI changes introduce support for blob-related functionality (EIP-4844) through new events and methods. This significant enhancement should be documented.
Consider adding code comments or updating the package documentation to:
- Explain the purpose of blob-related parameters
- Document the relationship between base fees and blob base fees
- Describe the role of different scalar parameters (blobScalar, commitScalar)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
rollup/abi/bridge_abi.go(1 hunks)rollup/abi/bridge_abi_test.go(1 hunks)rollup/internal/controller/relayer/l1_relayer_test.go(0 hunks)rollup/mock_bridge/MockBridge.sol(0 hunks)rollup/tests/bridge_test.go(0 hunks)rollup/tests/gas_oracle_test.go(1 hunks)
💤 Files with no reviewable changes (3)
- rollup/internal/controller/relayer/l1_relayer_test.go
- rollup/mock_bridge/MockBridge.sol
- rollup/tests/bridge_test.go
🔇 Additional comments (3)
rollup/abi/bridge_abi_test.go (1)
62-63: Verify impact of breaking changes
The renaming of setL1BaseFee to setL1BaseFeeAndBlobBaseFee and the addition of the blobBaseFee parameter appear to be breaking changes. This contradicts the PR objectives which state "no breaking changes".
Let's verify the impact:
rollup/tests/gas_oracle_test.go (1)
33-33: 🛠️ Refactor suggestion
Consider setting meaningful values for fork configuration parameters
The chain configuration includes new fork timing parameters (LondonBlock, DarwinTime, DarwinV2Time), but they are initialized with default values that might not reflect realistic test scenarios:
LondonBlockis set to 0DarwinTimeandDarwinV2Timeare initialized as empty pointers
Consider setting these parameters to values that better represent your test environment:
- l1Relayer, err := relayer.NewLayer1Relayer(context.Background(), db, l1Cfg.RelayerConfig, ¶ms.ChainConfig{LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64)}, relayer.ServiceTypeL1GasOracle, nil)
+ darwinTime := uint64(1234567890) // Set appropriate timestamp
+ darwinV2Time := uint64(1234567891) // Set appropriate timestamp
+ l1Relayer, err := relayer.NewLayer1Relayer(
+ context.Background(),
+ db,
+ l1Cfg.RelayerConfig,
+ ¶ms.ChainConfig{
+ LondonBlock: big.NewInt(12345), // Set appropriate block number
+ BernoulliBlock: big.NewInt(0),
+ CurieBlock: big.NewInt(0),
+ DarwinTime: &darwinTime,
+ DarwinV2Time: &darwinV2Time,
+ },
+ relayer.ServiceTypeL1GasOracle,
+ nil,
+ )Let's verify if these parameters are used meaningfully in the tests:
rollup/abi/bridge_abi.go (1)
37-37: Verify impact of ABI changes on dependent code
The L1GasPriceOracle ABI has undergone significant changes with the addition of blob-related functionality and the replacement of setL1BaseFee with setL1BaseFeeAndBlobBaseFee. This could impact dependent code that interacts with the contract.
Let's verify the usage of the old method in the codebase:
✅ Verification successful
ABI changes are properly integrated in the codebase
The codebase has been correctly updated to use the new setL1BaseFeeAndBlobBaseFee method instead of the old setL1BaseFee. The changes are consistently applied in both the implementation (l1_relayer.go) and test code (bridge_abi_test.go), with no remaining references to the old method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of the old setL1BaseFee method
# Expected: No direct calls to setL1BaseFee should remain
# Search for direct method calls
rg -l "setL1BaseFee\(" --type go
# Search for ABI encoding of the method
rg -l "setL1BaseFee.*abi\.encode" --type go
# Search for method signatures
rg -l "0x[a-fA-F0-9]{8}.*setL1BaseFee" --type go
Length of output: 131
Script:
#!/bin/bash
# Let's try a broader search to understand the context and dependencies
# Search for any mentions of L1GasPriceOracle
rg -l "L1GasPriceOracle" --type go
# Search for any gas price oracle related imports or contract references
rg -l "gaspriceoracle|gas_price_oracle" -i --type go
# Look for any test files that might be using the old interface
rg -l "Test.*L1GasPriceOracle|L1GasPriceOracle.*Test" --type go
# Search for any files containing method calls to set base fees
rg -l "SetL1BaseFee|setL1BaseFeeAndBlobBaseFee" --type go
Length of output: 1025
Script:
#!/bin/bash
# Let's examine the specific usage in the identified files
# Check l1_relayer.go implementation
echo "=== l1_relayer.go ===="
rg "setL1BaseFee|setL1BaseFeeAndBlobBaseFee" rollup/internal/controller/relayer/l1_relayer.go -B 2 -A 2
# Check test implementations
echo "=== bridge_abi_test.go ===="
rg "setL1BaseFee|setL1BaseFeeAndBlobBaseFee" rollup/abi/bridge_abi_test.go -B 2 -A 2
# Look for any gas oracle contract address or deployment references
echo "=== Gas Oracle Contract References ==="
rg "L1GasPriceOracle.*0x[a-fA-F0-9]{40}" --type go
Length of output: 999
Purpose or design rationale of this PR
This PR removes outdated logic, i.e. those deprecated fee updation logic.
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
Chores
setL1BaseFeefunction from theMockBridgecontract to simplify its interface.