feat(gas-oracle): support moving average base fee #1735
Conversation
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 detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
rollup/internal/controller/relayer/l1_relayer.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.rollup/internal/orm/l1_block.go (1)
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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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)
🧰 Additional context used
🧬 Code graph analysis (1)
rollup/internal/controller/relayer/l1_relayer.go (2)
rollup/internal/config/relayer.go (1)
GasOracleConfig(91-109)rollup/internal/orm/l1_block.go (2)
L1Block(14-31)L1Block(39-41)
🔇 Additional comments (3)
rollup/conf/config.json (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]rollup/internal/config/relayer.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.
common/version/version.go (1)
8-8: Version bump LGTM.Matches the PR's bump-version intent.
| return | ||
| } | ||
|
|
||
| block := blocks[0] |
There was a problem hiding this comment.
Is this the oldest or newest block?
| 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, notblock.BlobBaseFee - Send tx should probably not use
block.Hashfor ID (risk of conflict with other txs) - Need to consider DB updates, including gas oracle import status
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?
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rollup/internal/controller/relayer/l1_relayer.go (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)
🧹 Nitpick comments (1)
rollup/internal/controller/relayer/l1_relayer.go (1)
154-172: Consider using big.Int for more precise calculations.The current implementation uses floating-point arithmetic which could introduce precision errors. Consider using
big.Intorbig.Ratfor 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)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/internal/controller/relayer/l1_relayer.go(5 hunks)rollup/internal/orm/l1_block.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rollup/internal/controller/relayer/l1_relayer.go (2)
rollup/internal/config/relayer.go (1)
GasOracleConfig(91-109)rollup/internal/orm/l1_block.go (2)
L1Block(14-31)L1Block(39-41)
⏰ 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)
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
🔇 Additional comments (3)
rollup/internal/orm/l1_block.go (1)
74-86: LGTM! Clean implementation of the new query method.The
GetLatestL1Blocksmethod is well-implemented with proper error handling and follows the established ORM patterns in the codebase.rollup/internal/controller/relayer/l1_relayer.go (2)
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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. 🚀 New features to boost your workflow:
|
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
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