feat(opbot): add tx hash after refund [SLT-460] #3375
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="[config_reader] The configuration option 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3375 +/- ##
===================================================
+ Coverage 33.16732% 34.90814% +1.74082%
===================================================
Files 544 78 -466
Lines 34736 2667 -32069
Branches 82 82
===================================================
- Hits 11521 931 -10590
+ Misses 22197 1733 -20464
+ Partials 1018 3 -1015
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![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
🧹 Outside diff range and nitpick comments (1)
contrib/opbot/botmd/commands.go (1)
348-351: LGTM, but needs test coverageThe error handling improvements look good, with clear error messages and proper distinction between different error cases. However, these changes are not covered by tests.
Would you like me to help create unit tests for these error handling scenarios?
Also applies to: 356-356, 361-364
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 348-351: contrib/opbot/botmd/commands.go#L348-L351
Added lines #L348 - L351 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
contrib/opbot/botmd/commands.go(2 hunks)contrib/opbot/config/signer.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- contrib/opbot/config/signer.txt
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 56-56: contrib/opbot/botmd/commands.go#L56
Added line #L56 was not covered by tests
[warning] 348-351: contrib/opbot/botmd/commands.go#L348-L351
Added lines #L348 - L351 were not covered by tests
[warning] 356-356: contrib/opbot/botmd/commands.go#L356
Added line #L356 was not covered by tests
[warning] 361-361: contrib/opbot/botmd/commands.go#L361
Added line #L361 was not covered by tests
[warning] 363-364: contrib/opbot/botmd/commands.go#L363-L364
Added lines #L363 - L364 were not covered by tests
[warning] 373-373: contrib/opbot/botmd/commands.go#L373
Added line #L373 was not covered by tests
🪛 GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 56-56:
Duplicate words (testing) found (dupword)
🔇 Additional comments (1)
contrib/opbot/botmd/commands.go (1)
368-368: Verify the increased timeout duration
The timeout increase from 30s to 1m is reasonable for blockchain transactions, but let's verify this doesn't impact other system timeouts or user experience.
Also applies to: 370-370, 373-373
✅ Verification successful
Based on the search results, I can see that the timeout values in the codebase vary based on their specific use cases:
-
For blockchain transaction-related operations:
- RFQ service uses 1 minute timeout for transaction-related operations
- Explorer service uses timeouts ranging from 30s to 1m for transaction fetching
- CCTP relayer uses 90s timeout for blockchain operations
-
For retry operations with blockchain:
- Most services use 30s to 1m timeout for retry operations
- E2E tests use longer timeouts (5m) for setup operations
The change to increase the timeout from 30s to 1m in contrib/opbot/botmd/commands.go aligns well with the timeout patterns used across the codebase for blockchain transaction operations. This is a reasonable duration that matches other similar blockchain transaction handling timeouts.
Timeout duration of 1 minute is consistent with codebase patterns
The increased timeout from 30s to 1m aligns with similar blockchain transaction handling timeouts across the codebase and is a reasonable duration for waiting for transaction confirmations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other timeouts in the codebase that might need adjustment
rg -A 2 "WithMaxTotalTime|context.*WithTimeout" --type go
Length of output: 26811
contrib/opbot/botmd/commands.go
Outdated
| @@ -53,7 +53,7 @@ | |||
| func (b *Bot) traceCommand() *slacker.CommandDefinition { | |||
| return b.requiresSignoz(&slacker.CommandDefinition{ | |||
| Command: "trace {tags} {order}", | |||
| Description: "find a transaction in signoz", | |||
| Description: "testing testing", | |||
There was a problem hiding this comment.
Revert description to meaningful text
The description "testing testing" appears to be test text that was accidentally left in. This should be reverted to the original description "find a transaction in signoz" to maintain clarity and professionalism.
- Description: "testing testing",
+ Description: "find a transaction in signoz",📝 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.
| Description: "testing testing", | |
| Description: "find a transaction in signoz", |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 56-56: contrib/opbot/botmd/commands.go#L56
Added line #L56 was not covered by tests
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 56-56:
Duplicate words (testing) found (dupword)
Deploying sanguine-fe with
|
| Latest commit: |
ddbc721
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9ab46e2e.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-opbot-tracing.sanguine-fe.pages.dev |
golangisfun123
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
contrib/opbot/botmd/commands.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 323-326: contrib/opbot/botmd/commands.go#L323-L326
Added lines #L323 - L326 were not covered by tests
[warning] 338-339: contrib/opbot/botmd/commands.go#L338-L339
Added lines #L338 - L339 were not covered by tests
[warning] 350-350: contrib/opbot/botmd/commands.go#L350
Added line #L350 was not covered by tests
🔇 Additional comments (2)
contrib/opbot/botmd/commands.go (2)
323-326: Enhance error handling and add test coverage
The error handling could be more informative to help with debugging. Consider including more context in the user-facing message while keeping sensitive details in logs.
- _, err := ctx.Response().Reply("error submitting refund")
+ _, err := ctx.Response().Reply(fmt.Sprintf("Failed to submit refund. Please try again or contact support. Reference: %d", nonce))Let's verify the test coverage:
#!/bin/bash
# Check for existing test files covering refund error scenarios
fd -e go -E '*_test.go' . | xargs rg "TestRfqRefund.*Error"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 323-326: contrib/opbot/botmd/commands.go#L323-L326
Added lines #L323 - L326 were not covered by tests
338-343: Add test coverage for retry scenarios
The retry logic for transaction hash verification has been improved with more specific error handling. However, this critical path lacks test coverage.
Let's check for existing retry tests:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 338-339: contrib/opbot/botmd/commands.go#L338-L339
Added lines #L338 - L339 were not covered by tests
| if err != nil { | ||
| b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | ||
| _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | ||
| if err != nil { | ||
| log.Println(err) | ||
| b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) |
There was a problem hiding this comment.
Remove duplicate error logging
This line creates duplicate error logs for the same error condition that was already logged.
- b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", 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.
| b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 350-350: contrib/opbot/botmd/commands.go#L350
Added line #L350 was not covered by tests
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
Bug Fixes