[SLT-315] refactor(opbot): use gql api #3260
Conversation
Deploying sanguine-fe with
|
| Latest commit: |
1fbfbfa
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://08ae5784.sanguine-fe.pages.dev |
| Branch Preview URL: | https://opbot-migration.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3260 +/- ##
====================================================
- Coverage 43.84981% 30.65365% -13.19616%
====================================================
Files 74 448 +374
Lines 2317 29817 +27500
Branches 82 82
====================================================
+ Hits 1016 9140 +8124
- Misses 1295 19827 +18532
- Partials 6 850 +844
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
WalkthroughThe changes in this pull request primarily focus on 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (8)
contrib/opbot/internal/model.go (6)
3-9: Add a comment for the exported typeGetRFQByTxIDResponseThe struct is well-structured and follows Go conventions. However, to improve documentation and address the static analysis hint, please add a comment describing the purpose and usage of this type.
Example comment:
// GetRFQByTxIDResponse represents the complete response structure for a Request for Quotation (RFQ) lookup by transaction ID. // It includes details about the bridge, request, relay, proof, and claim associated with the RFQ.🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 3-3:
exported: exported type GetRFQByTxIDResponse should have comment or be unexported (revive)
11-23: Add a comment and consider changing the type ofSendChainGasThe
Bridgestruct is well-structured and follows Go conventions. However, please consider the following improvements:
Add a comment describing the purpose and usage of this type to improve documentation and address the static analysis hint.
The
SendChainGasfield is currently of typeint. Depending on the expected range of values, this might not be sufficient. Consider usinguint64orbig.Intif larger values are possible.Example comment and type change:
// Bridge represents the core information of a bridge transaction, // including details about the origin and destination chains, tokens, and amounts. type Bridge struct { // ... other fields ... SendChainGas uint64 `json:"sendChainGas"` }🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 11-11:
exported: exported type Bridge should have comment or be unexported (revive)
25-29: Add a comment and consider changing the type ofBlockNumberThe
BridgeRequeststruct is well-structured and follows Go conventions. However, please consider the following improvements:
Add a comment describing the purpose and usage of this type to improve documentation and address the static analysis hint.
The
BlockNumberfield is currently of typestring. Depending on how this value is used, consider changing it to a numeric type likeuint64for better type safety and easier numerical operations.Example comment and type change:
// BridgeRequest represents the details of a bridge request, // including the block number, timestamp, and transaction hash. type BridgeRequest struct { BlockNumber uint64 `json:"blockNumber,string"` BlockTimestamp int64 `json:"blockTimestamp"` TransactionHash string `json:"transactionHash"` }Note: If you need to keep the JSON representation as a string, you can use the
,stringtag as shown above.🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 25-25:
exported: exported type BridgeRequest should have comment or be unexported (revive)
31-37: Add a comment and consider changing the type ofBlockNumberThe
BridgeRelaystruct is well-structured and follows Go conventions. However, please consider the following improvements:
Add a comment describing the purpose and usage of this type to improve documentation and address the static analysis hint.
The
BlockNumberfield is currently of typestring. Depending on how this value is used, consider changing it to a numeric type likeuint64for better type safety and easier numerical operations.Example comment and type change:
// BridgeRelay represents the details of a bridge relay operation, // including block information, transaction hash, relayer, and destination address. type BridgeRelay struct { BlockNumber uint64 `json:"blockNumber,string"` BlockTimestamp int64 `json:"blockTimestamp"` TransactionHash string `json:"transactionHash"` Relayer string `json:"relayer"` To string `json:"to"` }Note: If you need to keep the JSON representation as a string, you can use the
,stringtag as shown above.🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 31-31:
exported: exported type BridgeRelay should have comment or be unexported (revive)
39-44: Add a comment and consider changing the type ofBlockNumberThe
BridgeProofstruct is well-structured and follows Go conventions. However, please consider the following improvements:
Add a comment describing the purpose and usage of this type to improve documentation and address the static analysis hint.
The
BlockNumberfield is currently of typestring. Depending on how this value is used, consider changing it to a numeric type likeuint64for better type safety and easier numerical operations.Example comment and type change:
// BridgeProof represents the proof details for a bridge operation, // including block information, transaction hash, and relayer address. type BridgeProof struct { BlockNumber uint64 `json:"blockNumber,string"` BlockTimestamp int64 `json:"blockTimestamp"` TransactionHash string `json:"transactionHash"` Relayer string `json:"relayer"` }Note: If you need to keep the JSON representation as a string, you can use the
,stringtag as shown above.🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 39-39:
exported: exported type BridgeProof should have comment or be unexported (revive)
46-53: Add a comment and consider changing the types ofBlockNumberandAmountFormattedThe
BridgeClaimstruct is well-structured and follows Go conventions. However, please consider the following improvements:
Add a comment describing the purpose and usage of this type to improve documentation and address the static analysis hint.
The
BlockNumberfield is currently of typestring. Depending on how this value is used, consider changing it to a numeric type likeuint64for better type safety and easier numerical operations.The
AmountFormattedfield is of typestring. Depending on the precision required and how this value is used, consider using a more appropriate type such asdecimal.Decimalfrom theshopspring/decimalpackage for accurate decimal arithmetic.Example comment and type changes:
import "github.com/shopspring/decimal" // BridgeClaim represents the details of a bridge claim operation, // including block information, transaction details, and the claimed amount. type BridgeClaim struct { BlockNumber uint64 `json:"blockNumber,string"` BlockTimestamp int64 `json:"blockTimestamp"` TransactionHash string `json:"transactionHash"` To string `json:"to"` Relayer string `json:"relayer"` AmountFormatted decimal.Decimal `json:"amountFormatted"` }Note: If you need to keep the JSON representation as a string for
BlockNumber, you can use the,stringtag as shown above. ForAmountFormatted, thedecimal.Decimaltype handles JSON marshaling and unmarshaling automatically.🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 46-46:
exported: exported type BridgeClaim should have comment or be unexported (revive)contrib/opbot/botmd/botmd.go (1)
33-33: LGTM: New rfqClient field addedThe addition of the
rfqClientfield of typeinternal.RFQClientaligns well with the PR objective of refactoring to use a GraphQL API. This encapsulation of RFQ functionality in an internal client is a good design choice.Consider adding a brief comment explaining the purpose of this field, e.g.:
// rfqClient handles interactions with the RFQ (Request for Quote) system rfqClient internal.RFQClientcontrib/opbot/internal/client.go (1)
1-1: Add a package comment for the 'internal' packageIt's recommended to include a package-level comment that summarizes the purpose of the package.
🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 1-1:
package-comments: should have a package comment (revive)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- contrib/opbot/botmd/botmd.go (3 hunks)
- contrib/opbot/botmd/commands.go (5 hunks)
- contrib/opbot/botmd/commands_test.go (0 hunks)
- contrib/opbot/botmd/export_test.go (0 hunks)
- contrib/opbot/config/config.go (1 hunks)
- contrib/opbot/internal/client.go (1 hunks)
- contrib/opbot/internal/model.go (1 hunks)
💤 Files with no reviewable changes (2)
- contrib/opbot/botmd/commands_test.go
- contrib/opbot/botmd/export_test.go
🧰 Additional context used
🪛 GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 194-194:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 210-210:
G115: integer overflow conversion int -> uint32 (gosec)contrib/opbot/internal/client.go
[failure] 66-66:
ifElseChain: rewrite if-else to switch statement (gocritic)
[failure] 1-1:
package-comments: should have a package comment (revive)contrib/opbot/internal/model.go
[failure] 3-3:
exported: exported type GetRFQByTxIDResponse should have comment or be unexported (revive)
[failure] 11-11:
exported: exported type Bridge should have comment or be unexported (revive)
[failure] 25-25:
exported: exported type BridgeRequest should have comment or be unexported (revive)
[failure] 31-31:
exported: exported type BridgeRelay should have comment or be unexported (revive)
[failure] 39-39:
exported: exported type BridgeProof should have comment or be unexported (revive)
[failure] 46-46:
exported: exported type BridgeClaim should have comment or be unexported (revive)
🔇 Additional comments (2)
contrib/opbot/internal/model.go (1)
1-53: Overall structure is good, but some improvements are recommendedThe file is well-structured and follows Go conventions. The defined structs provide a clear representation of the RFQ (Request for Quotation) process and related components. However, there are a few areas for improvement:
- Add comments for all exported types to improve documentation and address static analysis hints.
- Consider changing the type of
BlockNumberfields fromstringtouint64for better type safety and easier numerical operations.- For the
AmountFormattedfield inBridgeClaim, consider using a more appropriate type for decimal arithmetic, such asdecimal.Decimal.- Review the
SendChainGasfield in theBridgestruct to ensure theinttype is sufficient for all possible gas values.These changes will enhance the overall quality, maintainability, and type safety of the code.
🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 3-3:
exported: exported type GetRFQByTxIDResponse should have comment or be unexported (revive)
[failure] 11-11:
exported: exported type Bridge should have comment or be unexported (revive)
[failure] 25-25:
exported: exported type BridgeRequest should have comment or be unexported (revive)
[failure] 31-31:
exported: exported type BridgeRelay should have comment or be unexported (revive)
[failure] 39-39:
exported: exported type BridgeProof should have comment or be unexported (revive)
[failure] 46-46:
exported: exported type BridgeClaim should have comment or be unexported (revive)contrib/opbot/botmd/botmd.go (1)
9-9: LGTM: New import for internal packageThe addition of the
internalpackage import is appropriate for the refactoring being done. Using an internal package is a good practice for code that should not be imported by external projects.
| // RFQIndexerAPIURL is the URL of the RFQ indexer API. | ||
| RFQIndexerAPIURL string `yaml:"rfq_indexer_api_url"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the new RFQIndexerAPIURL field
The addition of the RFQIndexerAPIURL field is good, but there are a few suggestions to improve it:
- Add a comment explaining the purpose of this field, similar to other fields in the struct.
- Consider adding validation for this field in the
Validatemethod if it's required for the application to function correctly. - For consistency, you might want to rename
RFQApiURLtoRFQAPIUrlto match the naming convention of the new field.
Here's a suggested improvement for the field declaration:
+ // RFQIndexerAPIURL is the URL of the RFQ indexer API.
RFQIndexerAPIURL string `yaml:"rfq_indexer_api_url"`Consider updating the Validate method to include a check for this new field if necessary:
func (c *Config) Validate() error {
// ... existing validations ...
if c.RFQIndexerAPIURL == "" {
return errors.New("rfq_indexer_api_url is required")
}
return nil
}
contrib/opbot/botmd/botmd.go
Outdated
| handler: handler, | ||
| cfg: cfg, | ||
| server: server, | ||
| logger: experimentalLogger.MakeWrappedSugaredLogger(sugaredLogger), | ||
| rfqClient: internal.NewRFQClient(handler, cfg.RFQIndexerAPIURL, cfg.RelayerURLS), |
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: Missing RFQIndexerAPIURL in config.Config
The RFQIndexerAPIURL field is missing in the config.Config struct, which is required for the proper initialization of rfqClient.
- File:
contrib/opbot/config/config.go
🔗 Analysis chain
LGTM: Initialization of rfqClient and improved formatting
The initialization of the rfqClient in the NewBot function is correct and aligns with the new field added to the Bot struct. The reformatting of the struct initialization improves readability.
Please ensure that the RFQIndexerAPIURL and RelayerURLS fields have been added to the config.Config struct. Run the following script to verify:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the presence of new config fields
# Test: Check for RFQIndexerAPIURL and RelayerURLS in the config struct
rg --type go 'type\s+Config\s+struct' -A 20 | rg 'RFQIndexerAPIURL|RelayerURLS'
Length of output: 240
contrib/opbot/internal/client.go
Outdated
| SetResult(&res). | ||
| Get(fmt.Sprintf(getRequestByTxHash, txID)) | ||
| if err != nil { | ||
| return nil, "", fmt.Errorf("failed to get quote request by tx hash: %w", err) |
There was a problem hiding this comment.
Correct the error message in GetRFQByTxID function
The error message refers to "tx hash" instead of "tx ID". Update it for consistency with the function's purpose.
Apply this diff to fix the error message:
- return nil, "", fmt.Errorf("failed to get quote request by tx hash: %w", err)
+ return nil, "", fmt.Errorf("failed to get quote request by tx ID: %w", 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.
| return nil, "", fmt.Errorf("failed to get quote request by tx hash: %w", err) | |
| return nil, "", fmt.Errorf("failed to get quote request by tx ID: %w", err) |
contrib/opbot/internal/client.go
Outdated
| for _, relayerClient := range r.relayerClients { | ||
| resp, err = relayerClient.GetQuoteRequestByTxHash(ctx, txHash) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get quote request by tx hash: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return resp, nil |
There was a problem hiding this comment.
Handle errors from multiple relayer clients appropriately
Currently, the function returns on the first error encountered, which means it doesn't attempt to fetch from other relayer clients. Consider modifying the logic to try all clients before returning an error.
Apply this diff to continue trying other clients on error:
func (r *rfqClientImpl) GetRFQByTxHash(ctx context.Context, txHash string) (*relapi.GetQuoteRequestResponse, error) {
var resp *relapi.GetQuoteRequestResponse
- var err error
+ var errs []error
for _, relayerClient := range r.relayerClients {
resp, err := relayerClient.GetQuoteRequestByTxHash(ctx, txHash)
if err != nil {
- return nil, fmt.Errorf("failed to get quote request by tx hash: %w", err)
+ errs = append(errs, err)
+ continue
}
+ return resp, nil
}
- }
-
- return resp, nil
+ return nil, fmt.Errorf("failed to get quote request by tx hash from all relayer clients: %v", errs)
}📝 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.
| for _, relayerClient := range r.relayerClients { | |
| resp, err = relayerClient.GetQuoteRequestByTxHash(ctx, txHash) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get quote request by tx hash: %w", err) | |
| } | |
| } | |
| return resp, nil | |
| func (r *rfqClientImpl) GetRFQByTxHash(ctx context.Context, txHash string) (*relapi.GetQuoteRequestResponse, error) { | |
| var resp *relapi.GetQuoteRequestResponse | |
| var errs []error | |
| for _, relayerClient := range r.relayerClients { | |
| resp, err := relayerClient.GetQuoteRequestByTxHash(ctx, txHash) | |
| if err != nil { | |
| errs = append(errs, err) | |
| continue | |
| } | |
| return resp, nil | |
| } | |
| return nil, fmt.Errorf("failed to get quote request by tx hash from all relayer clients: %v", errs) | |
| } |
contrib/opbot/internal/client.go
Outdated
| if res.BridgeClaim != (BridgeClaim{}) { | ||
| status = "Claimed" | ||
| } else if res.BridgeProof != (BridgeProof{}) { | ||
| status = "Proven" | ||
| } else if res.BridgeRelay != (BridgeRelay{}) { | ||
| status = "Relayed" | ||
| } else if res.BridgeRequest != (BridgeRequest{}) { | ||
| status = "Requested" | ||
| } else { | ||
| status = "Unknown" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the if-else chain into a switch statement
The current if-else chain can be simplified using a switch statement for improved readability and maintainability.
Apply this diff to refactor the code:
var status string
- if res.BridgeClaim != (BridgeClaim{}) {
+ switch {
+ case res.BridgeClaim != (BridgeClaim{}):
status = "Claimed"
- } else if res.BridgeProof != (BridgeProof{}) {
+ case res.BridgeProof != (BridgeProof{}):
status = "Proven"
- } else if res.BridgeRelay != (BridgeRelay{}) {
+ case res.BridgeRelay != (BridgeRelay{}):
status = "Relayed"
- } else if res.BridgeRequest != (BridgeRequest{}) {
+ case res.BridgeRequest != (BridgeRequest{}):
status = "Requested"
- } else {
+ default:
status = "Unknown"
}📝 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.
| if res.BridgeClaim != (BridgeClaim{}) { | |
| status = "Claimed" | |
| } else if res.BridgeProof != (BridgeProof{}) { | |
| status = "Proven" | |
| } else if res.BridgeRelay != (BridgeRelay{}) { | |
| status = "Relayed" | |
| } else if res.BridgeRequest != (BridgeRequest{}) { | |
| status = "Requested" | |
| } else { | |
| status = "Unknown" | |
| } | |
| var status string | |
| switch { | |
| case res.BridgeClaim != (BridgeClaim{}): | |
| status = "Claimed" | |
| case res.BridgeProof != (BridgeProof{}): | |
| status = "Proven" | |
| case res.BridgeRelay != (BridgeRelay{}): | |
| status = "Relayed" | |
| case res.BridgeRequest != (BridgeRequest{}): | |
| status = "Requested" | |
| default: | |
| status = "Unknown" | |
| } |
🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 66-66:
ifElseChain: rewrite if-else to switch statement (gocritic)
| } else { | ||
| objects = append(objects, &slack.TextBlockObject{ | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, uint32(res.Bridge.DestChainID))), |
There was a problem hiding this comment.
Potential integer overflow when converting int to uint32
At line 210, res.Bridge.DestChainID is being converted from int to uint32 using uint32(res.Bridge.DestChainID). Similar to the issue at line 194, this could cause an integer overflow if res.Bridge.DestChainID exceeds the uint32 maximum value, leading to incorrect chain IDs.
Ensure that res.Bridge.DestChainID is within the uint32 range. If not, consider:
- Switching to a larger unsigned integer type like
uint64. - Adjusting the
toTXSlackLinkfunction to accept anintorint64to safely handle larger chain ID values without risking overflow.
🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 210-210:
G115: integer overflow conversion int -> uint32 (gosec)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
contrib/opbot/internal/client.go (1)
28-50: LGTM with a suggestion: Consider adding URL validation.The
rfqClientImplstruct andNewRFQClientfunction are well-implemented. The client is properly configured with headers and tracing. However, consider adding validation for theindexerURLandrelayerURLsto ensure they are valid before creating the clients.Consider adding URL validation before creating the clients. Here's a simple example:
import "net/url" func validateURL(urlStr string) error { _, err := url.ParseRequestURI(urlStr) return err } // In NewRFQClient if err := validateURL(indexerURL); err != nil { return nil, fmt.Errorf("invalid indexer URL: %w", err) } for _, urlStr := range relayerURLs { if err := validateURL(urlStr); err != nil { return nil, fmt.Errorf("invalid relayer URL %s: %w", urlStr, err) } // ... create relayer client }This will help prevent runtime errors due to invalid URLs.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-44: contrib/opbot/internal/client.go#L34-L44
Added lines #L34 - L44 were not covered by tests
[warning] 46-49: contrib/opbot/internal/client.go#L46-L49
Added lines #L46 - L49 were not covered by testscontrib/opbot/botmd/commands.go (5)
167-170: Add unit tests for new RFQ client callThe new interaction with
b.rfqClient.GetRFQByTxIDis not covered by tests. Adding unit tests for this functionality will help catch potential issues and improve code reliability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 167-170: contrib/opbot/botmd/commands.go#L167-L170
Added lines #L167 - L170 were not covered by tests
179-201: Enhance test coverage for Slack message constructionThe block constructing
objectsfor the Slack message is currently not covered by tests. Consider adding unit tests to ensure the message formatting and content are correct.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 179-201: contrib/opbot/botmd/commands.go#L179-L201
Added lines #L179 - L201 were not covered by tests
203-213: VerifyDestTxHashhandling for different statusesEnsure that the logic for appending
DestTxHashtoobjectshandles all possible statuses correctly. Consider adding unit tests to cover both the "Requested" status and other cases.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 203-213: contrib/opbot/botmd/commands.go#L203-L213
Added lines #L203 - L213 were not covered by tests
243-243: Add unit tests for refund functionalityThe call to
b.rfqClient.GetRFQByTxHashintroduces new logic for refunds that is not currently tested. Adding unit tests will help verify that the refund process works as intended.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 243-243: contrib/opbot/botmd/commands.go#L243
Added line #L243 was not covered by tests
349-350: Add unit tests forgetTxAgefunctionThe new
getTxAgefunction is crucial for accurate time display. Adding unit tests will help ensure it behaves as expected with various timestamp inputs.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 349-350: contrib/opbot/botmd/commands.go#L349-L350
Added lines #L349 - L350 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- contrib/opbot/botmd/commands.go (5 hunks)
- contrib/opbot/internal/client.go (1 hunks)
- contrib/opbot/internal/model.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contrib/opbot/internal/model.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 167-170: contrib/opbot/botmd/commands.go#L167-L170
Added lines #L167 - L170 were not covered by tests
[warning] 179-201: contrib/opbot/botmd/commands.go#L179-L201
Added lines #L179 - L201 were not covered by tests
[warning] 203-213: contrib/opbot/botmd/commands.go#L203-L213
Added lines #L203 - L213 were not covered by tests
[warning] 216-218: contrib/opbot/botmd/commands.go#L216-L218
Added lines #L216 - L218 were not covered by tests
[warning] 243-243: contrib/opbot/botmd/commands.go#L243
Added line #L243 was not covered by tests
[warning] 306-309: contrib/opbot/botmd/commands.go#L306-L309
Added lines #L306 - L309 were not covered by tests
[warning] 313-313: contrib/opbot/botmd/commands.go#L313
Added line #L313 was not covered by tests
[warning] 349-350: contrib/opbot/botmd/commands.go#L349-L350
Added lines #L349 - L350 were not covered by tests
[warning] 356-356: contrib/opbot/botmd/commands.go#L356
Added line #L356 was not covered by testscontrib/opbot/internal/client.go
[warning] 34-44: contrib/opbot/internal/client.go#L34-L44
Added lines #L34 - L44 were not covered by tests
[warning] 46-49: contrib/opbot/internal/client.go#L46-L49
Added lines #L46 - L49 were not covered by tests
[warning] 53-60: contrib/opbot/internal/client.go#L53-L60
Added lines #L53 - L60 were not covered by tests
[warning] 62-64: contrib/opbot/internal/client.go#L62-L64
Added lines #L62 - L64 were not covered by tests
[warning] 66-77: contrib/opbot/internal/client.go#L66-L77
Added lines #L66 - L77 were not covered by tests
[warning] 80-80: contrib/opbot/internal/client.go#L80
Added line #L80 was not covered by tests
[warning] 84-91: contrib/opbot/internal/client.go#L84-L91
Added lines #L84 - L91 were not covered by tests
[warning] 94-94: contrib/opbot/internal/client.go#L94
Added line #L94 was not covered by tests
🔇 Additional comments (5)
contrib/opbot/internal/client.go (5)
1-14: LGTM: Package declaration and imports look good.The package name "internal" is appropriate for an internal implementation, and the imports seem relevant to the RFQ client functionality being implemented.
16-18: LGTM: Constant declaration is appropriate.The
getRequestByTxHashconstant is well-named and follows Go conventions. It's used for constructing the API endpoint, which is a good practice for maintaining consistency and ease of updates.
20-26: LGTM: RFQClient interface is well-defined.The
RFQClientinterface is well-documented and provides a clear abstraction for RFQ operations. The method signatures are appropriate and follow Go conventions.
52-81: LGTM: GetRFQByTxID implementation looks good.The
GetRFQByTxIDmethod is well-implemented with proper error handling and status determination. The switch statement for status determination is a good improvement from the previous if-else chain.The error message in line 59 still refers to "tx hash" instead of "tx ID". This issue was pointed out in a previous review. Please update it for consistency:
- return nil, "", fmt.Errorf("failed to get quote request by tx hash: %w", err) + return nil, "", fmt.Errorf("failed to get quote request by tx ID: %w", err)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 53-60: contrib/opbot/internal/client.go#L53-L60
Added lines #L53 - L60 were not covered by tests
[warning] 62-64: contrib/opbot/internal/client.go#L62-L64
Added lines #L62 - L64 were not covered by tests
[warning] 66-77: contrib/opbot/internal/client.go#L66-L77
Added lines #L66 - L77 were not covered by tests
[warning] 80-80: contrib/opbot/internal/client.go#L80
Added line #L80 was not covered by tests
97-97: LGTM: Good practice for interface compliance check.The type assertion
var _ RFQClient = &rfqClientImpl{}is a great practice. It ensures at compile-time thatrfqClientImplcorrectly implements theRFQClientinterface.
contrib/opbot/internal/client.go
Outdated
| // GetRFQByTxHash gets a quote request by transaction hash. | ||
| func (r *rfqClientImpl) GetRFQByTxHash(ctx context.Context, txHash string) (*relapi.GetQuoteRequestResponse, error) { | ||
| var resp *relapi.GetQuoteRequestResponse | ||
| var err error | ||
| for _, relayerClient := range r.relayerClients { | ||
| resp, err = relayerClient.GetQuoteRequestByTxHash(ctx, txHash) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get quote request by tx hash: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
Improve error handling in GetRFQByTxHash method.
The current implementation of GetRFQByTxHash returns on the first error encountered, which means it doesn't attempt to fetch from other relayer clients. This might not be optimal if some relayer clients are temporarily unavailable.
Consider modifying the logic to try all clients before returning an error. Here's a suggested improvement:
func (r *rfqClientImpl) GetRFQByTxHash(ctx context.Context, txHash string) (*relapi.GetQuoteRequestResponse, error) {
var lastErr error
for _, relayerClient := range r.relayerClients {
resp, err := relayerClient.GetQuoteRequestByTxHash(ctx, txHash)
if err == nil {
return resp, nil
}
lastErr = err
}
return nil, fmt.Errorf("failed to get quote request by tx hash from all relayer clients: %w", lastErr)
}This change will attempt to fetch from all available relayer clients before giving up, improving the robustness of the method.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 84-91: contrib/opbot/internal/client.go#L84-L91
Added lines #L84 - L91 were not covered by tests
[warning] 94-94: contrib/opbot/internal/client.go#L94
Added line #L94 was not covered by tests
contrib/opbot/internal/client.go
Outdated
| // Package internal provides the RFQ client implementation. | ||
| package internal | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net/http" | ||
|
|
||
| "github.com/dubonzi/otelresty" | ||
| "github.com/go-http-utils/headers" | ||
| "github.com/go-resty/resty/v2" | ||
| "github.com/synapsecns/sanguine/core/metrics" | ||
| "github.com/synapsecns/sanguine/services/rfq/relayer/relapi" | ||
| ) | ||
|
|
||
| const ( | ||
| getRequestByTxHash = "/api/transaction-id/%s" | ||
| ) | ||
|
|
||
| // RFQClient is the interface for the RFQ client. | ||
| type RFQClient interface { | ||
| // GetRFQByTxID gets a quote request by transaction ID. | ||
| GetRFQByTxID(ctx context.Context, txID string) (resp *GetRFQByTxIDResponse, status string, err error) | ||
| // GetRFQByTxHash gets a quote request by transaction hash. | ||
| GetRFQByTxHash(ctx context.Context, txHash string) (resp *relapi.GetQuoteRequestResponse, err error) | ||
| } | ||
|
|
||
| type rfqClientImpl struct { | ||
| client *resty.Client | ||
| relayerClients []relapi.RelayerClient | ||
| } | ||
|
|
||
| // NewRFQClient creates a new RFQClient. | ||
| func NewRFQClient(handler metrics.Handler, indexerURL string, relayerURLs []string) RFQClient { | ||
| client := resty.New() | ||
| client.SetBaseURL(indexerURL) | ||
| client.SetHeader(headers.UserAgent, "rfq-client") | ||
|
|
||
| otelresty.TraceClient(client, otelresty.WithTracerProvider(handler.GetTracerProvider())) | ||
|
|
||
| var relayerClients []relapi.RelayerClient | ||
| for _, url := range relayerURLs { | ||
| relayerClients = append(relayerClients, relapi.NewRelayerClient(handler, url)) | ||
| } | ||
|
|
||
| return &rfqClientImpl{ | ||
| client: client, | ||
| relayerClients: relayerClients, | ||
| } | ||
| } | ||
|
|
||
| // GetRFQByTxID gets a quote request by transaction ID. | ||
| func (r *rfqClientImpl) GetRFQByTxID(ctx context.Context, txID string) (*GetRFQByTxIDResponse, string, error) { | ||
| var res GetRFQByTxIDResponse | ||
| resp, err := r.client.R().SetContext(ctx). | ||
| SetResult(&res). | ||
| Get(fmt.Sprintf(getRequestByTxHash, txID)) | ||
| if err != nil { | ||
| return nil, "", fmt.Errorf("failed to get quote request by tx hash: %w", err) | ||
| } | ||
|
|
||
| if resp.StatusCode() != http.StatusOK { | ||
| return nil, "", fmt.Errorf("unexpected status code: %d", resp.StatusCode()) | ||
| } | ||
|
|
||
| var status string | ||
| switch { | ||
| case res.BridgeClaim != (BridgeClaim{}): | ||
| status = "Claimed" | ||
| case res.BridgeProof != (BridgeProof{}): | ||
| status = "Proven" | ||
| case res.BridgeRelay != (BridgeRelay{}): | ||
| status = "Relayed" | ||
| case res.BridgeRequest != (BridgeRequest{}): | ||
| status = "Requested" | ||
| default: | ||
| status = "Unknown" | ||
| } | ||
|
|
||
| return &res, status, nil | ||
| } | ||
|
|
||
| // GetRFQByTxHash gets a quote request by transaction hash. | ||
| func (r *rfqClientImpl) GetRFQByTxHash(ctx context.Context, txHash string) (*relapi.GetQuoteRequestResponse, error) { | ||
| var resp *relapi.GetQuoteRequestResponse | ||
| var err error | ||
| for _, relayerClient := range r.relayerClients { | ||
| resp, err = relayerClient.GetQuoteRequestByTxHash(ctx, txHash) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get quote request by tx hash: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return resp, nil | ||
| } | ||
|
|
||
| var _ RFQClient = &rfqClientImpl{} |
There was a problem hiding this comment.
Improve test coverage for the RFQ client implementation.
The static analysis tool indicates that several parts of this file are not covered by tests. While 100% test coverage isn't always necessary, it's important to have good test coverage for critical parts of the code, especially for a client implementation like this.
Consider adding unit tests for the following:
NewRFQClientfunctionGetRFQByTxIDmethodGetRFQByTxHashmethod
These tests should cover various scenarios, including:
- Successful requests
- Error handling
- Edge cases (e.g., empty responses, network errors)
Adding these tests will improve the reliability and maintainability of the code.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-44: contrib/opbot/internal/client.go#L34-L44
Added lines #L34 - L44 were not covered by tests
[warning] 46-49: contrib/opbot/internal/client.go#L46-L49
Added lines #L46 - L49 were not covered by tests
[warning] 53-60: contrib/opbot/internal/client.go#L53-L60
Added lines #L53 - L60 were not covered by tests
[warning] 62-64: contrib/opbot/internal/client.go#L62-L64
Added lines #L62 - L64 were not covered by tests
[warning] 66-77: contrib/opbot/internal/client.go#L66-L77
Added lines #L66 - L77 were not covered by tests
[warning] 80-80: contrib/opbot/internal/client.go#L80
Added line #L80 was not covered by tests
[warning] 84-91: contrib/opbot/internal/client.go#L84-L91
Added lines #L84 - L91 were not covered by tests
[warning] 94-94: contrib/opbot/internal/client.go#L94
Added line #L94 was not covered by tests
contrib/opbot/botmd/commands.go
Outdated
| return | ||
| } | ||
|
|
||
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toExplorerSlackLink(status.TxHash().String()))) | ||
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toTXSlackLink(status.TxHash().String(), rawRequest.OriginChainID))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors when sending refund transaction link
Similar to the previous point, if there's an error when sending the refund transaction link, consider handling it appropriately to enhance user experience and error transparency.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 313-313: contrib/opbot/botmd/commands.go#L313
Added line #L313 was not covered by tests
| } | ||
|
|
||
| func toExplorerSlackLink(ogHash string) string { | ||
| rfqHash := strings.ToUpper(ogHash) | ||
| // cut off 0x | ||
| if strings.HasPrefix(rfqHash, "0x") { | ||
| if strings.HasPrefix(rfqHash, "0X") { |
There was a problem hiding this comment.
Ensure consistent hash prefix handling in toExplorerSlackLink
The current code checks for the "0X" prefix but does not handle the lowercase "0x" prefix. Consider updating the condition to handle both cases to prevent inconsistent behavior with different hash formats.
Apply this diff to address the issue:
if strings.HasPrefix(rfqHash, "0X") || strings.HasPrefix(rfqHash, "0x") {
rfqHash = rfqHash[2:]
}
rfqHash = strings.ToLower(rfqHash)Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 356-356: contrib/opbot/botmd/commands.go#L356
Added line #L356 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
contrib/opbot/internal/client.go (4)
16-18: Consider using a more descriptive constant name.The constant
getRequestByTxHashis used, but it might be more clear if it was named to indicate it's an API endpoint path.Consider renaming the constant to something like
getRequestByTxHashEndpointfor improved clarity:- getRequestByTxHash = "/api/transaction-id/%s" + getRequestByTxHashEndpoint = "/api/transaction-id/%s"
28-50: LGTM: rfqClientImpl and NewRFQClient are well-implemented.The rfqClientImpl struct and NewRFQClient function are correctly implemented. The function properly initializes the HTTP client with tracing and sets up the relayer clients.
Consider adding error handling for the case when no relayer URLs are provided:
func NewRFQClient(handler metrics.Handler, indexerURL string, relayerURLs []string) RFQClient { + if len(relayerURLs) == 0 { + panic("no relayer URLs provided") + } client := resty.New() // ... rest of the function }
52-81: LGTM: GetRFQByTxID method is well-implemented.The GetRFQByTxID method correctly retrieves the quote request by transaction ID and handles errors appropriately. The status determination logic uses a switch statement as suggested in a previous review.
Consider extracting the status determination logic into a separate function for improved readability and reusability:
func determineStatus(res *GetRFQByTxIDResponse) string { switch { case res.BridgeClaim != (BridgeClaim{}): return "Claimed" case res.BridgeProof != (BridgeProof{}): return "Proven" case res.BridgeRelay != (BridgeRelay{}): return "Relayed" case res.BridgeRequest != (BridgeRequest{}): return "Requested" default: return "Unknown" } }Then, you can simplify the GetRFQByTxID method:
status := determineStatus(&res) return &res, status, nil
83-95: LGTM: GetRFQByTxHash method is well-implemented.The GetRFQByTxHash method correctly iterates through all relayer clients and returns the first successful response. Error handling has been improved as suggested in a previous review.
Consider using a more descriptive error message when all clients fail:
- return nil, fmt.Errorf("could not get quote request by tx hash: %v", errs) + return nil, fmt.Errorf("failed to get quote request by tx hash from all relayer clients (%d): %v", len(errs), errs)This change provides more context about the number of failed attempts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- contrib/opbot/internal/client.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
contrib/opbot/internal/client.go (3)
1-14: LGTM: Package declaration and imports are appropriate.The package name "internal" is suitable for an internal implementation. The imports include necessary standard library packages and relevant external dependencies for the RFQ client implementation.
20-26: LGTM: RFQClient interface is well-defined.The RFQClient interface is clearly defined with two methods, GetRFQByTxID and GetRFQByTxHash. Each method has a descriptive comment explaining its purpose.
1-97: Overall, the RFQ client implementation is well-structured and follows good practices.The code addresses previous review comments and implements proper error handling. Minor suggestions for improvements have been made in this review.
As mentioned in a previous review comment, it's important to improve test coverage for this file. Please add unit tests for the following:
NewRFQClientfunctionGetRFQByTxIDmethodGetRFQByTxHashmethodThese tests should cover various scenarios, including successful requests, error handling, and edge cases. To verify the current test coverage, run the following command:
This will help ensure the reliability and maintainability of the code.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- contrib/opbot/botmd/commands.go (5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 167-170: contrib/opbot/botmd/commands.go#L167-L170
Added lines #L167 - L170 were not covered by tests
[warning] 179-201: contrib/opbot/botmd/commands.go#L179-L201
Added lines #L179 - L201 were not covered by tests
[warning] 203-213: contrib/opbot/botmd/commands.go#L203-L213
Added lines #L203 - L213 were not covered by tests
[warning] 216-218: contrib/opbot/botmd/commands.go#L216-L218
Added lines #L216 - L218 were not covered by tests
[warning] 243-243: contrib/opbot/botmd/commands.go#L243
Added line #L243 was not covered by tests
[warning] 306-309: contrib/opbot/botmd/commands.go#L306-L309
Added lines #L306 - L309 were not covered by tests
[warning] 313-313: contrib/opbot/botmd/commands.go#L313
Added line #L313 was not covered by tests
[warning] 352-352: contrib/opbot/botmd/commands.go#L352
Added line #L352 was not covered by tests
contrib/opbot/botmd/commands.go
Outdated
| res, status, err := b.rfqClient.GetRFQByTxID(ctx.Context(), tx) | ||
| if err != nil { | ||
| b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | ||
| _, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error())) |
There was a problem hiding this comment.
Add unit tests for error handling in rfqLookupCommand
Lines 167-170 introduce new error handling when fetching the quote request using GetRFQByTxID. These lines are not covered by unit tests. Consider adding tests to ensure this error handling behaves as expected and to improve code coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 167-170: contrib/opbot/botmd/commands.go#L167-L170
Added lines #L167 - L170 were not covered by tests
| if status == "Requested" { | ||
| objects = append(objects, &slack.TextBlockObject{ | ||
| Type: slack.MarkdownType, | ||
| Text: "*DestTxHash*: not available", | ||
| }) | ||
| } else { | ||
| //nolint: gosec | ||
| objects = append(objects, &slack.TextBlockObject{ | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, uint32(res.Bridge.DestChainID))), | ||
| }) |
There was a problem hiding this comment.
Add unit tests for conditional handling of DestTxHash
Lines 203-213 introduce conditional logic to handle the presence or absence of DestTxHash based on the status. This conditional logic is not currently covered by unit tests. Consider adding tests to verify that the correct message is displayed for each possible status.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 203-213: contrib/opbot/botmd/commands.go#L203-L213
Added lines #L203 - L213 were not covered by tests
| slackBlocks = append(slackBlocks, slack.NewSectionBlock(nil, objects, nil)) | ||
|
|
||
| _, err = ctx.Response().ReplyBlocks(slackBlocks, slacker.WithUnfurlLinks(false)) |
There was a problem hiding this comment.
Handle errors when sending Slack message blocks
When sending Slack message blocks (lines 216-218), if an error occurs, it is logged but not communicated to the user. Consider notifying the user when message delivery fails to enhance error transparency and user experience.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 216-218: contrib/opbot/botmd/commands.go#L216-L218
Added lines #L216 - L218 were not covered by tests
contrib/opbot/botmd/commands.go
Outdated
| break | ||
| } | ||
| } | ||
| rawRequest, err := b.rfqClient.GetRFQByTxHash(ctx.Context(), tx) |
There was a problem hiding this comment.
Add unit tests for fetching quote request in rfqRefund
Line 243 adds a call to GetRFQByTxHash in the rfqRefund command. This line is not covered by unit tests. Adding tests will help ensure that fetching the quote request and error handling work correctly.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 243-243: contrib/opbot/botmd/commands.go#L243
Added line #L243 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
contrib/opbot/botmd/commands.go (2)
Line range hint
243-287: Improve error handling in refund submission processThe refactored code simplifies the refund process by using a dedicated client and improves overall error handling. However, there's room for improvement in error propagation during the transaction submission:
On lines 278-287, the error from
b.submitter.SubmitTransactionis logged but not propagated to the user. This could lead to silent failures where the user is not informed of the submission error.Suggestion:
nonce, err := b.submitter.SubmitTransaction( ctx.Context(), big.NewInt(int64(rawRequest.Bridge.OriginChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.Bridge.Request)) if err != nil { return nil, fmt.Errorf("error submitting refund: %w", err) } return tx, nil }) if err != nil { - log.Printf("error submitting refund: %v\n", err) - return + b.logger.Errorf(ctx.Context(), "error submitting refund: %v", err) + _, replyErr := ctx.Response().Reply("Error submitting refund. Please try again later or contact support.") + if replyErr != nil { + log.Println(replyErr) + } + return }This change ensures that the user is informed of submission errors while maintaining appropriate error logging for debugging purposes.
Line range hint
324-349: Enhance error wrapping in makeFastBridge functionThe changes in the
makeFastBridgefunction align well with the new RFQ client implementation. The error handling has been improved, but there's an opportunity to enhance error wrapping for better debugging:Consider using
fmt.Errorfwith%wverb consistently for all error wrapping. This allows for better error inspection and handling up the call stack. Apply this change:func (b *Bot) makeFastBridge(ctx context.Context, req *internal.GetRFQByTxIDResponse) (*fastbridge.FastBridge, error) { client, err := rfqClient.NewUnauthenticatedClient(b.handler, b.cfg.RFQApiURL) if err != nil { return nil, fmt.Errorf("error creating rfq client: %w", err) } contracts, err := client.GetRFQContracts(ctx) if err != nil { return nil, fmt.Errorf("error fetching rfq contracts: %w", err) } chainClient, err := b.rpcClient.GetChainClient(ctx, int(req.Bridge.OriginChainID)) if err != nil { return nil, fmt.Errorf("error getting chain client: %w", err) } contractAddress, ok := contracts.Contracts[uint32(req.Bridge.OriginChainID)] if !ok { - return nil, errors.New("contract address not found") + return nil, fmt.Errorf("contract address not found for chain ID %d", req.Bridge.OriginChainID) } fastBridgeHandle, err := fastbridge.NewFastBridge(common.HexToAddress(contractAddress), chainClient) if err != nil { return nil, fmt.Errorf("error creating fast bridge: %w", err) } return fastBridgeHandle, nil }This change provides more context in the error message and maintains consistency in error wrapping throughout the function.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- contrib/opbot/botmd/commands.go (10 hunks)
- contrib/opbot/internal/model.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contrib/opbot/internal/model.go
🧰 Additional context used
🔇 Additional comments (2)
contrib/opbot/botmd/commands.go (2)
23-28: New imports align with refactored RFQ functionalityThe added imports for the internal package and RFQ client are consistent with the changes made to the RFQ lookup and refund functionality. These additions support the new implementation.
355-358: 🛠️ Refactor suggestionImprove hash prefix handling in toExplorerSlackLink
The current implementation only handles the uppercase "0X" prefix. To ensure consistent behavior with different hash formats, consider updating the condition to handle both uppercase and lowercase prefixes.
Apply this diff to address the issue:
-if strings.HasPrefix(rfqHash, "0X") { +if strings.HasPrefix(rfqHash, "0X") || strings.HasPrefix(rfqHash, "0x") { rfqHash = strings.ToLower(rfqHash[2:]) }This change will ensure that both "0X" and "0x" prefixes are handled consistently, improving the robustness of the function.
Likely invalid or redundant comment.
| if status == "Requested" { | ||
| objects = append(objects, &slack.TextBlockObject{ | ||
| Type: slack.MarkdownType, | ||
| Text: "*DestTxHash*: not available", | ||
| }) | ||
| } else { | ||
| //nolint: gosec | ||
| objects = append(objects, &slack.TextBlockObject{ | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, uint32(res.Bridge.DestChainID))), | ||
| }) | ||
| } | ||
|
|
||
| _, err := ctx.Response().ReplyBlocks(slackBlocks, slacker.WithUnfurlLinks(false)) | ||
| slackBlocks = append(slackBlocks, slack.NewSectionBlock(nil, objects, nil)) | ||
|
|
||
| _, err = ctx.Response().ReplyBlocks(slackBlocks, slacker.WithUnfurlLinks(false)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve DestTxHash handling and address potential integer overflow
The conditional logic for handling different RFQ statuses improves the user experience by providing relevant information. However, there are two areas for improvement:
- DRY Principle: The code for adding TextBlockObjects is repeated. Consider refactoring to a helper function:
func addTextBlockObject(objects []*slack.TextBlockObject, text string) []*slack.TextBlockObject {
return append(objects, &slack.TextBlockObject{
Type: slack.MarkdownType,
Text: text,
})
}- Integer Overflow: There's another potential integer overflow when converting
res.Bridge.DestChainIDtouint32on line 212. Apply the same safe casting solution as suggested for the OriginChainID.
Apply these changes to address both issues:
func addTextBlockObject(objects []*slack.TextBlockObject, text string) []*slack.TextBlockObject {
return append(objects, &slack.TextBlockObject{
Type: slack.MarkdownType,
Text: text,
})
}
// In the main function
if status == "Requested" {
- objects = append(objects, &slack.TextBlockObject{
- Type: slack.MarkdownType,
- Text: "*DestTxHash*: not available",
- })
+ objects = addTextBlockObject(objects, "*DestTxHash*: not available")
} else {
- objects = append(objects, &slack.TextBlockObject{
- Type: slack.MarkdownType,
- Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, uint32(res.Bridge.DestChainID))),
- })
+ objects = addTextBlockObject(objects, fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, safeCastToUint32(res.Bridge.DestChainID))))
}These changes will improve code reusability and prevent potential runtime errors due to integer overflow.
| Description: "find a rfq transaction by either tx hash or txid from the rfq-indexer api", | ||
| Examples: []string{ | ||
| "rfq 0x30f96b45ba689c809f7e936c140609eb31c99b182bef54fccf49778716a7e1ca", | ||
| }, | ||
| Handler: func(ctx *slacker.CommandContext) { | ||
| type Status struct { | ||
| relayer string | ||
| *relapi.GetQuoteRequestResponse | ||
| } | ||
|
|
||
| var statuses []Status | ||
| var sliceMux sync.Mutex | ||
|
|
||
| if len(b.cfg.RelayerURLS) == 0 { | ||
| _, err := ctx.Response().Reply("no relayer urls configured") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| tx := stripLinks(ctx.Request().Param("tx")) | ||
|
|
||
| var wg sync.WaitGroup | ||
| // 2 routines per relayer, one for tx hashh one for tx id | ||
| wg.Add(len(b.cfg.RelayerURLS) * 2) | ||
| for _, relayer := range b.cfg.RelayerURLS { | ||
| client := relapi.NewRelayerClient(b.handler, relayer) | ||
| go func() { | ||
| defer wg.Done() | ||
| res, err := client.GetQuoteRequestByTxHash(ctx.Context(), tx) | ||
| if err != nil { | ||
| log.Printf("error fetching quote request status by tx hash: %v\n", err) | ||
| return | ||
| } | ||
| sliceMux.Lock() | ||
| defer sliceMux.Unlock() | ||
| statuses = append(statuses, Status{relayer: relayer, GetQuoteRequestResponse: res}) | ||
| }() | ||
|
|
||
| go func() { | ||
| defer wg.Done() | ||
| res, err := client.GetQuoteRequestByTXID(ctx.Context(), tx) | ||
| if err != nil { | ||
| log.Printf("error fetching quote request status by tx id: %v\n", err) | ||
| return | ||
| } | ||
| sliceMux.Lock() | ||
| defer sliceMux.Unlock() | ||
| statuses = append(statuses, Status{relayer: relayer, GetQuoteRequestResponse: res}) | ||
| }() | ||
| } | ||
| wg.Wait() | ||
|
|
||
| if len(statuses) == 0 { | ||
| _, err := ctx.Response().Reply("no quote request found") | ||
| res, status, err := b.rfqClient.GetRFQ(ctx.Context(), tx) | ||
| if err != nil { | ||
| b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | ||
| _, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error())) |
There was a problem hiding this comment.
Improve error message handling in rfqLookupCommand
The refactored code simplifies the RFQ lookup process by using a dedicated client, which is a good improvement. However, there's a potential issue with error message construction:
On line 170, the error message directly includes err.Error(). This could lead to leaking sensitive information or presenting unclear messages to the user. Consider using a more generic error message or sanitizing the error before including it in the response.
Suggestion:
- _, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error()))
+ _, err := ctx.Response().Reply("Error fetching quote request. Please try again later or contact support.")Also, consider logging the detailed error for debugging purposes while keeping the user-facing message generic.
📝 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: "find a rfq transaction by either tx hash or txid from the rfq-indexer api", | |
| Examples: []string{ | |
| "rfq 0x30f96b45ba689c809f7e936c140609eb31c99b182bef54fccf49778716a7e1ca", | |
| }, | |
| Handler: func(ctx *slacker.CommandContext) { | |
| type Status struct { | |
| relayer string | |
| *relapi.GetQuoteRequestResponse | |
| } | |
| var statuses []Status | |
| var sliceMux sync.Mutex | |
| if len(b.cfg.RelayerURLS) == 0 { | |
| _, err := ctx.Response().Reply("no relayer urls configured") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } | |
| tx := stripLinks(ctx.Request().Param("tx")) | |
| var wg sync.WaitGroup | |
| // 2 routines per relayer, one for tx hashh one for tx id | |
| wg.Add(len(b.cfg.RelayerURLS) * 2) | |
| for _, relayer := range b.cfg.RelayerURLS { | |
| client := relapi.NewRelayerClient(b.handler, relayer) | |
| go func() { | |
| defer wg.Done() | |
| res, err := client.GetQuoteRequestByTxHash(ctx.Context(), tx) | |
| if err != nil { | |
| log.Printf("error fetching quote request status by tx hash: %v\n", err) | |
| return | |
| } | |
| sliceMux.Lock() | |
| defer sliceMux.Unlock() | |
| statuses = append(statuses, Status{relayer: relayer, GetQuoteRequestResponse: res}) | |
| }() | |
| go func() { | |
| defer wg.Done() | |
| res, err := client.GetQuoteRequestByTXID(ctx.Context(), tx) | |
| if err != nil { | |
| log.Printf("error fetching quote request status by tx id: %v\n", err) | |
| return | |
| } | |
| sliceMux.Lock() | |
| defer sliceMux.Unlock() | |
| statuses = append(statuses, Status{relayer: relayer, GetQuoteRequestResponse: res}) | |
| }() | |
| } | |
| wg.Wait() | |
| if len(statuses) == 0 { | |
| _, err := ctx.Response().Reply("no quote request found") | |
| res, status, err := b.rfqClient.GetRFQ(ctx.Context(), tx) | |
| if err != nil { | |
| b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | |
| _, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error())) | |
| Description: "find a rfq transaction by either tx hash or txid from the rfq-indexer api", | |
| Examples: []string{ | |
| "rfq 0x30f96b45ba689c809f7e936c140609eb31c99b182bef54fccf49778716a7e1ca", | |
| }, | |
| Handler: func(ctx *slacker.CommandContext) { | |
| tx := stripLinks(ctx.Request().Param("tx")) | |
| res, status, err := b.rfqClient.GetRFQ(ctx.Context(), tx) | |
| if err != nil { | |
| b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | |
| _, err := ctx.Response().Reply("Error fetching quote request. Please try again later or contact support.") |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- contrib/opbot/botmd/commands.go (9 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 160-160: contrib/opbot/botmd/commands.go#L160
Added line #L160 was not covered by tests
[warning] 167-170: contrib/opbot/botmd/commands.go#L167-L170
Added lines #L167 - L170 were not covered by tests
[warning] 179-201: contrib/opbot/botmd/commands.go#L179-L201
Added lines #L179 - L201 were not covered by tests
[warning] 203-213: contrib/opbot/botmd/commands.go#L203-L213
Added lines #L203 - L213 were not covered by tests
[warning] 216-218: contrib/opbot/botmd/commands.go#L216-L218
Added lines #L216 - L218 were not covered by tests
[warning] 243-243: contrib/opbot/botmd/commands.go#L243
Added line #L243 was not covered by tests
[warning] 262-262: contrib/opbot/botmd/commands.go#L262
Added line #L262 was not covered by tests
[warning] 278-286: contrib/opbot/botmd/commands.go#L278-L286
Added lines #L278 - L286 were not covered by tests
[warning] 297-297: contrib/opbot/botmd/commands.go#L297
Added line #L297 was not covered by tests
[warning] 309-312: contrib/opbot/botmd/commands.go#L309-L312
Added lines #L309 - L312 were not covered by tests
[warning] 316-316: contrib/opbot/botmd/commands.go#L316
Added line #L316 was not covered by tests
[warning] 324-324: contrib/opbot/botmd/commands.go#L324
Added line #L324 was not covered by tests
[warning] 335-335: contrib/opbot/botmd/commands.go#L335
Added line #L335 was not covered by tests
[warning] 340-340: contrib/opbot/botmd/commands.go#L340
Added line #L340 was not covered by tests
[warning] 355-355: contrib/opbot/botmd/commands.go#L355
Added line #L355 was not covered by tests
🪛 GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 316-316:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 340-340:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 335-335:
unnecessary conversion (unconvert)
🔇 Additional comments (3)
contrib/opbot/botmd/commands.go (3)
23-23: LGTM: Good use of internal packageThe addition of the internal package import is a positive change, promoting better code organization and encapsulation.
Line range hint
1-378: LGTM: Improved code structure and consistencyThe refactoring to use the new RFQ client and the consistent use of Slack block objects for message construction have improved the overall code structure and readability. The error handling has been updated consistently throughout the file, which is a positive change.
These changes make the code more maintainable and easier to understand. Good job on the refactoring effort!
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 179-201: contrib/opbot/botmd/commands.go#L179-L201
Added lines #L179 - L201 were not covered by tests
[warning] 203-213: contrib/opbot/botmd/commands.go#L203-L213
Added lines #L203 - L213 were not covered by tests
[warning] 216-218: contrib/opbot/botmd/commands.go#L216-L218
Added lines #L216 - L218 were not covered by tests
Line range hint
160-358: Improve test coverage for new and modified codeThe static analysis indicates that many of the new and modified lines of code are not covered by tests. This includes critical parts of the
rfqLookupCommand,rfqRefund, andmakeFastBridgefunctions.To improve the overall quality and reliability of the code, please add or update unit tests to cover the new functionality. This will help prevent regressions and make future refactoring easier.
Here's a shell script to check the current test coverage:
Please run this script and aim to increase the coverage, particularly for the modified functions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 179-201: contrib/opbot/botmd/commands.go#L179-L201
Added lines #L179 - L201 were not covered by tests
[warning] 203-213: contrib/opbot/botmd/commands.go#L203-L213
Added lines #L203 - L213 were not covered by tests
[warning] 216-218: contrib/opbot/botmd/commands.go#L216-L218
Added lines #L216 - L218 were not covered by tests
| { | ||
| Type: slack.MarkdownType, | ||
| //nolint: gosec | ||
| Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, uint32(res.Bridge.OriginChainID))), |
There was a problem hiding this comment.
Potential integer overflow when converting chain ID
The conversion of res.Bridge.OriginChainID to uint32 could lead to an integer overflow if the chain ID exceeds the maximum value of uint32 (4,294,967,295).
Consider implementing a safe conversion function:
func safeUint32(v int) uint32 {
if v < 0 || v > math.MaxUint32 {
// Handle error or return a default value
return 0
}
return uint32(v)
}Then use it in the function:
- Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, uint32(res.Bridge.OriginChainID))),
+ Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, safeUint32(res.Bridge.OriginChainID))),| if err != nil { | ||
| return nil, fmt.Errorf("error getting chain client: %w", err) | ||
| } | ||
|
|
||
| contractAddress, ok := contracts.Contracts[req.OriginChainID] | ||
| contractAddress, ok := contracts.Contracts[uint32(req.Bridge.OriginChainID)] |
There was a problem hiding this comment.
Potential integer overflow when accessing contract address
The conversion of req.Bridge.OriginChainID to uint32 could lead to an integer overflow if the chain ID exceeds the maximum value of uint32 (4,294,967,295).
Consider using a safe conversion function similar to the one suggested earlier:
- contractAddress, ok := contracts.Contracts[uint32(req.Bridge.OriginChainID)]
+ contractAddress, ok := contracts.Contracts[safeUint32(req.Bridge.OriginChainID)]Where safeUint32 is defined as:
func safeUint32(v int) uint32 {
if v < 0 || v > math.MaxUint32 {
// Handle error or return a default value
return 0
}
return uint32(v)
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 340-340: contrib/opbot/botmd/commands.go#L340
Added line #L340 was not covered by tests
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 340-340:
G115: integer overflow conversion int -> uint32 (gosec)
| if strings.HasPrefix(rfqHash, "0X") { | ||
| rfqHash = strings.ToLower(rfqHash[2:]) | ||
| } | ||
|
|
There was a problem hiding this comment.
Improve hash prefix handling in toExplorerSlackLink
The current implementation only handles the uppercase "0X" prefix. To ensure consistent behavior with different hash formats, consider updating the condition to handle both uppercase and lowercase prefixes.
Apply this diff to address the issue:
- if strings.HasPrefix(rfqHash, "0X") {
+ if strings.HasPrefix(rfqHash, "0X") || strings.HasPrefix(rfqHash, "0x") {
rfqHash = strings.ToLower(rfqHash[2:])
}This change will ensure that both "0X" and "0x" prefixes are handled consistently.
📝 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.
| if strings.HasPrefix(rfqHash, "0X") { | |
| rfqHash = strings.ToLower(rfqHash[2:]) | |
| } | |
| if strings.HasPrefix(rfqHash, "0X") || strings.HasPrefix(rfqHash, "0x") { | |
| rfqHash = strings.ToLower(rfqHash[2:]) | |
| } | |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 355-355: contrib/opbot/botmd/commands.go#L355
Added line #L355 was not covered by tests
| _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } |
There was a problem hiding this comment.
Improve error handling after refund submission
The current implementation sends a success message even when an error occurs during the status check. This could mislead users about the state of their refund.
Consider modifying the error handling to provide more accurate feedback:
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))
+ _, err := ctx.Response().Reply(fmt.Sprintf("Refund submitted with nonce %d, but there was an error confirming the status. Please check later.", nonce))
if err != nil {
log.Println(err)
}
return
}Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 309-312: contrib/opbot/botmd/commands.go#L309-L312
Added lines #L309 - L312 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
contrib/opbot/botmd/commands.go (1)
Line range hint
297-301: Improve error handling in retry block.The error handling combines both nil check and HasTx check in a single error message, making it harder to diagnose issues.
Apply this diff to improve error handling:
status, err = b.submitter.GetSubmissionStatus(ctx, big.NewInt(int64(rawRequest.Bridge.OriginChainID)), nonce) -if err != nil || !status.HasTx() { - b.logger.Errorf(ctx, "error fetching quote request: %v", err) - return fmt.Errorf("error fetching quote request: %w", err) +if err != nil { + b.logger.Errorf(ctx, "failed to get submission status: %v", err) + return fmt.Errorf("failed to get submission status: %w", err) +} +if !status.HasTx() { + return errors.New("transaction not found in submission status") }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- contrib/opbot/botmd/commands.go (10 hunks)
- services/rfq/relayer/quoter/quoter_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/quoter/quoter_test.go
🧰 Additional context used
🪛 GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 316-316:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 340-340:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 335-335:
unnecessary conversion (unconvert)
🔇 Additional comments (1)
contrib/opbot/botmd/commands.go (1)
340-340:⚠️ Potential issueUse safe chain ID conversion in contract lookup.
Direct conversion of chain ID to uint32 in map lookup could cause integer overflow.
Apply this diff:
-contractAddress, ok := contracts.Contracts[uint32(req.Bridge.OriginChainID)] +contractAddress, ok := contracts.Contracts[safeChainIDToUint32(req.Bridge.OriginChainID)]Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 340-340:
G115: integer overflow conversion int -> uint32 (gosec)
| //nolint: gosec | ||
| Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, uint32(res.Bridge.OriginChainID))), |
There was a problem hiding this comment.
Implement safe chain ID conversion to prevent integer overflow.
The direct conversion of chain IDs to uint32 could cause integer overflow if the values exceed 4,294,967,295.
Add a safe conversion function and use it:
+func safeChainIDToUint32(chainID int) uint32 {
+ if chainID < 0 || chainID > math.MaxUint32 {
+ // Log the error and return a default value
+ return 0
+ }
+ return uint32(chainID)
+}
// Use in both places:
-Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, uint32(res.Bridge.OriginChainID))),
+Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, safeChainIDToUint32(res.Bridge.OriginChainID))),
-Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, uint32(res.Bridge.DestChainID))),
+Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(res.BridgeRelay.TransactionHash, safeChainIDToUint32(res.Bridge.DestChainID))),Also applies to: 209-212
| return | ||
| } | ||
|
|
||
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toExplorerSlackLink(status.TxHash().String()))) | ||
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toTXSlackLink(status.TxHash().String(), uint32(rawRequest.Bridge.OriginChainID)))) |
There was a problem hiding this comment.
Use safe chain ID conversion in toTXSlackLink call.
Similar to previous occurrences, direct conversion of chain ID to uint32 could cause integer overflow.
Apply this diff:
-_, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toTXSlackLink(status.TxHash().String(), uint32(rawRequest.Bridge.OriginChainID))))
+_, err = ctx.Response().Reply(fmt.Sprintf("refund submitted: %s", toTXSlackLink(status.TxHash().String(), safeChainIDToUint32(rawRequest.Bridge.OriginChainID))))Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 316-316:
G115: integer overflow conversion int -> uint32 (gosec)
| res, status, err := b.rfqClient.GetRFQ(ctx.Context(), tx) | ||
| if err != nil { | ||
| b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | ||
| _, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error())) |
There was a problem hiding this comment.
Improve error handling to avoid exposing sensitive information.
The current implementation directly exposes the error message to users, which could potentially leak sensitive information.
Apply this diff to improve error handling:
if err != nil {
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err)
- _, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error()))
+ _, err := ctx.Response().Reply("Failed to fetch quote request. Please try again later.")
if err != nil {
log.Println(err)
}
return
}📝 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.
| res, status, err := b.rfqClient.GetRFQ(ctx.Context(), tx) | |
| if err != nil { | |
| b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | |
| _, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error())) | |
| res, status, err := b.rfqClient.GetRFQ(ctx.Context(), tx) | |
| if err != nil { | |
| b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err) | |
| _, err := ctx.Response().Reply("Failed to fetch quote request. Please try again later.") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
contrib/opbot/internal/client.go (2)
1-2: Enhance package documentation.
Consider adding more comprehensive package documentation that describes:
- The purpose of the RFQ client
- Key components and responsibilities
- Usage examples or guidelines
-// Package internal provides the RFQ client implementation.
+// Package internal provides the RFQ (Request for Quote) client implementation.
+// It offers functionality to interact with RFQ services through a clean interface,
+// supporting operations such as retrieving quote requests by transaction ID or hash.
+// The package handles communication with both the indexer and relayer services.16-18: Make the route constant name more descriptive.
The current constant name getRFQRoute could be more specific about its purpose.
- getRFQRoute = "/transaction-id/%s"
+ getRFQByTxIDRoute = "/transaction-id/%s"📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- contrib/opbot/internal/client.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/internal/client.go
[warning] 32-42: contrib/opbot/internal/client.go#L32-L42
Added lines #L32 - L42 were not covered by tests
[warning] 44-47: contrib/opbot/internal/client.go#L44-L47
Added lines #L44 - L47 were not covered by tests
[warning] 51-58: contrib/opbot/internal/client.go#L51-L58
Added lines #L51 - L58 were not covered by tests
[warning] 60-62: contrib/opbot/internal/client.go#L60-L62
Added lines #L60 - L62 were not covered by tests
[warning] 64-75: contrib/opbot/internal/client.go#L64-L75
Added lines #L64 - L75 were not covered by tests
[warning] 78-78: contrib/opbot/internal/client.go#L78
Added line #L78 was not covered by tests
🔇 Additional comments (1)
contrib/opbot/internal/client.go (1)
51-79: 🛠️ Refactor suggestion
Add timeout and response validation.
The implementation could be improved in several ways:
- Add context timeout for the request
- Validate the response body
- Add comprehensive test coverage
func (r *rfqClientImpl) GetRFQ(ctx context.Context, txIdentifier string) (*GetRFQByTxIDResponse, string, error) {
+ if txIdentifier == "" {
+ return nil, "", fmt.Errorf("transaction identifier cannot be empty")
+ }
+
+ // Add timeout to prevent long-running requests
+ ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
+
var res GetRFQByTxIDResponse
resp, err := r.client.R().SetContext(ctx).
SetResult(&res).
Get(fmt.Sprintf(getRFQRoute, txIdentifier))
if err != nil {
return nil, "", fmt.Errorf("failed to get quote request by tx ID: %w", err)
}
if resp.StatusCode() != http.StatusOK {
- return nil, "", fmt.Errorf("unexpected status code: %d", resp.StatusCode())
+ return nil, "", fmt.Errorf("unexpected status code: %d, body: %s", resp.StatusCode(), resp.String())
}Consider adding the following test cases:
- Test successful request
- Test timeout scenario
- Test various status responses
- Test error conditions
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 51-58: contrib/opbot/internal/client.go#L51-L58
Added lines #L51 - L58 were not covered by tests
[warning] 60-62: contrib/opbot/internal/client.go#L60-L62
Added lines #L60 - L62 were not covered by tests
[warning] 64-75: contrib/opbot/internal/client.go#L64-L75
Added lines #L64 - L75 were not covered by tests
[warning] 78-78: contrib/opbot/internal/client.go#L78
Added line #L78 was not covered by tests
contrib/opbot/internal/client.go
Outdated
| func NewRFQClient(handler metrics.Handler, indexerURL string, relayerURLs []string) RFQClient { | ||
| client := resty.New() | ||
| client.SetBaseURL(indexerURL) | ||
| client.SetHeader(headers.UserAgent, "rfq-client") | ||
|
|
||
| otelresty.TraceClient(client, otelresty.WithTracerProvider(handler.GetTracerProvider())) | ||
|
|
||
| var relayerClients []relapi.RelayerClient | ||
| for _, url := range relayerURLs { | ||
| relayerClients = append(relayerClients, relapi.NewRelayerClient(handler, url)) | ||
| } | ||
|
|
||
| return &rfqClientImpl{ | ||
| client: client, | ||
| relayerClients: relayerClients, | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add input validation and test coverage for RFQClient constructor
The review comment's suggestions are valid as:
- There are no tests for the
NewRFQClientconstructor in the package - URL validation is a common practice in the codebase, as seen in other components that panic on empty inputs
- The constructor lacks validation for critical URL parameters that could cause runtime issues if empty
Required changes:
- Add input validation as suggested in the review
- Create unit tests for the constructor in
contrib/opbot/internal/client_test.go
🔗 Analysis chain
Add input validation and error handling.
The constructor should validate its inputs to ensure neither the indexer URL nor relayer URLs are empty.
func NewRFQClient(handler metrics.Handler, indexerURL string, relayerURLs []string) RFQClient {
+ if indexerURL == "" {
+ panic("indexer URL cannot be empty")
+ }
+ if len(relayerURLs) == 0 {
+ panic("at least one relayer URL must be provided")
+ }
+
client := resty.New()Additionally, consider adding unit tests to cover the constructor's functionality:
- Test with valid inputs
- Test with invalid inputs
- Verify client configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any test files for this package
find . -name "*_test.go" -type f -exec grep -l "TestNewRFQClient" {} \;
Length of output: 74
Script:
#!/bin/bash
# Let's first check if there are any test files in the package directory
fd "_test.go" contrib/opbot/internal/
# Then let's check the current test coverage for client.go
rg -l "NewRFQClient" contrib/opbot/internal/
Length of output: 115
Script:
#!/bin/bash
# Let's check if there are any existing tests in the package and their patterns
rg "func Test" contrib/opbot/internal/ -A 2
# Also check if there are any URL validations in other constructors
ast-grep --pattern 'if $_ == "" {
panic($_)
}'
Length of output: 477
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-42: contrib/opbot/internal/client.go#L32-L42
Added lines #L32 - L42 were not covered by tests
[warning] 44-47: contrib/opbot/internal/client.go#L44-L47
Added lines #L44 - L47 were not covered by tests
| ChainID: 480, | ||
| ChainName: "world chain", | ||
| Explorer: "https://worldscan.org", |
There was a problem hiding this comment.
Standardize chain name format for consistency
The chain name "world chain" contains a space, which deviates from the naming pattern used in other entries (e.g., "ethereum", "arbitrum"). This might cause issues with API integrations or string matching.
Consider updating to remove the space:
- ChainName: "world chain",
+ ChainName: "worldchain",📝 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.
| ChainID: 480, | |
| ChainName: "world chain", | |
| Explorer: "https://worldscan.org", | |
| ChainID: 480, | |
| ChainName: "worldchain", | |
| Explorer: "https://worldscan.org", |
| { | ||
| ChainID: 59144, | ||
| ChainName: "linea", | ||
| Explorer: "https://lineascan.build", | ||
| }, | ||
| { | ||
| ChainID: 480, | ||
| ChainName: "world chain", | ||
| Explorer: "https://worldscan.org", | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Found duplicate entries with ChainID 66:
okexchainwith explorer URL "https://www.oklink.com/okexchain"okcwith explorer URL "https://www.oklink.com/en/okc"
This appears to be the same chain with different names (OKX Chain/OKC), but having duplicate entries could cause confusion or issues. Consider consolidating these entries into a single one with the most current chain name and explorer URL.
🔗 Analysis chain
LGTM! Chain metadata entries are well-structured
The new entries for Linea and World Chain follow the correct structure and include all required fields. The chain IDs and explorer URLs are properly formatted.
Let's verify there are no duplicate chain IDs:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate chain IDs in the file
# Expect: No duplicate chain IDs should be found
rg -o 'ChainID:\s+\d+' ethergo/chaindata/chaindata.go | sort | uniq -d
Length of output: 86
Script:
#!/bin/bash
# Let's get the full context of both occurrences of ChainID 66
rg -B2 -A2 'ChainID:\s+66' ethergo/chaindata/chaindata.go
Length of output: 251
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
contrib/opbot/botmd/commands.go (2)
160-160: Update command description to be more descriptiveThe command description could be more informative about what the command actually does (e.g., retrieving and displaying RFQ transaction details).
- Description: "find a rfq transaction by either tx hash or txid from the rfq-indexer api", + Description: "retrieve and display detailed information about an RFQ transaction using either transaction hash or ID",
Line range hint
324-328: Consider caching the RFQ clientThe function creates a new unauthenticated RFQ client on every call, which could be inefficient for frequent requests. Consider creating the client once during bot initialization and reusing it.
type Bot struct { // ... existing fields ... + rfqUnauthClient *rfqClient.Client } func NewBot(...) *Bot { // ... existing initialization ... + unauthClient, _ := rfqClient.NewUnauthenticatedClient(handler, cfg.RFQApiURL) + return &Bot{ // ... existing fields ... + rfqUnauthClient: unauthClient, } } func (b *Bot) makeFastBridge(ctx context.Context, req *internal.GetRFQByTxIDResponse) (*fastbridge.FastBridge, error) { - client, err := rfqClient.NewUnauthenticatedClient(b.handler, b.cfg.RFQApiURL) - if err != nil { - return nil, fmt.Errorf("error creating rfq client: %w", err) - } - - contracts, err := client.GetRFQContracts(ctx) + contracts, err := b.rfqUnauthClient.GetRFQContracts(ctx)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
contrib/opbot/botmd/botmd.go(3 hunks)contrib/opbot/botmd/commands.go(10 hunks)contrib/opbot/internal/client.go(1 hunks)
🔇 Additional comments (5)
contrib/opbot/botmd/botmd.go (4)
9-9: LGTM: Import added for internal package
The new import is correctly added to support the RFQ client integration.
33-33: LGTM: RFQ client field added to Bot struct
The new rfqClient field is properly integrated into the Bot struct with appropriate type.
47-50: LGTM: Improved struct initialization formatting
The struct initialization is now more readable with explicit field names.
51-51: Verify RFQIndexerAPIURL configuration
Please ensure:
- The
RFQIndexerAPIURLfield exists in the config struct - The URL is properly validated before being used
✅ Verification successful
Let me check for URL validation in the RFQ client initialization.
Let me try searching for the RFQ client implementation with a different approach.
RFQIndexerAPIURL configuration is properly handled
The verification shows that:
- The
RFQIndexerAPIURLfield exists in the config struct with proper YAML binding - The URL is used correctly in
NewRFQClientwhere it's set as the base URL for the REST client - Basic validation is inherently handled by the resty client's
SetBaseURLmethod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify config field and validation
# Test 1: Check if RFQIndexerAPIURL exists in config
echo "Checking config struct..."
rg --type go "type\s+Config\s+struct" -A 20 config/config.go
# Test 2: Check for URL validation
echo "Checking URL validation..."
rg --type go "RFQIndexerAPIURL" -A 5 -B 5
Length of output: 1913
Script:
#!/bin/bash
# Check RFQ client initialization and URL validation
ast-grep --pattern 'func NewRFQClient($_, $_) {
$$$
}'
Length of output: 58
Script:
#!/bin/bash
# Search for RFQ client implementation
echo "Searching for NewRFQClient implementation..."
rg --type go "func NewRFQClient" -A 10
echo "Searching for RFQ client URL usage..."
rg --type go "RFQIndexerAPIURL" -A 10 contrib/opbot/internal/
Length of output: 967
contrib/opbot/internal/client.go (1)
30-34: Add input validation for 'indexerURL' in 'NewRFQClient'.
Ensure that the indexerURL parameter is not empty to prevent potential runtime errors when initializing the client.
contrib/opbot/botmd/commands.go
Outdated
| func (b *Bot) rfqRefund() *slacker.CommandDefinition { | ||
| return &slacker.CommandDefinition{ | ||
| Command: "refund <tx>", | ||
| Description: "refund a quote request", | ||
| Description: "TESTING TESTING a quote request", |
There was a problem hiding this comment.
Update command description to reflect production status
The current description suggests this is a testing command ("TESTING TESTING"), but the implementation appears to be production-ready.
- Description: "TESTING TESTING a quote request",
+ Description: "process a refund for the specified quote request transaction",📝 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 a quote request", | |
| Description: "process a refund for the specified quote request transaction", |
| func NewRFQClient(handler metrics.Handler, indexerURL string) RFQClient { | ||
| client := resty.New() | ||
| client.SetBaseURL(indexerURL) | ||
| client.SetHeader(headers.UserAgent, "rfq-client") | ||
|
|
||
| otelresty.TraceClient(client, otelresty.WithTracerProvider(handler.GetTracerProvider())) | ||
|
|
||
| return &rfqClientImpl{ | ||
| client: client, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for 'NewRFQClient' constructor.
To improve test coverage and ensure reliability, consider adding unit tests for the NewRFQClient function, covering cases such as valid and invalid inputs.
| func (r *rfqClientImpl) GetRFQ(ctx context.Context, txIdentifier string) (*GetRFQByTxIDResponse, string, error) { | ||
| var res GetRFQByTxIDResponse | ||
| resp, err := r.client.R().SetContext(ctx). | ||
| SetResult(&res). | ||
| Get(fmt.Sprintf(getRFQRoute, txIdentifier)) | ||
| if err != nil { | ||
| return nil, "", fmt.Errorf("failed to get quote request by tx ID: %w", err) | ||
| } | ||
|
|
||
| if resp.StatusCode() != http.StatusOK { | ||
| return nil, "", fmt.Errorf("unexpected status code: %d", resp.StatusCode()) | ||
| } | ||
|
|
||
| var status string | ||
| switch { | ||
| case res.BridgeClaim != (BridgeClaim{}): | ||
| status = "Claimed" | ||
| case res.BridgeProof != (BridgeProof{}): | ||
| status = "Proven" | ||
| case res.BridgeRelay != (BridgeRelay{}): | ||
| status = "Relayed" | ||
| case res.BridgeRequest != (BridgeRequest{}): | ||
| status = "Requested" | ||
| default: | ||
| status = "Unknown" | ||
| } | ||
|
|
||
| return &res, status, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for 'GetRFQ' method.
The GetRFQ method is essential for fetching quote requests. Adding unit tests for this method will enhance code quality and reliability.
| SetResult(&res). | ||
| Get(fmt.Sprintf(getRFQRoute, txIdentifier)) | ||
| if err != nil { | ||
| return nil, "", fmt.Errorf("failed to get quote request by tx ID: %w", err) |
There was a problem hiding this comment.
Clarify the error message in 'GetRFQ' method.
The error message specifies "tx ID", but txIdentifier can be a transaction ID or transaction hash. Consider updating the message for accuracy.
Apply this diff to fix the error message:
-return nil, "", fmt.Errorf("failed to get quote request by tx ID: %w", err)
+return nil, "", fmt.Errorf("failed to get quote request by transaction identifier: %w", 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.
| return nil, "", fmt.Errorf("failed to get quote request by tx ID: %w", err) | |
| return nil, "", fmt.Errorf("failed to get quote request by transaction identifier: %w", err) |
| } | ||
| } | ||
|
|
||
| // GetRFQByTxID gets a quote request by transaction ID or transaction hash. |
There was a problem hiding this comment.
Update the function comment to match the method name.
The comment refers to GetRFQByTxID, but the method is named GetRFQ. Please update the comment for consistency.
Apply this diff to fix the comment:
-// GetRFQByTxID gets a quote request by transaction ID or transaction hash.
+// GetRFQ gets a quote request by transaction ID or transaction hash.📝 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.
| // GetRFQByTxID gets a quote request by transaction ID or transaction hash. | |
| // GetRFQ gets a quote request by transaction ID or transaction hash. |
There was a problem hiding this comment.
I believe this has been live for a while, so let's get this meeges once the lint issues are resolved.
#3375 can then be updated with latest master and will therefore be ready for staging
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
contrib/opbot/botmd/commands.go (1)
Line range hint
325-342: Consider caching RFQ contractsThe
makeFastBridgefunction fetches RFQ contracts on every call, which could be inefficient for frequent requests.Consider implementing a cache for the RFQ contracts with an appropriate TTL to reduce API calls:
type contractCache struct { contracts map[uint32]string expiry time.Time mu sync.RWMutex } func (b *Bot) getCachedContracts(ctx context.Context) (map[uint32]string, error) { b.contractCache.mu.RLock() if time.Now().Before(b.contractCache.expiry) { defer b.contractCache.mu.RUnlock() return b.contractCache.contracts, nil } b.contractCache.mu.RUnlock() // Fetch new contracts client, err := rfqClient.NewUnauthenticatedClient(b.handler, b.cfg.RFQApiURL) if err != nil { return nil, err } contracts, err := client.GetRFQContracts(ctx) if err != nil { return nil, err } b.contractCache.mu.Lock() defer b.contractCache.mu.Unlock() b.contractCache.contracts = contracts.Contracts b.contractCache.expiry = time.Now().Add(1 * time.Hour) return contracts.Contracts, nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 342-342: contrib/opbot/botmd/commands.go#L342
Added line #L342 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
contrib/opbot/botmd/commands.go(10 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 160-160: contrib/opbot/botmd/commands.go#L160
Added line #L160 was not covered by tests
[warning] 167-170: contrib/opbot/botmd/commands.go#L167-L170
Added lines #L167 - L170 were not covered by tests
[warning] 179-201: contrib/opbot/botmd/commands.go#L179-L201
Added lines #L179 - L201 were not covered by tests
[warning] 203-213: contrib/opbot/botmd/commands.go#L203-L213
Added lines #L203 - L213 were not covered by tests
[warning] 216-218: contrib/opbot/botmd/commands.go#L216-L218
Added lines #L216 - L218 were not covered by tests
[warning] 243-243: contrib/opbot/botmd/commands.go#L243
Added line #L243 was not covered by tests
[warning] 262-262: contrib/opbot/botmd/commands.go#L262
Added line #L262 was not covered by tests
[warning] 278-286: contrib/opbot/botmd/commands.go#L278-L286
Added lines #L278 - L286 were not covered by tests
[warning] 297-297: contrib/opbot/botmd/commands.go#L297
Added line #L297 was not covered by tests
[warning] 309-312: contrib/opbot/botmd/commands.go#L309-L312
Added lines #L309 - L312 were not covered by tests
[warning] 317-317: contrib/opbot/botmd/commands.go#L317
Added line #L317 was not covered by tests
[warning] 325-325: contrib/opbot/botmd/commands.go#L325
Added line #L325 was not covered by tests
[warning] 342-342: contrib/opbot/botmd/commands.go#L342
Added line #L342 was not covered by tests
[warning] 357-357: contrib/opbot/botmd/commands.go#L357
Added line #L357 was not covered by tests
🔇 Additional comments (3)
contrib/opbot/botmd/commands.go (3)
23-23: LGTM: New import for internal package
The addition of the internal package import aligns with the refactoring to use the GraphQL API.
357-360: 🛠️ Refactor suggestion
Improve hash prefix handling
The current implementation only handles the uppercase "0X" prefix.
Apply this diff to handle both uppercase and lowercase prefixes consistently:
-if strings.HasPrefix(rfqHash, "0X") {
+if strings.HasPrefix(rfqHash, "0X") || strings.HasPrefix(rfqHash, "0x") {
rfqHash = strings.ToLower(rfqHash[2:])
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 357-357: contrib/opbot/botmd/commands.go#L357
Added line #L357 was not covered by tests
160-170:
Add error context and improve error handling
The error message directly exposes internal error details to users, which could leak sensitive information.
Apply this diff to improve error handling:
if err != nil {
b.logger.Errorf(ctx.Context(), "error fetching quote request: %v", err)
- _, err := ctx.Response().Reply(fmt.Sprintf("error fetching quote request %s", err.Error()))
+ _, err := ctx.Response().Reply("Failed to fetch quote request. Please try again later.")
if err != nil {
log.Println(err)
}
return
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 160-160: contrib/opbot/botmd/commands.go#L160
Added line #L160 was not covered by tests
[warning] 167-170: contrib/opbot/botmd/commands.go#L167-L170
Added lines #L167 - L170 were not covered by tests
| nonce, err := b.submitter.SubmitTransaction( | ||
| ctx.Context(), | ||
| big.NewInt(int64(rawRequest.Bridge.OriginChainID)), | ||
| func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.Bridge.Request[2:])) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error submitting refund: %w", err) | ||
| } | ||
| return tx, nil | ||
| }) |
There was a problem hiding this comment.
Add error handling for transaction submission
The function returns early on error without notifying the user about the transaction submission failure.
Apply this diff to improve error handling:
if err != nil {
log.Printf("error submitting refund: %v\n", err)
+ _, replyErr := ctx.Response().Reply("Failed to submit refund transaction. Please try again later.")
+ if replyErr != nil {
+ log.Println(replyErr)
+ }
return
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 278-286: contrib/opbot/botmd/commands.go#L278-L286
Added lines #L278 - L286 were not covered by tests
| objects := []*slack.TextBlockObject{ | ||
| { | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*Relayer*: %s", res.BridgeRelay.Relayer), | ||
| }, | ||
| { | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*Status*: %s", status), | ||
| }, | ||
| { | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*TxID*: %s", toExplorerSlackLink(res.Bridge.TransactionID)), | ||
| }, | ||
| { | ||
| Type: slack.MarkdownType, | ||
| //nolint: gosec | ||
| Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(res.BridgeRequest.TransactionHash, uint32(res.Bridge.OriginChainID))), | ||
| }, | ||
| { | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*Estimated Tx Age*: %s", humanize.Time(time.Unix(res.BridgeRelay.BlockTimestamp, 0))), | ||
| }, | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add unit tests for Slack message construction and helper functions
The codebase currently lacks test coverage for Slack message construction and its helper functions. Specifically:
- No tests exist for the Slack message block construction logic
- Helper functions
toExplorerSlackLinkandtoTXSlackLinkare untested - Only a basic
StripLinksutility function is tested
Key areas that need test coverage:
- Message block construction with different bridge relay statuses
- Link formatting for transaction hashes and explorer URLs
- Edge cases for transaction hash formatting and chain IDs
🔗 Analysis chain
Add unit tests for Slack message construction
The new Slack message construction logic lacks test coverage, which could lead to undetected issues with message formatting or content.
Would you like me to help create unit tests for the Slack message construction logic?
Also applies to: 203-218
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing tests of Slack message construction
rg -l "Test.*SlackMessage" --type go
Length of output: 38
Script:
#!/bin/bash
# Let's check for any existing test files in the same directory
fd ".*_test.go" contrib/opbot/botmd/
# Also check for any test files that might be testing Slack-related functionality
rg -l "slack\." --type go | grep "_test"
# Look for the actual Slack message construction implementation
rg -B 5 -A 5 "slack\.TextBlockObject" --type go
Length of output: 2650
Script:
#!/bin/bash
# Let's examine the existing test files in the botmd directory to see if they contain any Slack-related tests
cat contrib/opbot/botmd/commands_test.go
cat contrib/opbot/botmd/export_test.go
# Also check for any helper functions that might be used in Slack message construction
rg -B 2 -A 2 "toTXSlackLink|toExplorerSlackLink" --type go
Length of output: 2572
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 179-201: contrib/opbot/botmd/commands.go#L179-L201
Added lines #L179 - L201 were 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
Documentation
Chores
02ea573: explorer-ui preview link
7f43b20: explorer-ui preview link
5e94f50: explorer-ui preview link
385000a: explorer-ui preview link
e111b9e: explorer-ui preview link
14756bd: explorer-ui preview link
f28af6a: explorer-ui preview link
9671087: explorer-ui preview link
59c875c: explorer-ui preview link
d3ddc9f: explorer-ui preview link
8ddb7de: explorer-ui preview link
47954f4: explorer-ui preview link
df3e3b0: explorer-ui preview link
749a014: explorer-ui preview link
bf87f93: explorer-ui preview link