[SLT-158] fix(opbot): fixed message #3201
Conversation
WalkthroughThe changes in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
Deploying sanguine-fe with
|
| Latest commit: |
a48be5e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://09793cca.sanguine-fe.pages.dev |
| Branch Preview URL: | https://opbot-message.sanguine-fe.pages.dev |
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- contrib/opbot/botmd/commands.go (2 hunks)
🔇 Additional comments (1)
contrib/opbot/botmd/commands.go (1)
367-368: Verify the reduced retry parametersThe maximum attempt time has been reduced from 30 seconds to 15 seconds, and the maximum attempts from 5 to 3. Please verify that these settings are sufficient to allow the refund confirmation process to complete successfully and won't cause premature termination of the retries.
| b.logger.Errorf(ctx, "refund not confirmed yet") | ||
| return errors.New("refund not confirmed yet") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use appropriate log level for expected retries
The message "refund not confirmed yet" is logged as an error. Since this might be a normal condition during retries, consider using a lower log level like Infof or Debugf to prevent cluttering the error logs.
Suggested change:
- b.logger.Errorf(ctx, "refund not confirmed yet")
+ b.logger.Infof(ctx, "refund not confirmed yet")📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| b.logger.Errorf(ctx, "refund not confirmed yet") | |
| return errors.New("refund not confirmed yet") | |
| b.logger.Infof(ctx, "refund not confirmed yet") | |
| return errors.New("refund not confirmed yet") |
| _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | ||
| log.Printf("error fetching quote request: %v\n", err) | ||
| return |
There was a problem hiding this comment.
Check for errors before logging and correct log message
The log message "error fetching quote request" does not match the context, as this section handles refund submission. Additionally, err should be checked before logging to avoid logging nil errors.
Suggested fix:
_, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce))
-if err != nil {
- log.Printf("error fetching quote request: %v\n", err)
+if err != nil {
log.Printf("error sending refund confirmation: %v\n", err)
+}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | |
| log.Printf("error fetching quote request: %v\n", err) | |
| return | |
| _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | |
| if err != nil { | |
| log.Printf("error sending refund confirmation: %v\n", err) | |
| } | |
| return |
| _, err := ctx.Response().Reply("error submitting refund") | ||
| log.Printf("error submitting refund: %v\n", err) | ||
| return |
There was a problem hiding this comment.
Check for errors before logging
After sending the reply with ctx.Response().Reply, the code logs an error without checking if err != nil. This could lead to unnecessary logging when there is no error. Please check the error before logging.
Suggested fix:
_, err := ctx.Response().Reply("error submitting refund")
-if err != nil {
+if err != nil {
log.Printf("error submitting refund: %v\n", err)
+}Committable suggestion was skipped due to low confidence.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3201 +/- ##
===================================================
- Coverage 39.46606% 37.88710% -1.57897%
===================================================
Files 479 418 -61
Lines 26932 24251 -2681
Branches 342 82 -260
===================================================
- Hits 10629 9188 -1441
+ Misses 15557 14320 -1237
+ Partials 746 743 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
golangisfun123
changed the title
golangisfun123
changed the title
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
was returning prematurely on transactions that had not been confirmed yet
Summary by CodeRabbit
New Features
Bug Fixes