Fix bug sequencer submission strategy and log commit price #1664
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning Rate limit exceeded@ranchalp has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
## Walkthrough
This update expands the batch submission strategy timeouts in the Layer 2 relayer, introducing additional offset timeout keys and enforcing explicit validation when selecting a strategy. It also adds a new Prometheus metric to track the commit price. The `SendTransaction` method signature was changed to return an additional `blobBaseFee` value, and all call sites were updated accordingly. Minor logging and metrics improvements were made in the relayer's processing logic.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------|
| rollup/internal/controller/relayer/l2_relayer.go | Expanded `bestParams` map with additional timeout keys offset by +20 minutes; explicit batch strategy lookup with error handling; updated `SendTransaction` calls to capture new return value; added metric setting for commit price; minor logging and error handling adjustments. |
| rollup/internal/controller/relayer/l2_relayer_metrics.go | Added a new Prometheus gauge metric `rollupL2RelayerCommitPrice` to the metrics struct and initialized it. |
| rollup/internal/controller/sender/sender.go | Updated `SendTransaction` method signature to return an additional `blobBaseFee` value; updated error returns to `nil` error where appropriate. |
| rollup/internal/controller/sender/sender_test.go | Updated all `SendTransaction` calls in tests to capture the new third return value, ignoring it with `_`. |
| rollup/internal/controller/relayer/l1_relayer.go | Renamed transaction hash variable from `hash` to `txHash` and updated `SendTransaction` calls to capture three return values. |
| common/version/version.go | Updated version tag from "v4.5.16" to "v4.5.17". |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Config
participant L2Relayer
participant Sender
participant Metrics
Config->>L2Relayer: Provide batch submission timeout
L2Relayer->>L2Relayer: Lookup batch strategy in bestParams
alt Timeout invalid
L2Relayer->>Config: Return error
else Timeout valid
L2Relayer->>Sender: SendTransaction(...) returns (txHash, err, blobBaseFee)
alt Transaction success
L2Relayer->>Metrics: Set rollupL2RelayerCommitPrice(blobBaseFee)
end
endPossibly related PRs
Suggested reviewers
Poem
|
colinlyguo
reviewed
May 23, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/relayer/l2_relayer_metrics.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rollup/internal/controller/relayer/l2_relayer.go (1)
rollup/internal/config/relayer.go (1)
BatchSubmission(34-43)
🪛 golangci-lint (1.64.8)
rollup/internal/controller/relayer/l2_relayer.go
1130-1130: cannot use target (variable of type *big.Int) as float64 value in argument to r.metrics.rollupL2RelayerCommitPrice.Set
(typecheck)
🪛 GitHub Actions: Rollup
rollup/internal/controller/relayer/l2_relayer.go
[error] 1130-1130: Type mismatch error: cannot use target (variable of type *big.Int) as float64 value in argument to r.metrics.rollupL2RelayerCommitPrice.Set
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (3)
rollup/internal/controller/relayer/l2_relayer_metrics.go (1)
34-34: LGTM! Clean metric addition.The new
rollupL2RelayerCommitPricemetric is properly declared and initialized following the established patterns in the codebase. The naming and help text clearly indicate its purpose for tracking the L2 relayer's submission strategy commit price.Also applies to: 129-132
rollup/internal/controller/relayer/l2_relayer.go (2)
107-116: Good expansion of batch submission strategies.The addition of offset timeout keys (adding 20 minutes to account for batch creation time) provides more granular strategy options. The comment clearly explains the rationale, and the parameter values maintain consistency with the original strategies.
155-158: Excellent improvement to strategy validation.Adding explicit validation when selecting a batch submission strategy prevents potential runtime panics from invalid map access. This defensive programming approach will help catch configuration errors early.
Also applies to: 177-177
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rollup/internal/controller/relayer/l1_relayer.go(2 hunks)rollup/internal/controller/relayer/l2_relayer.go(8 hunks)rollup/internal/controller/sender/sender.go(3 hunks)rollup/internal/controller/sender/sender_test.go(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- rollup/internal/controller/relayer/l1_relayer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup/internal/controller/relayer/l2_relayer.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
rollup/internal/controller/sender/sender.go
193-193: not enough return values
have (nil, error)
want ("github.com/scroll-tech/go-ethereum/common".Hash, error, uint64)
(typecheck)
196-196: not enough return values
have (nil, error)
want ("github.com/scroll-tech/go-ethereum/common".Hash, error, uint64)
(typecheck)
203-203: not enough return values
have (nil, error)
want ("github.com/scroll-tech/go-ethereum/common".Hash, error, uint64)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (3)
rollup/internal/controller/sender/sender.go (2)
174-174: LGTM: Method signature updated to return blobBaseFee.The addition of the third return value
uint64(blobBaseFee) aligns with the PR objective to add logging for commit price metrics.
252-252: LGTM: Successful return correctly implements new signature.The successful return statement properly returns all three values: transaction hash, nil error, and blobBaseFee.
rollup/internal/controller/sender/sender_test.go (1)
190-190: LGTM: Test calls properly updated for new method signature.All calls to
SendTransactioncorrectly capture the additionalblobBaseFeereturn value using_to ignore it, which is appropriate for test scenarios that don't need to validate the metric value.Also applies to: 548-548, 551-551, 592-592, 634-634, 694-694, 764-764, 838-838, 896-896, 899-899
jonastheis
reviewed
May 23, 2025
colinlyguo
reviewed
May 23, 2025
colinlyguo
approved these changes
May 23, 2025
jonastheis
approved these changes
May 23, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1664 +/- ##
===========================================
+ Coverage 40.86% 40.90% +0.04%
===========================================
Files 225 225
Lines 18061 18072 +11
===========================================
+ Hits 7380 7392 +12
+ Misses 9965 9964 -1
Partials 716 716
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:
|
13 tasks
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose or design rationale of this PR
Fix bug sequencer submission strategy and log commit price
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