fix(opbot): fix refund forever #3082
Conversation
golangisfun123
requested review from
dwasse,
trajan0x and
aureliusbtc
as code owners
WalkthroughThe changes involve the integration of a new dependency on the Changes
Sequence Diagram(s)sequenceDiagram
participant Bot
participant ScreenerClient
participant QuoteRequest
Bot->>ScreenerClient: Initialize with NewClient
Bot->>QuoteRequest: Fetch quote request
QuoteRequest-->>Bot: Return quote request
Bot->>ScreenerClient: Submit refund
ScreenerClient-->>Bot: Confirmation of refund
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3082 +/- ##
===================================================
+ Coverage 23.06899% 25.04098% +1.97199%
===================================================
Files 193 173 -20
Lines 11639 10371 -1268
Branches 82 82
===================================================
- Hits 2685 2597 -88
+ Misses 8712 7535 -1177
+ Partials 242 239 -3
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: |
d5ad596
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://117247b5.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-refund-forever.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
contrib/opbot/botmd/botmd.go (1)
94-99: Test coverage missing for new logic inStartmethod.The recent changes to the
Startmethod in theBotstruct, specifically the initialization of thescreenerclient, are not covered by existing tests. Consider adding test cases to ensure this new logic is verified and robust.Analysis chain
Updated
Startmethod handlesscreenerclient initialization correctly.The updates to the
Startmethod for initializing thescreenerclient are correctly implemented with appropriate error handling. However, the added lines are not covered by tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for the new logic in the `Start` method. # Test: Search for test cases covering the new logic. Expect: At least one test case covering lines 94-99. rg --type go -A 5 $'func Test.*Start.*'Length of output: 391
Tools
GitHub Check: codecov/patch
[warning] 94-99: contrib/opbot/botmd/botmd.go#L94-L99
Added lines #L94 - L99 were not covered by tests
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 (2 hunks)
- contrib/opbot/cmd/commands.go (1 hunks)
- contrib/opbot/config/config.go (1 hunks)
- contrib/opbot/go.mod (2 hunks)
- services/rfq/relayer/relapi/handler.go (2 hunks)
- services/rfq/relayer/relapi/model.go (1 hunks)
Files skipped from review due to trivial changes (1)
- contrib/opbot/cmd/commands.go
Additional context used
GitHub Check: codecov/patch
contrib/opbot/botmd/botmd.go
[warning] 94-99: contrib/opbot/botmd/botmd.go#L94-L99
Added lines #L94 - L99 were not covered by testscontrib/opbot/botmd/commands.go
[warning] 326-326: contrib/opbot/botmd/commands.go#L326
Added line #L326 was not covered by tests
[warning] 328-328: contrib/opbot/botmd/commands.go#L328
Added line #L328 was not covered by tests
[warning] 333-333: contrib/opbot/botmd/commands.go#L333
Added line #L333 was not covered by tests
[warning] 414-416: contrib/opbot/botmd/commands.go#L414-L416
Added lines #L414 - L416 were not covered by tests
[warning] 420-421: contrib/opbot/botmd/commands.go#L420-L421
Added lines #L420 - L421 were not covered by tests
[warning] 424-424: contrib/opbot/botmd/commands.go#L424
Added line #L424 was not covered by tests
GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 341-341:
S1023: redundantreturnstatement (gosimple)
Additional comments not posted (6)
services/rfq/relayer/relapi/model.go (1)
20-20: Addition ofSenderfield is correctly implemented.The new field
Senderin theGetQuoteRequestResponsestruct is correctly annotated with a JSON tag for serialization. This addition enhances the response structure by providing more contextual information.contrib/opbot/config/config.go (1)
37-37: Addition ofScreenerURLfield is correctly implemented.The new field
ScreenerURLin theConfigstruct is correctly annotated with a YAML tag for serialization. This addition enhances the configuration capabilities by allowing the storage of an additional parameter related to screening functionality.contrib/opbot/botmd/botmd.go (1)
32-32: Addition ofscreenerfield is correctly implemented.The new field
screenerin theBotstruct is correctly declared and is intended to enable interaction with the screener service. This addition expands the bot's capabilities significantly.services/rfq/relayer/relapi/handler.go (2)
115-115: Addition of Sender field in GetQuoteRequestByTxID response.The inclusion of the
Senderfield in the response is a useful enhancement for tracking purposes. Ensure that theSenderfield is consistently populated across different scenarios and that error handling remains robust.The code changes are approved.
145-145: Addition of Sender field in GetQuoteRequestByTxHash response.The consistent addition of the
Senderfield across different retrieval functions is good for maintaining uniformity in the response structure. Verify that theSenderfield is handled similarly in other related functions for consistency.The code changes are approved.
contrib/opbot/go.mod (1)
25-25: Addition ofscreener-apidependency and local path replacement.The inclusion of the
screener-apimodule in thego.modfile is a significant change that supports the new functionality introduced in therfqRefundfunction. The use of a local path replacement for this dependency suggests that the development is still in progress or that there is a need for customization that isn't addressed by the main repository.The changes are logical and align with the functionality enhancements described in the PR. However, ensure that the local path is accessible and correctly set up in your development environment to avoid build issues.
Also applies to: 298-298
contrib/opbot/botmd/commands.go
Outdated
| var rawRequest *relapi.GetQuoteRequestResponse | ||
| var err error | ||
| for _, relayer := range b.cfg.RelayerURLS { | ||
| relClient := relapi.NewRelayerClient(b.handler, relayer) | ||
|
|
||
| rawRequest, err := getQuoteRequest(ctx.Context(), relClient, tx) | ||
| rawRequest, err = getQuoteRequest(ctx.Context(), relClient, tx) | ||
| if err == nil { | ||
| break | ||
| } | ||
| } | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error fetching quote request") | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error fetching quote request") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest) | ||
| fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply(err.Error()) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply(err.Error()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| log.Println(err) | ||
| } | ||
| 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 | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| canRefund, err := b.screener.ScreenAddress(ctx.Context(), rawRequest.Sender) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error screening address") | ||
| if err != nil { | ||
| log.Printf("error submitting refund: %v\n", err) | ||
| continue | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // TODO: follow the lead of https://github.com/synapsecns/sanguine/pull/2845 | ||
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | ||
| if !canRefund { | ||
| _, err := ctx.Response().Reply("address cannot be refunded") | ||
| 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) | ||
| return | ||
| } | ||
|
|
||
| // TODO: follow the lead of https://github.com/synapsecns/sanguine/pull/2845 | ||
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return |
There was a problem hiding this comment.
Refactor and enhance error handling in rfqRefund function.
The changes in the rfqRefund function improve the clarity and robustness of error handling by breaking out the error checks and using early returns. This makes the function easier to read and maintain. The addition of a check to see if an address can be refunded before proceeding with the transaction is a critical improvement that prevents unnecessary transactions.
However, there are several lines of code that are not covered by tests, specifically lines 326, 328, and 333. It's crucial to ensure that these lines are covered to maintain high code quality and avoid potential issues in production.
Consider adding unit tests for these lines to ensure they function as expected under various scenarios.
Tools
GitHub Check: codecov/patch
[warning] 326-326: contrib/opbot/botmd/commands.go#L326
Added line #L326 was not covered by tests
[warning] 328-328: contrib/opbot/botmd/commands.go#L328
Added line #L328 was not covered by tests
[warning] 333-333: contrib/opbot/botmd/commands.go#L333
Added line #L333 was not covered by tests
GitHub Check: Lint (contrib/opbot)
[failure] 341-341:
S1023: redundantreturnstatement (gosimple)
contrib/opbot/botmd/commands.go
Outdated
| func getQuoteRequest(ctx context.Context, client relapi.RelayerClient, tx string) (qr *relapi.GetQuoteRequestResponse, err error) { | ||
| if qr, err = client.GetQuoteRequestByTxHash(ctx, tx); err == nil { | ||
| return qr, nil | ||
| } | ||
|
|
||
| // look up quote request | ||
| qr, err := client.GetQuoteRequestByTXID(ctx, tx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error fetching quote request: %w", err) | ||
| if qr, err = client.GetQuoteRequestByTXID(ctx, tx); err == nil { | ||
| return qr, nil | ||
| } | ||
|
|
||
| return qr, nil | ||
| return nil, fmt.Errorf("error fetching quote request: %w", err) |
There was a problem hiding this comment.
Simplify error handling in getQuoteRequest function.
The refactoring of the getQuoteRequest function to use named return values simplifies the function and makes the error handling more straightforward. This change aligns with the PR's goal to enhance readability and maintainability.
However, the new lines added in this function (414-416 and 420-421) are not covered by tests, which could lead to potential issues if edge cases are not properly handled.
It's recommended to add test coverage for these lines to ensure the function's reliability, especially since it plays a crucial role in fetching quote requests.
Tools
GitHub Check: codecov/patch
[warning] 414-416: contrib/opbot/botmd/commands.go#L414-L416
Added lines #L414 - L416 were not covered by tests
[warning] 420-421: contrib/opbot/botmd/commands.go#L420-L421
Added lines #L420 - L421 were not covered by tests
[warning] 424-424: contrib/opbot/botmd/commands.go#L424
Added line #L424 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- contrib/opbot/botmd/commands.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- contrib/opbot/botmd/commands.go
#3076 first, then this
Summary by CodeRabbit
New Features
Bug Fixes
Chores