RFQ: failed rebalance does not block chain listener #2430
Conversation
WalkthroughThe recent changes involve reordering imports, updating package references, setting Changes
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 (
|
Deploying sanguine-fe with
|
| Latest commit: |
b8f6b6f
|
| Status: | ⚡️ Build in progress... |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- ethergo/listener/listener.go (4 hunks)
- services/rfq/relayer/service/chainindexer.go (1 hunks)
Additional comments not posted (2)
ethergo/listener/listener.go (2)
112-118: Ensure proper error handling and backoff strategy.
The current implementation correctly sets thepollIntervalbased on the backoff strategy after handling errors. This is a good practice for managing retry logic in case of transient failures. However, ensure that the backoff configuration (newBackoffConfig()) is appropriately tuned for your use case to avoid excessive retries or long delays.
164-164: Ensure error handling is comprehensive and consistent.
The error message "could not put latest block" is clear and includes the wrapped error. This is a good practice for error handling as it provides context for debugging. Ensure that similar error handling practices are applied consistently throughout the application.
| @@ -205,7 +205,7 @@ type decimalsRes struct { | |||
| func (r *Relayer) handleDepositClaimed(ctx context.Context, event *fastbridge.FastBridgeBridgeDepositClaimed, chainID int) error { | |||
| err := r.inventory.Rebalance(ctx, chainID, event.Token) | |||
| if err != nil { | |||
| return fmt.Errorf("could not rebalance: %w", err) | |||
| logger.Errorf("could not rebalance: %w", err) | |||
There was a problem hiding this comment.
Replace the %w verb with %v when logging errors.
- logger.Errorf("could not rebalance: %w", err)
+ logger.Errorf("could not rebalance: %v", err)The %w verb is used for wrapping errors when using fmt.Errorf to allow errors to be unwrapped later. For logging purposes, %v or %s should be used instead, as wrapping is not necessary.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| logger.Errorf("could not rebalance: %w", err) | |
| logger.Errorf("could not rebalance: %v", err) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2430 +/- ##
===================================================
- Coverage 46.94871% 46.94045% -0.00826%
===================================================
Files 400 383 -17
Lines 30610 29269 -1341
Branches 83 83
===================================================
- Hits 14371 13739 -632
+ Misses 14707 14069 -638
+ Partials 1532 1461 -71
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- ethergo/listener/listener.go (7 hunks)
Additional Context Used
GitHub Check Runs (1)
Lint (ethergo) failure (4)
ethergo/listener/listener.go: [failure] 209-209:
Comment should end in a period (godot)
There was a problem hiding this comment.
Actionable comments posted: 3
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- ethergo/listener/listener.go (9 hunks)
Additional Context Used
GitHub Check Runs (2)
codecov/patch success (4)
ethergo/listener/listener.go: [warning] 190-191: ethergo/listener/listener.go#L190-L191
Added lines #L190 - L191 were not covered by tests
ethergo/listener/listener.go: [warning] 196-196: ethergo/listener/listener.go#L196
Added line #L196 was not covered by tests
ethergo/listener/listener.go: [warning] 218-219: ethergo/listener/listener.go#L218-L219
Added lines #L218 - L219 were not covered by testsLint (ethergo) failure (4)
ethergo/listener/listener.go: [failure] 209-209:
Comment should end in a period (godot)
Additional comments not posted (2)
ethergo/listener/listener.go (2)
10-11: The reordering of imports and updating of package references appear to be done for clarity and consistency.
108-120: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [111-139]
Introducing
endBlockas a variable and its subsequent usage in the log processing logic is a positive change. It enhances the flexibility and accuracy of block range handling.
| @@ -130,7 +136,7 @@ | |||
|
|
|||
| // Handle if the listener is more than one get logs range behind the head | |||
| // Note: this does not cover the edge case of a reorg that includes a new tx | |||
| endBlock := c.latestBlock | |||
| endBlock = c.latestBlock | |||
There was a problem hiding this comment.
Consider handling the edge case where c.startBlock + maxGetLogsRange exceeds the maximum value of uint64 to prevent overflow.
- endBlock = c.startBlock + maxGetLogsRange
+ if c.startBlock > math.MaxUint64 - maxGetLogsRange {
+ endBlock = math.MaxUint64
+ } else {
+ endBlock = c.startBlock + maxGetLogsRange
+ }This change ensures that the calculation of endBlock does not overflow, which could potentially cause the application to behave unexpectedly or miss logs.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| endBlock = c.latestBlock | |
| if c.startBlock > math.MaxUint64 - maxGetLogsRange { | |
| endBlock = math.MaxUint64 | |
| } else { | |
| endBlock = c.startBlock + maxGetLogsRange | |
| } |
ethergo/listener/listener.go
Outdated
| @@ -211,6 +205,21 @@ | |||
| return startBlock, chainID, nil | |||
| } | |||
|
|
|||
| // TODO: consider some kind of backoff here in case rpcs are down at boot. | |||
| // this becomes more of an issue as we add more chains | |||
There was a problem hiding this comment.
The comment should end in a period to follow Go documentation conventions.
- // this becomes more of an issue as we add more chains
+ // This becomes more of an issue as we add more chains.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // this becomes more of an issue as we add more chains | |
| // This becomes more of an issue as we add more chains. |
| return 0, 0, fmt.Errorf("could not get chain ID: %w", err) | ||
| } |
There was a problem hiding this comment.
The added lines related to error handling and chain ID retrieval were not covered by tests. It's crucial to add unit tests for these changes to ensure the new logic works as expected and does not introduce regressions.
Also applies to: 196-196, 218-219
Description
If we encounter an error when rebalancing, we should only log the error instead of returning it, since a bubbled-up error results in reprocessing a log range.
Summary by CodeRabbit
Refactor
Bug Fixes