feat(gas-oracle): support moving average base fee - #1735
Conversation
Sorry, something went wrong.
WalkthroughBumps version tag to v4.5.47, adds calculate_average_fees_window_size to config and GasOracleConfig, adds ORM method to fetch latest L1 blocks, and changes the L1 relayer to retrieve up to N recent blocks and compute averaged base/blob fees for gas-oracle updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Config
participant R as L1Relayer
participant O as ORM (L1Block)
participant X as ExchangeRateProvider
participant G as GasOracleContract
R->>C: Read CalculateAverageFeesWindowSize (N)
R->>O: GetLatestL1Blocks(limit=N)
alt No blocks
R-->>R: Log warning and return
else Blocks found
R-->>R: calculateAverageFees(blocks) -> avgBaseFee, avgBlobBaseFee
opt AlternativeGasToken enabled
R->>X: Get exchange rate
X-->>R: rate
R-->>R: Adjust avgBaseFee / avgBlobBaseFee (ceil, overflow checks)
end
R->>G: Submit updateL1GasOracle-<id> with baseFee/blobBaseFee
G-->>R: Tx result / confirmation
R-->>R: Update status/metrics/last fees
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review details Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 94da520 and 3dd9ab6. 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)rollup/internal/controller/relayer/l1_relayer.go (1)📜 Review detailsrollup/internal/orm/l1_block.go (1)130-131: Use a time-weighted or weighted-median aggregation for fee calculation
Replace the simple arithmetic mean in calculateAverageFees with a TWAP (time-weighted average) or a weighted-median over the last N blocks to give more weight to recent blocks and reduce manipulation risk. You may also layer in staleness checks, multi-source median aggregation, and tiered (fast/standard/slow) buffers as per oracle best practices.74-86: Validate limit to avoid silent LIMIT 0 queries.
If limit <= 0, GORM will issue LIMIT 0 and return no rows, possibly breaking downstream averaging. Guard and fail fast.
func (o *L1Block) GetLatestL1Blocks(ctx context.Context, limit int) ([]L1Block, error) { + if limit <= 0 { + return nil, fmt.Errorf("L1Block.GetLatestL1Blocks invalid limit: %d (must be > 0)", limit) + } db := o.db.WithContext(ctx) db = db.Model(&L1Block{}) db = db.Order("number DESC") db = db.Limit(limit)
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between a5a7844 and 277e71e.
📒 Files selected for processing (5)- common/version/version.go (1 hunks)
- rollup/conf/config.json (1 hunks)
- rollup/internal/config/relayer.go (1 hunks)
- rollup/internal/controller/relayer/l1_relayer.go (3 hunks)
- rollup/internal/orm/l1_block.go (1 hunks)
rollup/internal/config/relayer.go (1)🔇 Additional comments (3)rollup/internal/orm/l1_block.go (2)
- GasOracleConfig (91-109)
- L1Block (14-31)
- L1Block (39-41)
rollup/conf/config.json (1)rollup/internal/config/relayer.go (1)25-25: Ensure fee-averaging window matches desired oracle responsiveness
100 blocks ≈ 100 × 12 s ≈ 20 minutes; confirm that averaging over ~20 minutes balances stability and responsiveness for your gas oracle. [rollup/conf/config.json:25]common/version/version.go (1)107-108: LGTM! Clear configuration field for window size.
The field name and JSON tag are descriptive and the type is appropriate for representing a block count.
8-8: Version bump LGTM.
Matches the PR's bump-version intent.
Sorry, something went wrong.
| return | ||
| } | ||
|
|
||
| block := blocks[0] |
There was a problem hiding this comment.
Is this the oldest or newest block?
Sorry, something went wrong.
There was a problem hiding this comment.
it's the newest block.
Sorry, something went wrong.
| baseFee := block.BaseFee | ||
| blobBaseFee := block.BlobBaseFee | ||
| // calculate the average base fee and blob base fee of the last N blocks | ||
| baseFee, blobBaseFee := r.calculateAverageFees(blocks) |
There was a problem hiding this comment.
Many places below still use block.
- Threshold should be checked against blobBaseFee, not block.BlobBaseFee
- Send tx should probably not use block.Hash for ID (risk of conflict with other txs)
- Need to consider DB updates, including gas oracle import status
Sorry, something went wrong.
There was a problem hiding this comment.
Send tx should probably not use block.Hash for ID (risk of conflict with other txs)
I think we can add a prefix updateL1GasOracle- before block.Hash. btw, just want to check this is an already existing risk, instead of introducing by this PR right?
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
Currently we update the status to GasOracleImporting and then to GasOracleImported, ensuring that we import the same L1 block info at most once.
If your intention that we keep exactly the same logic, but now instead of meaning "this block is being imported / has been imported", it will mean that "the avg price with the window up to this block is being imported / has been imported"?
In that case, my points 2 and 3 above can be ignored. But please double check that this will work as intended.
Sorry, something went wrong.
There was a problem hiding this comment.
it will mean that "the avg price with the window up to this block is being imported / has been imported"?
Yes, that's my intention. We only care about if the newest block of the window has been imported/importing or not. If it has, we just skip it, and waiting for the next new l1 block.
I think this will work as intended.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)rollup/internal/controller/relayer/l1_relayer.go (1)🧹 Nitpick comments (1)109-120: Add configuration validation for window size.
The window size configuration is used directly without validation. A negative or excessively large value could cause performance issues or unexpected behavior.
limit := r.cfg.GasOracleConfig.CalculateAverageFeesWindowSize +// Validate and sanitize the window size +if limit <= 0 { + log.Error("Invalid CalculateAverageFeesWindowSize, using default", "configured", limit) + limit = 10 // reasonable default +} else if limit > 1000 { + log.Warn("CalculateAverageFeesWindowSize is very large, capping to 1000", "configured", limit) + limit = 1000 +} blocks, err := r.l1BlockOrm.GetLatestL1Blocks(r.ctx, limit)
rollup/internal/controller/relayer/l1_relayer.go (1)📜 Review details154-172: Consider using big.Int for more precise calculations.
The current implementation uses floating-point arithmetic which could introduce precision errors. Consider using big.Int or big.Rat for exact calculations, especially for financial values.
Example approach using big.Int:
// Use big.Int for precise calculation baseFeeInt := new(big.Int).SetUint64(baseFee) exchangeRateInt := new(big.Int).SetUint64(uint64(exchangeRate * 1e18)) // Scale exchange rate scaledBaseFee := new(big.Int).Mul(baseFeeInt, big.NewInt(1e18)) adjustedBaseFee := new(big.Int).Div(scaledBaseFee, exchangeRateInt) // Add 1 for ceiling effect if new(big.Int).Mod(scaledBaseFee, exchangeRateInt).Sign() > 0 { adjustedBaseFee.Add(adjustedBaseFee, big.NewInt(1)) }
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 4d9fd55 and 94da520.
📒 Files selected for processing (2)- rollup/internal/controller/relayer/l1_relayer.go (5 hunks)
- rollup/internal/orm/l1_block.go (1 hunks)
rollup/internal/config/relayer.go (1)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)rollup/internal/orm/l1_block.go (2)
- GasOracleConfig (91-109)
- L1Block (14-31)
- L1Block (39-41)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
rollup/internal/orm/l1_block.go (1)rollup/internal/controller/relayer/l1_relayer.go (2)74-86: LGTM! Clean implementation of the new query method.
The GetLatestL1Blocks method is well-implemented with proper error handling and follows the established ORM patterns in the codebase.
130-131: LGTM! Good use of helper function for average calculation.
The extraction of average fee calculation into a dedicated helper function improves code organization and maintainability.
195-197: LGTM! Good addition of unique transaction identifier.
The updateL1GasOracle- prefix helps avoid transaction ID conflicts and improves traceability.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 6.06061% with 62 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## develop #1735 +/- ##
===========================================
- Coverage 36.98% 36.82% -0.17%
===========================================
Files 245 245
Lines 20804 20853 +49
===========================================
- Hits 7695 7679 -16
- Misses 12299 12363 +64
- Partials 810 811 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Purpose or design rationale of this PR
This PR updates the gas-oracle to use a moving average of the base fee, rather than relying solely on the latest block’s baseFee and blobBaseFee. This approach should provide a more accurate representation of the L1 fee.
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 tag in common/version.go been updated or have you added bump-version label to this PR?
Breaking change label
Does this PR have the breaking-change label?
Summary by CodeRabbit
New Features
Bug Fixes / Chores