fix(opbot): reduce complexity, link to refund tx, and fix premature return #3029
Conversation
|
Warning Rate limit exceeded@golangisfun123 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 35 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent refactor enhances the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
PR Summary
This pull request refactors the rfqLookupCommand and rfqRefund functions in the opbot's commands.go file, aiming to reduce complexity and improve functionality.
- Introduced
getStatusesfunction to fetch quote request statuses, but it contains a critical bug returning nil instead of the statuses - Added
statusesToSlackBlocksfunction to convert statuses to Slack blocks for improved readability - Modified
rfqRefundto include a link to the refund transaction in the response - Fixed a premature return in
rfqLookupCommandby moving the Slack response outside the loop - Reduced overall complexity by separating concerns into helper functions
1 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
contrib/opbot/botmd/commands.go
Outdated
| } | ||
| wg.Wait() | ||
|
|
||
| return nil |
There was a problem hiding this comment.
logic: This function always returns nil, which will cause the rfqLookupCommand to fail
contrib/opbot/botmd/commands.go
Outdated
| // TODO: follow the lead of https://github.com/synapsecns/sanguine/pull/2845 | ||
| txHash, err := relClient.GetTxHashByNonce(ctx.Context(), &relapi.GetTxByNonceRequest{ | ||
| ChainID: rawRequest.OriginChainID, | ||
| Nonce: nonce, | ||
| }, | ||
| ) | ||
| _, err = ctx.Response().Reply( | ||
| fmt.Sprintf("refund submitted with nonce %d, transaction %s", nonce, toTXSlackLink(txHash.Hash, rawRequest.OriginChainID)), | ||
| ) |
There was a problem hiding this comment.
style: Consider handling the error from GetTxHashByNonce
| tx := stripLinks(ctx.Request().Param("tx")) | ||
|
|
||
| var wg sync.WaitGroup | ||
| // 2 routines per relayer, one for tx hashh one for tx id |
There was a problem hiding this comment.
syntax: Typo in comment: 'hashh' should be 'hash'
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- contrib/opbot/botmd/commands.go (8 hunks)
Additional context used
GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 242-242:
S1023: redundantreturnstatement (gosimple)
[failure] 231-231:
SA4006: this value oferris never used (staticcheck)
[failure] 275-275:
exported typeStatusshould have comment or be unexported (golint)
[failure] 280-280:
(*Bot).getStatuses - result 0 ([]github.com/synapsecns/sanguine/contrib/opbot/botmd.Status) is always nil (unparam)
Additional comments not posted (4)
contrib/opbot/botmd/commands.go (4)
Line range hint
161-175:
Refactoring improves modularity and readability.The use of
getStatusesandstatusesToSlackBlocksmethods simplifies the command logic and enhances maintainability. Ensure that these methods are tested to verify they maintain the intended functionality.
Line range hint
247-268:
Simplified function signature improves maintainability.The change to accept
originChainIDdirectly reduces dependencies and simplifies the function. Ensure that the logic remains correct with this new parameter.
320-365: Well-structured function enhances code organization.The
statusesToSlackBlocksfunction effectively separates concerns and improves readability. Ensure that the error handling and Slack message formatting are correct.
195-242: Improved error handling and transaction linking.The refactoring enhances resilience by continuing through relayers and linking refund transactions. Ensure that the
makeFastBridgefunction is used correctly with the updated signature.However, verify that all calls to
makeFastBridgeare updated to match the new signature.Verification successful
All
makeFastBridgecalls match the updated signature.The function call to
makeFastBridgeincommands.gois consistent with its updated signature, and there are no discrepancies found in the usage throughout the codebase. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `makeFastBridge` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'makeFastBridge'Length of output: 893
Tools
GitHub Check: Lint (contrib/opbot)
[failure] 242-242:
S1023: redundantreturnstatement (gosimple)
[failure] 231-231:
SA4006: this value oferris never used (staticcheck)
| type Status struct { | ||
| relayer string | ||
| *relapi.GetQuoteRequestStatusResponse | ||
| } |
There was a problem hiding this comment.
Add a comment to the exported Status type.
The Status type should have a comment explaining its purpose, as it is exported.
// Status represents the status of a quote request from a relayer.
type Status struct {
relayer string
*relapi.GetQuoteRequestStatusResponse
}Tools
GitHub Check: Lint (contrib/opbot)
[failure] 275-275:
exported typeStatusshould have comment or be unexported (golint)
contrib/opbot/botmd/commands.go
Outdated
| func (b *Bot) getStatuses(ctx *slacker.CommandContext) []Status { | ||
| var statuses []Status | ||
| var sliceMux sync.Mutex | ||
|
|
||
| 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.GetQuoteRequestStatusByTxHash(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, GetQuoteRequestStatusResponse: res}) | ||
| }() | ||
|
|
||
| go func() { | ||
| defer wg.Done() | ||
| res, err := client.GetQuoteRequestStatusByTxID(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, GetQuoteRequestStatusResponse: res}) | ||
| }() | ||
| } | ||
| wg.Wait() | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Fix the return value of getStatuses.
The function should return the statuses slice instead of nil.
- return nil
+ return statusesCommittable 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.
| func (b *Bot) getStatuses(ctx *slacker.CommandContext) []Status { | |
| var statuses []Status | |
| var sliceMux sync.Mutex | |
| 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.GetQuoteRequestStatusByTxHash(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, GetQuoteRequestStatusResponse: res}) | |
| }() | |
| go func() { | |
| defer wg.Done() | |
| res, err := client.GetQuoteRequestStatusByTxID(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, GetQuoteRequestStatusResponse: res}) | |
| }() | |
| } | |
| wg.Wait() | |
| return nil | |
| func (b *Bot) getStatuses(ctx *slacker.CommandContext) []Status { | |
| var statuses []Status | |
| var sliceMux sync.Mutex | |
| 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.GetQuoteRequestStatusByTxHash(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, GetQuoteRequestStatusResponse: res}) | |
| }() | |
| go func() { | |
| defer wg.Done() | |
| res, err := client.GetQuoteRequestStatusByTxID(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, GetQuoteRequestStatusResponse: res}) | |
| }() | |
| } | |
| wg.Wait() | |
| return statuses |
Tools
GitHub Check: Lint (contrib/opbot)
[failure] 280-280:
(*Bot).getStatuses - result 0 ([]github.com/synapsecns/sanguine/contrib/opbot/botmd.Status) is always nil (unparam)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3029 +/- ##
===================================================
+ Coverage 20.71326% 23.35381% +2.64055%
===================================================
Files 490 644 +154
Lines 42509 50146 +7637
Branches 82 82
===================================================
+ Hits 8805 11711 +2906
- Misses 32769 37271 +4502
- Partials 935 1164 +229
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
d1f7c5f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5ea42345.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-opbot-error.sanguine-fe.pages.dev |
There was a problem hiding this comment.
PR Summary
(updates since last review)
This pull request focuses on improving the opbot's command handling in the contrib/opbot/botmd/commands.go file. The main changes include:
- Enhanced error handling in the
rfqRefundcommand, allowing it to continue through multiple relayers if errors occur. - Added a new
Statusstruct to hold relayer and quote request status information. - Implemented a
getStatusesfunction to fetch quote request statuses concurrently from multiple relayers. - Created a
statusesToSlackBlocksfunction to convert statuses to Slack blocks for improved readability. - Fixed a bug in the
getStatusesfunction that was causing premature returns.
These changes aim to improve the robustness and functionality of the opbot, particularly in handling RFQ (Request for Quote) related commands. The refactoring should lead to better code organization and error handling, enhancing the overall reliability of the bot's interactions with the RFQ relayer service.
1 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings
contrib/opbot/botmd/commands.go
Outdated
| txHash, err := relClient.GetTxHashByNonce(ctx.Context(), &relapi.GetTxByNonceRequest{ | ||
| ChainID: rawRequest.OriginChainID, | ||
| Nonce: nonce, | ||
| }) | ||
| if err != nil { | ||
| log.Printf("error fetching tx hash by nonce: %v\n", err) | ||
| } |
There was a problem hiding this comment.
style: Error handling for GetTxHashByNonce is good, but consider returning early or notifying the user if the hash couldn't be fetched
| tx := stripLinks(ctx.Request().Param("tx")) | ||
|
|
||
| var wg sync.WaitGroup | ||
| // 2 routines per relayer, one for tx hashh one for tx id |
There was a problem hiding this comment.
syntax: Typo in comment: 'hashh' should be 'hash'
| go func() { | ||
| defer wg.Done() | ||
| res, err := client.GetQuoteRequestStatusByTxHash(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, GetQuoteRequestStatusResponse: res}) | ||
| }() |
There was a problem hiding this comment.
style: Consider using a pointer to the Status struct to avoid copying large objects
| defer wg.Done() | ||
| res, err := client.GetQuoteRequestStatusByTxID(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, GetQuoteRequestStatusResponse: res}) | ||
| }() |
There was a problem hiding this comment.
style: This goroutine is nearly identical to the previous one. Consider refactoring to reduce code duplication
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
contrib/opbot/botmd/commands.go (1)
Line range hint
249-265:
Add test coverage formakeFastBridge.Ensure that the new function signature and logic are covered by tests to validate its behavior.
Would you like assistance in generating unit tests for this function?
Tools
GitHub Check: codecov/patch
[warning] 260-260: contrib/opbot/botmd/commands.go#L260
Added line #L260 was not covered by tests
[warning] 265-265: contrib/opbot/botmd/commands.go#L265
Added line #L265 was not covered by tests
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- contrib/opbot/botmd/commands.go (8 hunks)
Additional context used
GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 161-161: contrib/opbot/botmd/commands.go#L161
Added line #L161 was not covered by tests
[warning] 170-170: contrib/opbot/botmd/commands.go#L170
Added line #L170 was not covered by tests
[warning] 195-197: contrib/opbot/botmd/commands.go#L195-L197
Added lines #L195 - L197 were not covered by tests
[warning] 199-200: contrib/opbot/botmd/commands.go#L199-L200
Added lines #L199 - L200 were not covered by tests
[warning] 211-213: contrib/opbot/botmd/commands.go#L211-L213
Added lines #L211 - L213 were not covered by tests
[warning] 219-224: contrib/opbot/botmd/commands.go#L219-L224
Added lines #L219 - L224 were not covered by tests
[warning] 226-228: contrib/opbot/botmd/commands.go#L226-L228
Added lines #L226 - L228 were not covered by tests
[warning] 231-237: contrib/opbot/botmd/commands.go#L231-L237
Added lines #L231 - L237 were not covered by tests
[warning] 239-244: contrib/opbot/botmd/commands.go#L239-L244
Added lines #L239 - L244 were not covered by tests
[warning] 249-249: contrib/opbot/botmd/commands.go#L249
Added line #L249 was not covered by tests
[warning] 260-260: contrib/opbot/botmd/commands.go#L260
Added line #L260 was not covered by tests
[warning] 265-265: contrib/opbot/botmd/commands.go#L265
Added line #L265 was not covered by tests
[warning] 283-303: contrib/opbot/botmd/commands.go#L283-L303
Added lines #L283 - L303 were not covered by tests
[warning] 306-315: contrib/opbot/botmd/commands.go#L306-L315
Added lines #L306 - L315 were not covered by tests
[warning] 318-320: contrib/opbot/botmd/commands.go#L318-L320
Added lines #L318 - L320 were not covered by tests
[warning] 323-329: contrib/opbot/botmd/commands.go#L323-L329
Added lines #L323 - L329 were not covered by tests
[warning] 331-364: contrib/opbot/botmd/commands.go#L331-L364
Added lines #L331 - L364 were not covered by tests
[warning] 366-366: contrib/opbot/botmd/commands.go#L366
Added line #L366 was not covered by tests
[warning] 368-368: contrib/opbot/botmd/commands.go#L368
Added line #L368 was not covered by tests
[warning] 412-412: contrib/opbot/botmd/commands.go#L412
Added line #L412 was not covered by tests
Additional comments not posted (5)
contrib/opbot/botmd/commands.go (5)
Line range hint
161-175:
LGTM!The refactoring in
rfqLookupCommandimproves modularity and readability by delegating tasks togetStatusesandstatusesToSlackBlocks.Tools
GitHub Check: codecov/patch
[warning] 170-170: contrib/opbot/botmd/commands.go#L170
Added line #L170 was not covered by tests
195-244: LGTM!The changes in
rfqRefundenhance error handling and resilience, allowing for continued processing through multiple relayers.Tools
GitHub Check: codecov/patch
[warning] 195-197: contrib/opbot/botmd/commands.go#L195-L197
Added lines #L195 - L197 were not covered by tests
[warning] 199-200: contrib/opbot/botmd/commands.go#L199-L200
Added lines #L199 - L200 were not covered by tests
[warning] 211-213: contrib/opbot/botmd/commands.go#L211-L213
Added lines #L211 - L213 were not covered by tests
[warning] 219-224: contrib/opbot/botmd/commands.go#L219-L224
Added lines #L219 - L224 were not covered by tests
[warning] 226-228: contrib/opbot/botmd/commands.go#L226-L228
Added lines #L226 - L228 were not covered by tests
[warning] 231-237: contrib/opbot/botmd/commands.go#L231-L237
Added lines #L231 - L237 were not covered by tests
[warning] 239-244: contrib/opbot/botmd/commands.go#L239-L244
Added lines #L239 - L244 were not covered by tests
Line range hint
249-265:
LGTM!The simplification of the
makeFastBridgefunction signature improves clarity and reduces dependencies.Tools
GitHub Check: codecov/patch
[warning] 260-260: contrib/opbot/botmd/commands.go#L260
Added line #L260 was not covered by tests
[warning] 265-265: contrib/opbot/botmd/commands.go#L265
Added line #L265 was not covered by tests
283-320: LGTM!The use of goroutines and a mutex in
getStatuseseffectively handles concurrent status retrieval.Tools
GitHub Check: codecov/patch
[warning] 283-303: contrib/opbot/botmd/commands.go#L283-L303
Added lines #L283 - L303 were not covered by tests
[warning] 306-315: contrib/opbot/botmd/commands.go#L306-L315
Added lines #L306 - L315 were not covered by tests
[warning] 318-320: contrib/opbot/botmd/commands.go#L318-L320
Added lines #L318 - L320 were not covered by tests
323-368: LGTM!The
statusesToSlackBlocksfunction is well-structured for formatting statuses into Slack blocks.Tools
GitHub Check: codecov/patch
[warning] 323-329: contrib/opbot/botmd/commands.go#L323-L329
Added lines #L323 - L329 were not covered by tests
[warning] 331-364: contrib/opbot/botmd/commands.go#L331-L364
Added lines #L331 - L364 were not covered by tests
[warning] 366-366: contrib/opbot/botmd/commands.go#L366
Added line #L366 was not covered by tests
[warning] 368-368: contrib/opbot/botmd/commands.go#L368
Added line #L368 was not covered by tests
| } | ||
| wg.Wait() | ||
|
|
||
| statuses := b.getStatuses(ctx) |
There was a problem hiding this comment.
Add test coverage for rfqLookupCommand.
The new logic in rfqLookupCommand involving getStatuses and statusesToSlackBlocks should be covered by tests to ensure reliability.
Would you like assistance in generating unit tests for this function?
Also applies to: 170-170
Tools
GitHub Check: codecov/patch
[warning] 161-161: contrib/opbot/botmd/commands.go#L161
Added line #L161 was not covered by tests
contrib/opbot/botmd/commands.go
Outdated
| var rawRequest *relapi.GetQuoteRequestResponse | ||
| var err error | ||
| var relClient relapi.RelayerClient | ||
| for _, relayer := range b.cfg.RelayerURLS { | ||
| relClient := relapi.NewRelayerClient(b.handler, relayer) | ||
|
|
||
| rawRequest, err := getQuoteRequest(ctx.Context(), relClient, tx) | ||
| relClient = relapi.NewRelayerClient(b.handler, relayer) | ||
| rawRequest, err = getQuoteRequest(ctx.Context(), relClient, tx) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error fetching quote request") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply(err.Error()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(rawRequest.OriginChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) | ||
| 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) | ||
| // continue through the rest of the relayers | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| // TODO: follow the lead of https://github.com/synapsecns/sanguine/pull/2845 | ||
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | ||
| fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest.OriginChainID) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply(err.Error()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(rawRequest.OriginChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) | ||
| 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) | ||
| } | ||
|
|
||
| // TODO: follow the lead of https://github.com/synapsecns/sanguine/pull/2845 | ||
| txHash, err := relClient.GetTxHashByNonce(ctx.Context(), &relapi.GetTxByNonceRequest{ | ||
| ChainID: rawRequest.OriginChainID, | ||
| Nonce: nonce, | ||
| }) | ||
| if err != nil { | ||
| log.Printf("error fetching tx hash by nonce: %v\n", err) | ||
| } | ||
|
|
||
| _, err = ctx.Response().Reply( | ||
| fmt.Sprintf("refund submitted with nonce %d, transaction %s", nonce, toTXSlackLink(txHash.Hash, rawRequest.OriginChainID)), | ||
| ) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } |
There was a problem hiding this comment.
Add test coverage for rfqRefund.
The refund logic and error handling should be covered by tests to ensure the function behaves as expected under various conditions.
Would you like assistance in generating unit tests for this function?
Tools
GitHub Check: codecov/patch
[warning] 195-197: contrib/opbot/botmd/commands.go#L195-L197
Added lines #L195 - L197 were not covered by tests
[warning] 199-200: contrib/opbot/botmd/commands.go#L199-L200
Added lines #L199 - L200 were not covered by tests
[warning] 211-213: contrib/opbot/botmd/commands.go#L211-L213
Added lines #L211 - L213 were not covered by tests
[warning] 219-224: contrib/opbot/botmd/commands.go#L219-L224
Added lines #L219 - L224 were not covered by tests
[warning] 226-228: contrib/opbot/botmd/commands.go#L226-L228
Added lines #L226 - L228 were not covered by tests
[warning] 231-237: contrib/opbot/botmd/commands.go#L231-L237
Added lines #L231 - L237 were not covered by tests
[warning] 239-244: contrib/opbot/botmd/commands.go#L239-L244
Added lines #L239 - L244 were not covered by tests
There was a problem hiding this comment.
PR Summary
(updates since last review)
This pull request enhances the opbot's command handling, focusing on the rfqRefund command in contrib/opbot/botmd/commands.go.
- Improved error handling in
rfqRefundcommand, allowing continuation through multiple relayers - Added
getStatusesfunction for concurrent fetching of quote request statuses from multiple relayers - Implemented
statusesToSlackBlocksfunction to convert statuses to Slack blocks for better readability - Fixed a bug in
getStatusesthat was causing premature returns - Refactored code to improve organization and reduce complexity in RFQ-related command handling
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
No major changes found since last review.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
This pull request focuses on improving the opbot commands implementation, particularly the refund command and related functions. Here's a concise summary of the key changes:
- Refactored the
rfqRefundcommand to handle multiple relayers, improving error handling and reliability. - Introduced new helper methods
getStatusesandstatusesToSlackBlocksto enhance modularity and readability. - Implemented concurrent fetching of quote request statuses, improving performance when dealing with multiple relayers.
- Added a link to the refund transaction in the Slack response, providing better traceability.
- Fixed a premature return issue in the refund process, allowing it to continue through all configured relayers.
These changes aim to reduce complexity, improve error handling, and enhance the overall functionality of the opbot, particularly in handling refund requests across multiple relayers.
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (2)
contrib/opbot/botmd/commands.go (2)
Line range hint
162-176:
Add test coverage forrfqLookupCommand.The new logic in
rfqLookupCommandinvolvinggetStatusesandstatusesToSlackBlocksshould be covered by tests to ensure reliability.Would you like assistance in generating unit tests for this function?
286-290: Add a comment to the exportedStatustype.The
Statustype should have a comment explaining its purpose, as it is exported.// Status represents the status of a quote request from a relayer. type Status struct { relayer string *relapi.GetQuoteRequestStatusResponse }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
contrib/screener-api/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sum
Files selected for processing (3)
- contrib/opbot/botmd/commands.go (9 hunks)
- contrib/screener-api/go.mod (1 hunks)
- ethergo/go.mod (1 hunks)
Files skipped from review due to trivial changes (2)
- contrib/screener-api/go.mod
- ethergo/go.mod
Additional context used
GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 232-232:
Error return value ofretry.WithBackoffis not checked (errcheck)
Additional comments not posted (1)
contrib/opbot/botmd/commands.go (1)
Line range hint
258-283:
LGTM!The refactored
makeFastBridgefunction simplifies the method signature and improves error handling.
contrib/opbot/botmd/commands.go
Outdated
| var rawRequest *relapi.GetQuoteRequestResponse | ||
| var err error | ||
| var relClient relapi.RelayerClient | ||
| for _, relayer := range b.cfg.RelayerURLS { | ||
| relClient := relapi.NewRelayerClient(b.handler, relayer) | ||
|
|
||
| rawRequest, err := getQuoteRequest(ctx.Context(), relClient, tx) | ||
| relClient = relapi.NewRelayerClient(b.handler, relayer) | ||
| rawRequest, err = getQuoteRequest(ctx.Context(), relClient, tx) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error fetching quote request") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply(err.Error()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(rawRequest.OriginChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) | ||
| 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) | ||
| // continue through the rest of the relayers | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| // TODO: follow the lead of https://github.com/synapsecns/sanguine/pull/2845 | ||
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | ||
| fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest.OriginChainID) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply(err.Error()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(rawRequest.OriginChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) | ||
| 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) | ||
| } | ||
|
|
||
| var txHash *relapi.TxHashByNonceResponse | ||
| retry.WithBackoff( | ||
| ctx.Context(), | ||
| func(_ context.Context) error { | ||
| txHash, err = relClient.GetTxHashByNonce(ctx.Context(), &relapi.GetTxByNonceRequest{ | ||
| ChainID: rawRequest.OriginChainID, | ||
| Nonce: nonce, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("error fetching tx hash by nonce: %w", err) | ||
| } | ||
| return nil | ||
| }, | ||
| retry.WithMaxAttempts(5), | ||
| retry.WithMaxTotalTime(30*time.Second), | ||
| ) | ||
|
|
||
| _, err = ctx.Response().Reply( | ||
| fmt.Sprintf("refund submitted with nonce %d, transaction %s", nonce, toTXSlackLink(txHash.Hash, rawRequest.OriginChainID)), | ||
| ) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } |
There was a problem hiding this comment.
Add test coverage for rfqRefund.
The refund logic and error handling should be covered by tests to ensure the function behaves as expected under various conditions.
Would you like assistance in generating unit tests for this function?
Tools
GitHub Check: Lint (contrib/opbot)
[failure] 232-232:
Error return value ofretry.WithBackoffis not checked (errcheck)
There was a problem hiding this comment.
PR Summary
(updates since last review)
No major changes found since last review.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (2)
contrib/opbot/botmd/commands.go (2)
Line range hint
162-171:
Add test coverage forrfqLookupCommand.The logic involving
getStatusesandstatusesToSlackBlocksshould be covered by tests to ensure reliability.Would you like assistance in generating unit tests for this function?
Tools
GitHub Check: codecov/patch
[warning] 162-162: contrib/opbot/botmd/commands.go#L162
Added line #L162 was not covered by tests
Line range hint
261-277:
Add test coverage formakeFastBridge.Tests should verify the function's behavior under various conditions, including error scenarios.
Would you like assistance in generating unit tests for this function?
Tools
GitHub Check: codecov/patch
[warning] 272-272: contrib/opbot/botmd/commands.go#L272
Added line #L272 was not covered by tests
[warning] 277-277: contrib/opbot/botmd/commands.go#L277
Added line #L277 was not covered by tests
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] 162-162: contrib/opbot/botmd/commands.go#L162
Added line #L162 was not covered by tests
[warning] 171-171: contrib/opbot/botmd/commands.go#L171
Added line #L171 was not covered by tests
[warning] 196-198: contrib/opbot/botmd/commands.go#L196-L198
Added lines #L196 - L198 were not covered by tests
[warning] 200-201: contrib/opbot/botmd/commands.go#L200-L201
Added lines #L200 - L201 were not covered by tests
[warning] 212-214: contrib/opbot/botmd/commands.go#L212-L214
Added lines #L212 - L214 were not covered by tests
[warning] 220-225: contrib/opbot/botmd/commands.go#L220-L225
Added lines #L220 - L225 were not covered by tests
[warning] 227-229: contrib/opbot/botmd/commands.go#L227-L229
Added lines #L227 - L229 were not covered by tests
[warning] 231-242: contrib/opbot/botmd/commands.go#L231-L242
Added lines #L231 - L242 were not covered by tests
[warning] 247-249: contrib/opbot/botmd/commands.go#L247-L249
Added lines #L247 - L249 were not covered by tests
[warning] 251-256: contrib/opbot/botmd/commands.go#L251-L256
Added lines #L251 - L256 were not covered by tests
[warning] 261-261: contrib/opbot/botmd/commands.go#L261
Added line #L261 was not covered by tests
[warning] 272-272: contrib/opbot/botmd/commands.go#L272
Added line #L272 was not covered by tests
[warning] 277-277: contrib/opbot/botmd/commands.go#L277
Added line #L277 was not covered by tests
[warning] 295-315: contrib/opbot/botmd/commands.go#L295-L315
Added lines #L295 - L315 were not covered by tests
[warning] 318-327: contrib/opbot/botmd/commands.go#L318-L327
Added lines #L318 - L327 were not covered by tests
[warning] 330-332: contrib/opbot/botmd/commands.go#L330-L332
Added lines #L330 - L332 were not covered by tests
[warning] 335-341: contrib/opbot/botmd/commands.go#L335-L341
Added lines #L335 - L341 were not covered by tests
[warning] 343-376: contrib/opbot/botmd/commands.go#L343-L376
Added lines #L343 - L376 were not covered by tests
[warning] 378-378: contrib/opbot/botmd/commands.go#L378
Added line #L378 was not covered by tests
[warning] 380-380: contrib/opbot/botmd/commands.go#L380
Added line #L380 was not covered by tests
[warning] 424-424: contrib/opbot/botmd/commands.go#L424
Added line #L424 was not covered by tests
Additional comments not posted (1)
contrib/opbot/botmd/commands.go (1)
424-424: LGTM!The logic for fetching a quote request is clear and well-handled.
Tools
GitHub Check: codecov/patch
[warning] 424-424: contrib/opbot/botmd/commands.go#L424
Added line #L424 was not covered by tests
contrib/opbot/botmd/commands.go
Outdated
| var rawRequest *relapi.GetQuoteRequestResponse | ||
| var err error | ||
| var relClient relapi.RelayerClient | ||
| for _, relayer := range b.cfg.RelayerURLS { | ||
| relClient := relapi.NewRelayerClient(b.handler, relayer) | ||
|
|
||
| rawRequest, err := getQuoteRequest(ctx.Context(), relClient, tx) | ||
| relClient = relapi.NewRelayerClient(b.handler, relayer) | ||
| rawRequest, err = getQuoteRequest(ctx.Context(), relClient, tx) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error fetching quote request") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply(err.Error()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(rawRequest.OriginChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) | ||
| 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) | ||
| // continue through the rest of the relayers | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| // TODO: follow the lead of https://github.com/synapsecns/sanguine/pull/2845 | ||
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | ||
| fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest.OriginChainID) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply(err.Error()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(rawRequest.OriginChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| tx, err = fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) | ||
| 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) | ||
| } | ||
|
|
||
| var txHash *relapi.TxHashByNonceResponse | ||
| err = retry.WithBackoff( | ||
| ctx.Context(), | ||
| func(_ context.Context) error { | ||
| txHash, err = relClient.GetTxHashByNonce(ctx.Context(), &relapi.GetTxByNonceRequest{ | ||
| ChainID: rawRequest.OriginChainID, | ||
| Nonce: nonce, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("error fetching tx hash by nonce: %w", err) | ||
| } | ||
| return nil | ||
| }, | ||
| retry.WithMaxAttempts(5), | ||
| retry.WithMaxTotalTime(30*time.Second), | ||
| ) | ||
| if err != nil { | ||
| log.Printf("error fetching tx hash by nonce: %v\n", err) | ||
| } | ||
|
|
||
| _, err = ctx.Response().Reply( | ||
| fmt.Sprintf("refund submitted with nonce %d, transaction %s", nonce, toTXSlackLink(txHash.Hash, rawRequest.OriginChainID)), | ||
| ) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } |
There was a problem hiding this comment.
Add test coverage for rfqRefund.
The refund logic and error handling should be covered by tests to ensure the function behaves as expected under various conditions.
Would you like assistance in generating unit tests for this function?
Tools
GitHub Check: codecov/patch
[warning] 196-198: contrib/opbot/botmd/commands.go#L196-L198
Added lines #L196 - L198 were not covered by tests
[warning] 200-201: contrib/opbot/botmd/commands.go#L200-L201
Added lines #L200 - L201 were not covered by tests
[warning] 212-214: contrib/opbot/botmd/commands.go#L212-L214
Added lines #L212 - L214 were not covered by tests
[warning] 220-225: contrib/opbot/botmd/commands.go#L220-L225
Added lines #L220 - L225 were not covered by tests
[warning] 227-229: contrib/opbot/botmd/commands.go#L227-L229
Added lines #L227 - L229 were not covered by tests
[warning] 231-242: contrib/opbot/botmd/commands.go#L231-L242
Added lines #L231 - L242 were not covered by tests
[warning] 247-249: contrib/opbot/botmd/commands.go#L247-L249
Added lines #L247 - L249 were not covered by tests
[warning] 251-256: contrib/opbot/botmd/commands.go#L251-L256
Added lines #L251 - L256 were not covered by tests
| func (b *Bot) getStatuses(ctx *slacker.CommandContext) []Status { | ||
| var statuses []Status | ||
| var sliceMux sync.Mutex | ||
|
|
||
| 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.GetQuoteRequestStatusByTxHash(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, GetQuoteRequestStatusResponse: res}) | ||
| }() | ||
|
|
||
| go func() { | ||
| defer wg.Done() | ||
| res, err := client.GetQuoteRequestStatusByTxID(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, GetQuoteRequestStatusResponse: res}) | ||
| }() | ||
| } | ||
| wg.Wait() | ||
|
|
||
| return statuses |
There was a problem hiding this comment.
Add test coverage for getStatuses.
The concurrent status retrieval logic should be covered by tests to ensure thread safety and correctness.
Would you like assistance in generating unit tests for this function?
Tools
GitHub Check: codecov/patch
[warning] 295-315: contrib/opbot/botmd/commands.go#L295-L315
Added lines #L295 - L315 were not covered by tests
[warning] 318-327: contrib/opbot/botmd/commands.go#L318-L327
Added lines #L318 - L327 were not covered by tests
[warning] 330-332: contrib/opbot/botmd/commands.go#L330-L332
Added lines #L330 - L332 were not covered by tests
| func (b *Bot) statusesToSlackBlocks(ctx *slacker.CommandContext, statuses []Status) []slack.Block { | ||
| var slackBlocks []slack.Block | ||
| for _, status := range statuses { | ||
| client, err := b.rpcClient.GetChainClient(ctx.Context(), int(status.OriginChainID)) | ||
| if err != nil { | ||
| log.Printf("error getting chain client: %v\n", err) | ||
| } | ||
|
|
||
| objects := []*slack.TextBlockObject{ | ||
| { | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*Relayer*: %s", status.relayer), | ||
| }, | ||
| { | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*Status*: %s", status.Status), | ||
| }, | ||
| { | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*TxID*: %s", toExplorerSlackLink(status.TxID)), | ||
| }, | ||
| { | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*OriginTxHash*: %s", toTXSlackLink(status.OriginTxHash, status.OriginChainID)), | ||
| }, | ||
| { | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*Estimated Tx Age*: %s", getTxAge(ctx.Context(), client, status.GetQuoteRequestStatusResponse)), | ||
| }, | ||
| } | ||
|
|
||
| if status.DestTxHash == (common.Hash{}).String() { | ||
| objects = append(objects, &slack.TextBlockObject{ | ||
| Type: slack.MarkdownType, | ||
| Text: "*DestTxHash*: not available", | ||
| }) | ||
| } else { | ||
| objects = append(objects, &slack.TextBlockObject{ | ||
| Type: slack.MarkdownType, | ||
| Text: fmt.Sprintf("*DestTxHash*: %s", toTXSlackLink(status.DestTxHash, status.DestChainID)), | ||
| }) | ||
| } | ||
|
|
||
| slackBlocks = append(slackBlocks, slack.NewSectionBlock(nil, objects, nil)) | ||
| } | ||
| return slackBlocks |
There was a problem hiding this comment.
Add test coverage for statusesToSlackBlocks.
Test the Slack block formatting logic to ensure it correctly represents the statuses.
Would you like assistance in generating unit tests for this function?
Tools
GitHub Check: codecov/patch
[warning] 335-341: contrib/opbot/botmd/commands.go#L335-L341
Added lines #L335 - L341 were not covered by tests
[warning] 343-376: contrib/opbot/botmd/commands.go#L343-L376
Added lines #L343 - L376 were not covered by tests
[warning] 378-378: contrib/opbot/botmd/commands.go#L378
Added line #L378 was not covered by tests
[warning] 380-380: contrib/opbot/botmd/commands.go#L380
Added line #L380 was not covered by tests
There was a problem hiding this comment.
PR Summary
(updates since last review)
This pull request introduces improvements to the rfqRefund function in the contrib/opbot/botmd/commands.go file, focusing on enhancing error handling and user experience for the refund process. Key changes include:
- Improved error handling when fetching quote requests from multiple relayers
- Added retry mechanism for fetching transaction hash by nonce
- Enhanced logging for better debugging and monitoring
- More detailed error messages for users
These updates should result in a more robust and user-friendly refund process within the OpBot system.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
This pull request further refines the OpBot's RFQ-related commands, focusing on the rfqLookupCommand and rfqRefund functions in the contrib/opbot/botmd/commands.go file. Key improvements include:
- Extracted
getStatusesmethod to handle concurrent fetching of quote request statuses from multiple relayers, improving code organization and performance. - Introduced
statusesToSlackBlocksmethod to separate the logic for constructing Slack message blocks, enhancing modularity. - Improved error handling in the refund process, allowing it to continue through multiple relayers if one fails.
- Added linking to the refund transaction in the Slack response, providing better visibility of the refund process.
- Reduced complexity in the main command functions by moving logic to separate methods, improving readability and maintainability.
These changes build upon the previous improvements, further enhancing the robustness and user experience of the OpBot's RFQ-related functionalities.
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
| } | ||
| wg.Wait() | ||
|
|
||
| statuses := b.getStatuses(ctx) | ||
| if len(statuses) == 0 { |
There was a problem hiding this comment.
style: Consider adding a more specific error message here, indicating why no quote request was found
| fmt.Sprintf("refund submitted with nonce %d, transaction %s", nonce, toTXSlackLink(txHash.Hash, rawRequest.OriginChainID)), | ||
| ) |
There was a problem hiding this comment.
style: Consider handling the case where txHash is nil
|
@golangisfun123 version of this pr deployed at 805dca0 kept crashing witht his error: Also, anecdotally rfq command was erroring a lot, I think b/c of status endpoint mismatch or something: 
|
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
Refactor