add refunder to opbot #2838
add refunder to opbot #2838
Conversation
|
Warning Rate limit exceeded@trajan0x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 33 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. WalkthroughThis update introduces significant enhancements across several components of the RFQ (Request for Quote) and bot systems. Key changes include the addition of refund handling functionality, improved context propagation within middleware, new endpoint definitions for API contract address retrieval, and database configuration updates. These modifications collectively advance error handling, database operations, submission processes, and service integrations, leading to improved system robustness and feature set expansion. Changes
Sequence DiagramssequenceDiagram
participant User
participant Bot
participant RPCClient
participant Signer
participant Submitter
participant RFQClient
User ->> Bot: Start Bot
Bot ->> RPCClient: Initialize RPC Client
Bot ->> Signer: Create Signer
Bot ->> Submitter: Initialize Submitter
Bot ->> RFQClient: Fetch RFQ Contracts
RFQClient -->> Bot: Contract Details
Bot ->> User: Bot Started with Contracts
sequenceDiagram
participant User
participant RFQClient
participant Database
User ->> RFQClient: Request Refund (rfqRefund)
RFQClient ->> Database: Fetch Quote
Database -->> RFQClient: Quote Details
RFQClient ->> User: Process Refund
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 Configration File (
|
Deploying sanguine-fe with
|
| Latest commit: |
27b8159
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e1ee89e2.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-rfq-refunder.sanguine-fe.pages.dev |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Added new API endpoint
/contractsfor retrieving contract addresses (services/rfq/api/docs/docs.go,services/rfq/api/docs/swagger.json,services/rfq/api/docs/swagger.yaml) - Introduced
GetContractsResponsestruct for the new endpoint (services/rfq/api/model/response.go) - Added
GetContractsmethod to the REST handler and updated handler initialization (services/rfq/api/rest/handler.go,services/rfq/api/rest/server.go) - Updated client to include
GetRFQContractsmethod and improved error handling (services/rfq/api/client/client.go) - Added documentation for new endpoints (
docs/bridge/docs/rfq/API/get-contract-addresses.api.mdx,docs/bridge/docs/rfq/API/relay-ack.api.mdx)
10 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
| --- | ||
| id: get-contract-addresses | ||
| title: "Get contract addresses" | ||
| description: "get quotes from all relayers." |
There was a problem hiding this comment.
📚 spelling: The description 'get quotes from all relayers.' seems incorrect for this endpoint. It should describe getting contract addresses.
|
|
||
|
|
||
|
|
||
| get quotes from all relayers. |
There was a problem hiding this comment.
📚 spelling: This line appears to be a leftover from a copy-paste. It should be removed or corrected to describe the 'get contract addresses' functionality.
services/rfq/api/client/client.go
Outdated
| func getStatus(resp *resty.Response) string { | ||
| if resp == nil { | ||
| return "http status unavailable" | ||
| } | ||
| return getStatus(resp) |
There was a problem hiding this comment.
🧠 logic: The getStatus function calls itself recursively, causing a stack overflow. It should return resp.Status() instead.
| func getStatus(resp *resty.Response) string { | |
| if resp == nil { | |
| return "http status unavailable" | |
| } | |
| return getStatus(resp) | |
| return resp.Status() |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2838 +/- ##
===================================================
- Coverage 25.90605% 25.69344% -0.21262%
===================================================
Files 709 770 +61
Lines 52536 55520 +2984
Branches 80 80
===================================================
+ Hits 13610 14265 +655
- Misses 37537 39792 +2255
- Partials 1389 1463 +74
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added context inheritance for tracing middleware (
contrib/opbot/botmd/middleware.go) - Updated metrics middleware to record command execution duration and manage request counts (
contrib/opbot/botmd/middleware.go) - Updated dependencies:
github.com/gorilla/websocket,github.com/slack-io/commander,github.com/slack-io/proper(contrib/opbot/go.mod) - Updated replacement for
github.com/slack-io/slackerto a newer version (contrib/opbot/go.mod)
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added refunder functionality to OpBot (
contrib/opbot/botmd/botmd.go) - Introduced
rfqRefundcommand for refunding quote requests (contrib/opbot/botmd/commands.go) - Added new configuration fields for RFQ API, Omni RPC, Signer, Submitter, and Database (
contrib/opbot/config/config.go) - Updated
slack-go/slackdependency to v0.13.0 (contrib/opbot/go.mod) - Created new database store implementations for MySQL and SQLite (
contrib/opbot/sql/*)
19 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings
| func (s Store) DB() *gorm.DB { | ||
| return s.db |
There was a problem hiding this comment.
🪶 style: Consider returning a pointer to the gorm.DB to avoid copying the struct.
| func (s Store) SubmitterDB() submitterDB.Service { | ||
| return s.submitterStore |
There was a problem hiding this comment.
🪶 style: Consider returning a pointer to the submitterDB.Service to avoid copying the struct.
| err = os.MkdirAll(dbPath, os.ModePerm) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create sqlite store") |
There was a problem hiding this comment.
🪶 style: Consider providing more specific error information here.
| SkipDefaultTransaction: true, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not connect to db %s: %w", dbPath, err) |
There was a problem hiding this comment.
🪶 style: Wrap the error with more context about the failure.
| if !skipMigrations { | ||
| err = gdb.WithContext(ctx).AutoMigrate(base.GetAllModels()...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not migrate models: %w", err) |
There was a problem hiding this comment.
🪶 style: Wrap the error with more context about the migration failure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Outside diff range and nitpick comments (2)
contrib/opbot/botmd/middleware.go (1)
[!TIP]
Codebase VerificationThe
metricsMiddlewaremethod does not appear to be directly covered by tests, as indicated by the lack of references in the search results. Ensure that tests are added to cover:
- Metrics recording using
otelRecorder.- Handling of command attributes.
Files to review:
core/metrics/base.gocontrib/opbot/botmd/botmd.goAnalysis chain
Line range hint
55-55: Add tests for new functionality.The new
metricsMiddlewaremethod is not covered by tests. Ensure that tests are added to cover:
- Metrics recording using
otelRecorder.- Handling of command attributes.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for new functionality in `metricsMiddleware`. # Test: Search for test cases covering the `metricsMiddleware` method. Expect: Test cases covering all new functionality. rg --type go 'metricsMiddleware'Length of output: 393
Tools
GitHub Check: codecov/patch
[warning] 24-24: contrib/opbot/botmd/middleware.go#L24
Added line #L24 was not covered by tests
[warning] 29-30: contrib/opbot/botmd/middleware.go#L29-L30
Added lines #L29 - L30 were not covered by testsservices/rfq/api/docs/swagger.yaml (1)
Line range hint
1-174: Missing Security DefinitionsThe OpenAPI specification lacks security definitions. Ensure that security requirements are defined in
securityDefinitions, and the global security field has rules defined.securityDefinitions: Bearer: type: apiKey name: Authorization in: header security: - Bearer: []
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
contrib/opbot/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
Files selected for processing (30)
- contrib/opbot/botmd/botmd.go (4 hunks)
- contrib/opbot/botmd/commands.go (2 hunks)
- contrib/opbot/botmd/middleware.go (1 hunks)
- contrib/opbot/config/config.go (2 hunks)
- contrib/opbot/go.mod (3 hunks)
- contrib/opbot/sql/base/base.go (1 hunks)
- contrib/opbot/sql/base/doc.go (1 hunks)
- contrib/opbot/sql/doc.go (1 hunks)
- contrib/opbot/sql/mysql/doc.go (1 hunks)
- contrib/opbot/sql/mysql/store.go (1 hunks)
- contrib/opbot/sql/sqlite/doc.go (1 hunks)
- contrib/opbot/sql/sqlite/sqlite.go (1 hunks)
- contrib/opbot/sql/store.go (1 hunks)
- docs/bridge/docs/rfq/API/get-contract-addresses.api.mdx (1 hunks)
- docs/bridge/docs/rfq/API/relay-ack.api.mdx (1 hunks)
- ethergo/submitter/db/service.go (1 hunks)
- services/cctp-relayer/db/relayer_db.go (2 hunks)
- services/rfq/api/client/client.go (5 hunks)
- services/rfq/api/docs/docs.go (2 hunks)
- services/rfq/api/docs/swagger.json (2 hunks)
- services/rfq/api/docs/swagger.yaml (2 hunks)
- services/rfq/api/model/response.go (1 hunks)
- services/rfq/api/rest/handler.go (3 hunks)
- services/rfq/api/rest/server.go (3 hunks)
- services/rfq/api/rest/server_test.go (2 hunks)
- services/rfq/relayer/relapi/client.go (2 hunks)
- services/rfq/relayer/relapi/client_test.go (2 hunks)
- services/rfq/relayer/relapi/handler.go (1 hunks)
- services/rfq/relayer/relapi/model.go (1 hunks)
- services/rfq/relayer/relapi/server.go (2 hunks)
Files skipped from review due to trivial changes (7)
- contrib/opbot/go.mod
- contrib/opbot/sql/base/doc.go
- contrib/opbot/sql/doc.go
- contrib/opbot/sql/mysql/doc.go
- contrib/opbot/sql/sqlite/doc.go
- docs/bridge/docs/rfq/API/get-contract-addresses.api.mdx
- docs/bridge/docs/rfq/API/relay-ack.api.mdx
Additional context used
GitHub Check: codecov/patch
contrib/opbot/sql/base/base.go
[warning] 18-20: contrib/opbot/sql/base/base.go#L18-L20
Added lines #L18 - L20 were not covered by tests
[warning] 24-25: contrib/opbot/sql/base/base.go#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 29-30: contrib/opbot/sql/base/base.go#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 35-37: contrib/opbot/sql/base/base.go#L35-L37
Added lines #L35 - L37 were not covered by testscontrib/opbot/sql/mysql/store.go
[warning] 22-34: contrib/opbot/sql/mysql/store.go#L22-L34
Added lines #L22 - L34 were not covered by tests
[warning] 36-39: contrib/opbot/sql/mysql/store.go#L36-L39
Added lines #L36 - L39 were not covered by tests
[warning] 42-50: contrib/opbot/sql/mysql/store.go#L42-L50
Added lines #L42 - L50 were not covered by tests
[warning] 52-52: contrib/opbot/sql/mysql/store.go#L52
Added line #L52 was not covered by testscontrib/opbot/sql/sqlite/sqlite.go
[warning] 25-31: contrib/opbot/sql/sqlite/sqlite.go#L25-L31
Added lines #L25 - L31 were not covered by tests
[warning] 34-37: contrib/opbot/sql/sqlite/sqlite.go#L34-L37
Added lines #L34 - L37 were not covered by tests
[warning] 39-49: contrib/opbot/sql/sqlite/sqlite.go#L39-L49
Added lines #L39 - L49 were not covered by testscontrib/opbot/botmd/botmd.go
[warning] 48-49: contrib/opbot/botmd/botmd.go#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 51-51: contrib/opbot/botmd/botmd.go#L51
Added line #L51 was not covered by tests
[warning] 71-75: contrib/opbot/botmd/botmd.go#L71-L75
Added lines #L71 - L75 were not covered by tests
[warning] 77-80: contrib/opbot/botmd/botmd.go#L77-L80
Added lines #L77 - L80 were not covered by tests
[warning] 82-85: contrib/opbot/botmd/botmd.go#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 87-92: contrib/opbot/botmd/botmd.go#L87-L92
Added lines #L87 - L92 were not covered by tests
[warning] 94-96: contrib/opbot/botmd/botmd.go#L94-L96
Added lines #L94 - L96 were not covered by tests
[warning] 99-99: contrib/opbot/botmd/botmd.go#L99
Added line #L99 was not covered by testscontrib/opbot/botmd/middleware.go
[warning] 24-24: contrib/opbot/botmd/middleware.go#L24
Added line #L24 was not covered by tests
[warning] 29-30: contrib/opbot/botmd/middleware.go#L29-L30
Added lines #L29 - L30 were not covered by testscontrib/opbot/botmd/commands.go
[warning] 248-258: contrib/opbot/botmd/commands.go#L248-L258
Added lines #L248 - L258 were not covered by tests
[warning] 260-264: contrib/opbot/botmd/commands.go#L260-L264
Added lines #L260 - L264 were not covered by tests
[warning] 266-275: contrib/opbot/botmd/commands.go#L266-L275
Added lines #L266 - L275 were not covered by tests
[warning] 278-282: contrib/opbot/botmd/commands.go#L278-L282
Added lines #L278 - L282 were not covered by tests
[warning] 284-290: contrib/opbot/botmd/commands.go#L284-L290
Added lines #L284 - L290 were not covered by tests
[warning] 293-297: contrib/opbot/botmd/commands.go#L293-L297
Added lines #L293 - L297 were not covered by tests
[warning] 299-304: contrib/opbot/botmd/commands.go#L299-L304
Added lines #L299 - L304 were not covered by tests
[warning] 307-312: contrib/opbot/botmd/commands.go#L307-L312
Added lines #L307 - L312 were not covered by tests
[warning] 315-316: contrib/opbot/botmd/commands.go#L315-L316
Added lines #L315 - L316 were not covered by tests
GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/botmd.go
[failure] 30-30:
dbis unused (structcheck)contrib/opbot/botmd/commands.go
[failure] 315-315:
ineffectual assignment to err (ineffassign)
[failure] 308-308:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
checkov
services/rfq/api/docs/swagger.yaml
[HIGH] 1-174: Ensure that security requirement defined in securityDefinitions - version 2.0 files
(CKV_OPENAPI_6)
[HIGH] 1-174: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[HIGH] 1-174: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[HIGH] 1-174: Ensure that securityDefinitions is defined and not empty - version 2.0 files
(CKV_OPENAPI_1)
GitHub Check: Lint (services/rfq)
services/rfq/relayer/relapi/handler.go
[failure] 145-145:
exported methodHandler.GetQuoteRequestByTxIDshould have comment or be unexported (golint)
[failure] 171-171:
unnecessary trailing newline (whitespace)services/rfq/api/rest/server.go
[failure] 147-147:
Comment should end in a period (godot)
Additional comments not posted (29)
services/rfq/relayer/relapi/model.go (1)
28-31: Verify the field name and JSON tag.The field name
QuoteRequestRawand JSON tagquote_requestshould be verified for consistency with the rest of the codebase.contrib/opbot/sql/store.go (1)
14-36: Verify error handling and consider adding support for Clickhouse.The error handling for unsupported drivers is appropriate. Consider adding support for Clickhouse if needed.
services/cctp-relayer/db/relayer_db.go (1)
36-36: LGTM!The new interface
CCTPRelayerDBis well-formed and appropriately extends the necessary interfaces.contrib/opbot/config/config.go (2)
27-35: Ensure proper validation for new configuration fields.The new fields
RFQApiURL,OmniRPCURL,Signer,SubmitterConfig, andDatabaseshould have validation logic to ensure they are correctly populated.Do you have validation logic for these new fields elsewhere in the codebase?
39-41: Ensure secure handling of database credentials.The
DSNfield inDatabaseConfigshould be securely handled to avoid exposing sensitive information.Is the
DSNvalue securely handled and stored in the environment variables or a secure vault?services/rfq/api/model/response.go (1)
39-43: Ensure comprehensive documentation for the new response schema.The
GetContractsResponsestruct should have comprehensive documentation to describe the response schema accurately.Consider adding more detailed comments to describe the
Contractsfield and its expected structure.services/rfq/relayer/relapi/client_test.go (1)
54-63: Ensure comprehensive test coverage.The new test function
TestGetQuoteByTXlooks good and follows the pattern of other test functions in this suite. However, make sure that this test covers all edge cases, such as:
- Handling invalid or malformed transaction IDs.
- Ensuring the correct response when no quote request is found for a given transaction ID.
contrib/opbot/botmd/botmd.go (1)
71-99: Add tests for new functionality.The new functionality added to the
Startmethod is not covered by tests. Ensure that tests are added to cover:
- Signer creation.
- Database connection.
- Submission handling.
- Server listening using
errgroup.Tools
GitHub Check: codecov/patch
[warning] 71-75: contrib/opbot/botmd/botmd.go#L71-L75
Added lines #L71 - L75 were not covered by tests
[warning] 77-80: contrib/opbot/botmd/botmd.go#L77-L80
Added lines #L77 - L80 were not covered by tests
[warning] 82-85: contrib/opbot/botmd/botmd.go#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 87-92: contrib/opbot/botmd/botmd.go#L87-L92
Added lines #L87 - L92 were not covered by tests
[warning] 94-96: contrib/opbot/botmd/botmd.go#L94-L96
Added lines #L94 - L96 were not covered by tests
[warning] 99-99: contrib/opbot/botmd/botmd.go#L99
Added line #L99 was not covered by testsservices/rfq/relayer/relapi/client.go (1)
127-142: Ensure comprehensive test coverage.The new method
GetQuoteRequestByTXIDlooks good and follows the pattern of other methods in this client. However, make sure that this method is covered by tests, including edge cases such as:
- Handling invalid or malformed transaction IDs.
- Ensuring the correct response when no quote request is found for a given transaction ID.
services/rfq/relayer/relapi/server.go (2)
104-104: New Route Added:getRequestByTxIDThe new route
getRequestByTxIDhas been added. Ensure that the corresponding handler functionGetQuoteRequestByTxIDis implemented and tested.
121-121: Route Registration:getRequestByTxIDThe route
getRequestByTxIDis registered to the handler functionGetQuoteRequestByTxID. Ensure that the handler function is correctly implemented to handle this route.services/rfq/api/docs/swagger.yaml (2)
2-9: New Definition:model.GetContractsResponseA new definition
model.GetContractsResponsehas been added. Ensure that this model correctly represents the response schema for contract addresses.
98-114: New Endpoint:/contractsA new endpoint
/contractswith aGETmethod has been added to retrieve contract addresses. Ensure that this endpoint is correctly implemented and tested.services/rfq/api/rest/handler.go (3)
16-17: New Field:cfginHandlerStructA new field
cfghas been added to theHandlerstruct. Ensure that this field is correctly initialized and used in the handler methods.
21-24: New Handler InitializationThe
NewHandlerfunction now initializes thecfgfield in theHandlerstruct. Ensure that the configuration is correctly passed and used.
165-182: New Method:GetContractsA new method
GetContractshas been added to theHandlerstruct to retrieve contract addresses. Ensure that this method is correctly implemented and tested.ethergo/submitter/db/service.go (1)
50-53: New Interface:SubmitterDBFactoryA new interface
SubmitterDBFactoryhas been added with a methodSubmitterDB. Ensure that this interface is correctly implemented and used in the database services.services/rfq/api/client/client.go (2)
38-38: LGTM! Verify the implementation of the new method.The addition of the
GetRFQContractsmethod to theUnauthenticatedClientinterface looks good.Ensure that the method implementation aligns with the method signature.
Verification successful
Re-verify the implementation of the
GetRFQContractsmethod.The previous attempt to verify the method implementation failed due to a regex parse error. We need to escape special characters in the method signature to ensure the regex is parsed correctly.
Verify the implementation details of the
GetRFQContractsmethod.The
GetRFQContractsmethod implementation was found. Now, we need to inspect the contents of this method to ensure it is correctly implemented.
Verified the implementation of the
GetRFQContractsmethod.The implementation of the
GetRFQContractsmethod in theUnauthenticatedClientinterface has been successfully verified. The method correctly handles the context, sets the result, and processes errors as expected.
- Implementation found in:
services/rfq/api/client/client.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `GetRFQContracts` method. # Test: Ensure that the method implementation exists and matches the signature. rg --type go $'func (c unauthenticatedClient) GetRFQContracts(ctx context.Context) (*model.GetContractsResponse, error)'Length of output: 370
Script:
#!/bin/bash # Description: Verify the implementation of the `GetRFQContracts` method. # Test: Ensure that the method implementation exists and matches the signature. rg --type go 'func \(c unauthenticatedClient\) GetRFQContracts\(ctx context\.Context\) \(\*model\.GetContractsResponse, error\)'Length of output: 271
Script:
#!/bin/bash # Description: Inspect the implementation details of the `GetRFQContracts` method. # Test: Extract the method implementation. rg --type go --context 10 'func \(c unauthenticatedClient\) GetRFQContracts\(ctx context\.Context\) \(\*model\.GetContractsResponse, error\)'Length of output: 1398
213-229: LGTM! Verify the correctness of the new method implementation.The implementation of the
GetRFQContractsmethod looks good.Ensure that the method implementation works as expected and matches the interface signature.
Verification successful
Let's correct the regex pattern by escaping the parentheses and re-run the verification script.
Let's use
ast-grepto search for theGetRFQContractsmethod implementation.
Verification Successful!
The
GetRFQContractsmethod implementation inservices/rfq/api/client/client.gois correctly defined and matches the interface signature.
services/rfq/api/client/client.go:
- Lines 213-229:
func (c unauthenticatedClient) GetRFQContracts(ctx context.Context) (*model.GetContractsResponse, error) { ... }Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `GetRFQContracts` method implementation. # Test: Ensure that the method implementation works as expected and matches the interface signature. rg --type go $'func (c unauthenticatedClient) GetRFQContracts(ctx context.Context) (*model.GetContractsResponse, error)'Length of output: 370
Script:
#!/bin/bash # Description: Verify the correctness of the `GetRFQContracts` method implementation. # Test: Ensure that the method implementation works as expected and matches the interface signature. rg --type go $'func (c unauthenticatedClient) GetRFQContracts(ctx context.Context) \\(\\*model.GetContractsResponse, error\\)'Length of output: 124
Script:
#!/bin/bash # Description: Verify the correctness of the `GetRFQContracts` method implementation using ast-grep. # Test: Ensure that the method implementation works as expected and matches the interface signature. ast-grep --lang go --pattern $'func (c unauthenticatedClient) GetRFQContracts(ctx context.Context) (*model.GetContractsResponse, error) { $$$ }'Length of output: 1289
services/rfq/api/docs/swagger.json (3)
38-63: Update the description for the/contractsendpoint.The description for the
/contractsendpoint should accurately reflect its functionality. Consider updating it to something more descriptive.- "description": "get quotes from all relayers.", + "description": "retrieve contract addresses for RFQ operations.",
153-164: Update the description for thecontractsproperty.The description for the
contractsproperty should accurately reflect its functionality. Consider updating it to something more descriptive.- "description": "Contracts is a map of chain id to contract address", + "description": "A map where the key is the chain ID and the value is the contract address.",
Line range hint
164-175: LGTM!The definitions for
model.GetQuoteResponselook correct.services/rfq/api/docs/docs.go (2)
49-74: Update the description for the/contractsendpoint.The description for the
/contractsendpoint should accurately reflect its functionality. Consider updating it to something more descriptive.- "description": "get quotes from all relayers.", + "description": "retrieve contract addresses for RFQ operations.",
164-175: Update the description for thecontractsproperty.The description for the
contractsproperty should accurately reflect its functionality. Consider updating it to something more descriptive.- "description": "Contracts is a map of chain id to contract address", + "description": "A map where the key is the chain ID and the value is the contract address.",services/rfq/relayer/relapi/handler.go (1)
145-171: Add a comment describing theGetQuoteRequestByTxIDmethod.The
GetQuoteRequestByTxIDmethod lacks a comment describing its functionality. Consider adding a comment to improve code readability and maintainability.// GetQuoteRequestByTxID retrieves a quote request by transaction ID.Tools
GitHub Check: Lint (services/rfq)
[failure] 145-145:
exported methodHandler.GetQuoteRequestByTxIDshould have comment or be unexported (golint)
[failure] 171-171:
unnecessary trailing newline (whitespace)contrib/opbot/botmd/commands.go (3)
Line range hint
41-107: LGTM!The
traceCommandfunction is well-structured and handles errors appropriately.
Line range hint
109-246: LGTM!The
rfqLookupCommandfunction is well-structured and handles errors appropriately.Tools
GitHub Check: codecov/patch
[warning] 248-258: contrib/opbot/botmd/commands.go#L248-L258
Added lines #L248 - L258 were not covered by tests
[warning] 260-264: contrib/opbot/botmd/commands.go#L260-L264
Added lines #L260 - L264 were not covered by tests
[warning] 266-275: contrib/opbot/botmd/commands.go#L266-L275
Added lines #L266 - L275 were not covered by tests
[warning] 278-282: contrib/opbot/botmd/commands.go#L278-L282
Added lines #L278 - L282 were not covered by tests
[warning] 284-290: contrib/opbot/botmd/commands.go#L284-L290
Added lines #L284 - L290 were not covered by tests
[warning] 293-297: contrib/opbot/botmd/commands.go#L293-L297
Added lines #L293 - L297 were not covered by tests
[warning] 299-304: contrib/opbot/botmd/commands.go#L299-L304
Added lines #L299 - L304 were not covered by tests
[warning] 307-312: contrib/opbot/botmd/commands.go#L307-L312
Added lines #L307 - L312 were not covered by tests
[warning] 315-316: contrib/opbot/botmd/commands.go#L315-L316
Added lines #L315 - L316 were not covered by testsGitHub Check: Lint (contrib/opbot)
[failure] 315-315:
ineffectual assignment to err (ineffassign)
[failure] 308-308:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
Line range hint
321-329: LGTM!The
toExplorerSlackLinkfunction is simple and effective.Tools
GitHub Check: codecov/patch
[warning] 248-258: contrib/opbot/botmd/commands.go#L248-L258
Added lines #L248 - L258 were not covered by tests
[warning] 260-264: contrib/opbot/botmd/commands.go#L260-L264
Added lines #L260 - L264 were not covered by tests
[warning] 266-275: contrib/opbot/botmd/commands.go#L266-L275
Added lines #L266 - L275 were not covered by tests
[warning] 278-282: contrib/opbot/botmd/commands.go#L278-L282
Added lines #L278 - L282 were not covered by tests
[warning] 284-290: contrib/opbot/botmd/commands.go#L284-L290
Added lines #L284 - L290 were not covered by tests
[warning] 293-297: contrib/opbot/botmd/commands.go#L293-L297
Added lines #L293 - L297 were not covered by tests
[warning] 299-304: contrib/opbot/botmd/commands.go#L299-L304
Added lines #L299 - L304 were not covered by tests
[warning] 307-312: contrib/opbot/botmd/commands.go#L307-L312
Added lines #L307 - L312 were not covered by tests
[warning] 315-316: contrib/opbot/botmd/commands.go#L315-L316
Added lines #L315 - L316 were not covered by testsGitHub Check: Lint (contrib/opbot)
[failure] 315-315:
ineffectual assignment to err (ineffassign)
[failure] 308-308:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)services/rfq/api/rest/server_test.go (1)
343-354: LGTM!The
TestContractsfunction is straightforward and effectively tests the retrieval of RFQ contracts.
| // NewStore creates a new store. | ||
| func NewStore(db *gorm.DB, metrics metrics.Handler) *Store { | ||
| txDB := txdb.NewTXStore(db, metrics) | ||
| return &Store{db: db, metrics: metrics, submitterStore: txDB} |
There was a problem hiding this comment.
Add test coverage for NewStore function.
The NewStore function is not covered by tests. Ensure that test cases are added to cover this function.
Tools
GitHub Check: codecov/patch
[warning] 18-20: contrib/opbot/sql/base/base.go#L18-L20
Added lines #L18 - L20 were not covered by tests
| // DB gets the database object for mutation outside of the lib. | ||
| func (s Store) DB() *gorm.DB { | ||
| return s.db |
There was a problem hiding this comment.
Add test coverage for DB function.
The DB function is not covered by tests. Ensure that test cases are added to cover this function.
Tools
GitHub Check: codecov/patch
[warning] 24-25: contrib/opbot/sql/base/base.go#L24-L25
Added lines #L24 - L25 were not covered by tests
| // SubmitterDB gets the submitter database object for mutation outside of the lib. | ||
| func (s Store) SubmitterDB() submitterDB.Service { | ||
| return s.submitterStore |
There was a problem hiding this comment.
Add test coverage for SubmitterDB function.
The SubmitterDB function is not covered by tests. Ensure that test cases are added to cover this function.
Tools
GitHub Check: codecov/patch
[warning] 29-30: contrib/opbot/sql/base/base.go#L29-L30
Added lines #L29 - L30 were not covered by tests
| // GetAllModels gets all models to migrate. | ||
| // see: https://medium.com/@SaifAbid/slice-interfaces-8c78f8b6345d for an explanation of why we can't do this at initialization time | ||
| func GetAllModels() (allModels []interface{}) { | ||
| allModels = append(allModels, txdb.GetAllModels()...) | ||
| return allModels |
There was a problem hiding this comment.
Add test coverage for GetAllModels function.
The GetAllModels function is not covered by tests. Ensure that test cases are added to cover this function.
Tools
GitHub Check: codecov/patch
[warning] 35-37: contrib/opbot/sql/base/base.go#L35-L37
Added lines #L35 - L37 were not covered by tests
| func NewMysqlStore(ctx context.Context, dbURL string, handler metrics.Handler) (*Store, error) { | ||
| logger.Debug("create mysql store") | ||
|
|
||
| gdb, err := gorm.Open(mysql.Open(dbURL), &gorm.Config{ | ||
| Logger: common_base.GetGormLogger(logger), | ||
| FullSaveAssociations: true, | ||
| NamingStrategy: NamingStrategy, | ||
| NowFunc: time.Now, | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create mysql connection: %w", err) | ||
| } |
There was a problem hiding this comment.
Add unit tests for the NewMysqlStore function.
The NewMysqlStore function is not covered by tests. Ensure it is thoroughly tested to validate the database connection logic and error handling.
Do you want me to help generate unit tests for this function?
Tools
GitHub Check: codecov/patch
[warning] 22-34: contrib/opbot/sql/mysql/store.go#L22-L34
Added lines #L22 - L34 were not covered by tests
| return nil, fmt.Errorf("error from server: %s: %w", getStatus(resp), err) | ||
| } | ||
|
|
||
| if resp.IsError() { | ||
| return nil, fmt.Errorf("error from server: %s", resp.Status()) | ||
| return nil, fmt.Errorf("error from server: %s", getStatus(resp)) |
There was a problem hiding this comment.
Fix recursive call in getStatus function.
The getStatus function calls itself recursively, causing a stack overflow. It should return resp.Status() instead.
- return fmt.Errorf("error from server: %s: %w", getStatus(resp), err)
+ return fmt.Errorf("error from server: %s: %w", resp.Status(), err)Committable suggestion was skipped due to low confidence.
services/rfq/api/client/client.go
Outdated
| func getStatus(resp *resty.Response) string { | ||
| if resp == nil { | ||
| return "http status unavailable" | ||
| } | ||
| return getStatus(resp) |
There was a problem hiding this comment.
Fix recursive call in the getStatus function.
The getStatus function calls itself recursively, causing a stack overflow. It should return resp.Status() instead.
- return getStatus(resp)
+ return resp.Status()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.
| func getStatus(resp *resty.Response) string { | |
| if resp == nil { | |
| return "http status unavailable" | |
| } | |
| return getStatus(resp) | |
| func getStatus(resp *resty.Response) string { | |
| if resp == nil { | |
| return "http status unavailable" | |
| } | |
| return resp.Status() |
| return nil, fmt.Errorf("error from server: %s: %w", getStatus(resp), err) | ||
| } | ||
|
|
||
| if resp.IsError() { | ||
| return nil, fmt.Errorf("error from server: %s", resp.Status()) | ||
| return nil, fmt.Errorf("error from server: %s", getStatus(resp)) |
There was a problem hiding this comment.
Fix recursive call in getStatus function.
The getStatus function calls itself recursively, causing a stack overflow. It should return resp.Status() instead.
- return fmt.Errorf("error from server: %s: %w", getStatus(resp), err)
+ return fmt.Errorf("error from server: %s: %w", resp.Status(), err)Committable suggestion was skipped due to low confidence.
| cmdCtx.WithContext(ctx) | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
Add tests for new functionality.
The tracingMiddleware method is not covered by any existing test cases. Please add tests to ensure the following functionalities are covered:
-
Context inheritance using
ctx. -
Span creation with appropriate attributes.
-
contrib/opbot/botmd/middleware.go
Analysis chain
Add tests for new functionality.
The new functionality added to the tracingMiddleware method is not covered by tests. Ensure that tests are added to cover:
- Context inheritance using
ctx. - Span creation with appropriate attributes.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for new functionality in `tracingMiddleware`.
# Test: Search for test cases covering the `tracingMiddleware` method. Expect: Test cases covering all new functionality.
rg --type go 'tracingMiddleware'
Length of output: 233
Script:
#!/bin/bash
# Description: Verify if there are tests covering the `tracingMiddleware` method.
# Find all Go test files in the repository
test_files=$(fd --type f --extension go --glob '*_test.go')
# Search for test cases covering the `tracingMiddleware` method in the test files
for file in $test_files; do
echo "Checking $file"
rg 'tracingMiddleware' "$file"
done
Length of output: 75047
Tools
GitHub Check: codecov/patch
[warning] 29-30: contrib/opbot/botmd/middleware.go#L29-L30
Added lines #L29 - L30 were not covered by tests
| @@ -21,12 +21,13 @@ | |||
| func (b *Bot) tracingMiddleware() slacker.CommandMiddlewareHandler { | |||
| return func(next slacker.CommandHandler) slacker.CommandHandler { | |||
| return func(cmdCtx *slacker.CommandContext) { | |||
| // TODO: context is not inherited here. | |||
| _, span := b.handler.Tracer().Start(cmdCtx.Context(), fmt.Sprintf("command.%s", cmdCtx.Definition().Command), trace.WithAttributes( | |||
| ctx, span := b.handler.Tracer().Start(cmdCtx.Context(), fmt.Sprintf("command.%s", cmdCtx.Definition().Command), trace.WithAttributes( | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Add tests for new functionality.
The new functionality added to the tracingMiddleware method is not covered by tests. Ensure that tests are added to cover:
-
Context inheritance using
ctx. -
Span creation with appropriate attributes.
-
contrib/opbot/botmd/middleware.go
Analysis chain
Add tests for new functionality.
The new functionality added to the tracingMiddleware method is not covered by tests. Ensure that tests are added to cover:
- Context inheritance using
ctx. - Span creation with appropriate attributes.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for new functionality in `tracingMiddleware`.
# Test: Search for test cases covering the `tracingMiddleware` method. Expect: Test cases covering all new functionality.
rg --type go 'tracingMiddleware'
Length of output: 233
Script:
#!/bin/bash
# Description: Verify test coverage for the `tracingMiddleware` method by searching for test files and functions.
# Find all test files in the repository.
fd --type f --name '*_test.go'
# Search for test functions that might cover `tracingMiddleware`.
rg --type go 'func Test' | rg 'tracingMiddleware'
Length of output: 333
Script:
#!/bin/bash
# Description: Verify test coverage for the `tracingMiddleware` method by searching for test files and functions.
# Find all test files in the repository.
fd '_test.go$'
# Search for references to `tracingMiddleware` within test files.
rg 'tracingMiddleware' $(fd '_test.go$')
Length of output: 25161
Tools
GitHub Check: codecov/patch
[warning] 24-24: contrib/opbot/botmd/middleware.go#L24
Added line #L24 was not covered by tests
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added RFQ refund command to OpBot (
contrib/opbot/botmd/botmd.go) - Enhanced error handling in
rfqRefundcommand (contrib/opbot/botmd/commands.go) - Updated dependencies to support MySQL and SQLite databases (
contrib/opbot/go.mod) - Updated
gorilla/websocketdependency from v1.5.1 to v1.5.3 across multiple modules (agents/go.mod,contrib/promexporter/go.mod,contrib/screener-api/go.mod,core/go.mod,ethergo/go.mod,services/cctp-relayer/go.mod,services/explorer/go.mod,services/omnirpc/go.mod,services/rfq/go.mod,services/scribe/go.mod,tools/go.mod)
14 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
| b.signer, err = signerConfig.SignerFromConfig(ctx, b.cfg.Signer) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create signer: %w", err) |
There was a problem hiding this comment.
🧠 logic: Error handling for signer creation should be reviewed to ensure it covers all edge cases.
| dbType, err := dbcommon.DBTypeFromString(b.cfg.Database.Type) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get db type: %w", err) |
There was a problem hiding this comment.
🧠 logic: Error handling for database type conversion should be reviewed to ensure it covers all edge cases.
| store, err := cctpSql.Connect(ctx, dbType, b.cfg.Database.DSN, b.handler) | ||
| if err != nil { | ||
| return fmt.Errorf("could not connect to database: %w", err) |
There was a problem hiding this comment.
🧠 logic: Error handling for database connection should be reviewed to ensure it covers all edge cases.
There was a problem hiding this comment.
Actionable comments posted: 10
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (12)
agents/go.sumis excluded by!**/*.sumcontrib/opbot/go.sumis excluded by!**/*.sumcontrib/promexporter/go.sumis excluded by!**/*.sumcontrib/screener-api/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sumethergo/go.sumis excluded by!**/*.sumservices/cctp-relayer/go.sumis excluded by!**/*.sumservices/explorer/go.sumis excluded by!**/*.sumservices/omnirpc/go.sumis excluded by!**/*.sumservices/rfq/go.sumis excluded by!**/*.sumservices/scribe/go.sumis excluded by!**/*.sumtools/go.sumis excluded by!**/*.sum
Files selected for processing (14)
- agents/go.mod (1 hunks)
- contrib/opbot/botmd/botmd.go (4 hunks)
- contrib/opbot/botmd/commands.go (2 hunks)
- contrib/opbot/go.mod (11 hunks)
- contrib/promexporter/go.mod (1 hunks)
- contrib/screener-api/go.mod (1 hunks)
- core/go.mod (1 hunks)
- ethergo/go.mod (1 hunks)
- services/cctp-relayer/go.mod (1 hunks)
- services/explorer/go.mod (1 hunks)
- services/omnirpc/go.mod (1 hunks)
- services/rfq/go.mod (1 hunks)
- services/scribe/go.mod (1 hunks)
- tools/go.mod (1 hunks)
Files skipped from review due to trivial changes (11)
- agents/go.mod
- contrib/promexporter/go.mod
- contrib/screener-api/go.mod
- core/go.mod
- ethergo/go.mod
- services/cctp-relayer/go.mod
- services/explorer/go.mod
- services/omnirpc/go.mod
- services/rfq/go.mod
- services/scribe/go.mod
- tools/go.mod
Additional context used
GitHub Check: codecov/patch
contrib/opbot/botmd/botmd.go
[warning] 46-47: contrib/opbot/botmd/botmd.go#L46-L47
Added lines #L46 - L47 were not covered by tests
[warning] 49-49: contrib/opbot/botmd/botmd.go#L49
Added line #L49 was not covered by tests
[warning] 69-73: contrib/opbot/botmd/botmd.go#L69-L73
Added lines #L69 - L73 were not covered by tests
[warning] 75-78: contrib/opbot/botmd/botmd.go#L75-L78
Added lines #L75 - L78 were not covered by tests
[warning] 80-83: contrib/opbot/botmd/botmd.go#L80-L83
Added lines #L80 - L83 were not covered by tests
[warning] 85-90: contrib/opbot/botmd/botmd.go#L85-L90
Added lines #L85 - L90 were not covered by tests
[warning] 92-94: contrib/opbot/botmd/botmd.go#L92-L94
Added lines #L92 - L94 were not covered by tests
[warning] 97-97: contrib/opbot/botmd/botmd.go#L97
Added line #L97 was not covered by testscontrib/opbot/botmd/commands.go
[warning] 248-258: contrib/opbot/botmd/commands.go#L248-L258
Added lines #L248 - L258 were not covered by tests
[warning] 260-264: contrib/opbot/botmd/commands.go#L260-L264
Added lines #L260 - L264 were not covered by tests
[warning] 266-275: contrib/opbot/botmd/commands.go#L266-L275
Added lines #L266 - L275 were not covered by tests
[warning] 278-282: contrib/opbot/botmd/commands.go#L278-L282
Added lines #L278 - L282 were not covered by tests
[warning] 284-290: contrib/opbot/botmd/commands.go#L284-L290
Added lines #L284 - L290 were not covered by tests
[warning] 293-297: contrib/opbot/botmd/commands.go#L293-L297
Added lines #L293 - L297 were not covered by tests
[warning] 299-304: contrib/opbot/botmd/commands.go#L299-L304
Added lines #L299 - L304 were not covered by tests
[warning] 307-312: contrib/opbot/botmd/commands.go#L307-L312
Added lines #L307 - L312 were not covered by tests
[warning] 315-319: contrib/opbot/botmd/commands.go#L315-L319
Added lines #L315 - L319 were not covered by tests
GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 308-308:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
Additional comments not posted (1)
contrib/opbot/go.mod (1)
17-17: Verify the necessity of new dependencies.Ensure that the newly added dependencies are necessary and correctly used in the codebase.
Also applies to: 27-28, 34-34, 37-39, 54-54, 138-141, 145-145, 155-155, 165-165, 170-170, 190-190, 222-223, 226-228, 289-289
Verification successful
The dependency
github.com/ipfs/go-log v1.0.5is necessary.The usage of
github.com/ipfs/go-loghas been confirmed across multiple files within the codebase, indicating its necessity.
services/scribe/graphql/server/graph/utils.goservices/scribe/testhelper/scribe.goservices/scribe/logger/handler.goservices/scribe/db/datastore/sql/base/log.goservices/scribe/db/datastore/sql/mysql/logger.goservices/scribe/api/server.goservices/omnirpc/http/logger.goservices/omnirpc/proxy/logger.goservices/omnirpc/modules/harmonyproxy/harmonyproxy.goservices/omnirpc/chainmanager/manager.goservices/rfq/relayer/reldb/mysql/mysql.goservices/rfq/relayer/service/statushandler.goservices/rfq/api/rest/server.goservices/explorer/db/sql/store.goservices/explorer/node/logger.goservices/explorer/backfill/logger.goservices/explorer/consumer/fetcher/logger.goservices/explorer/consumer/parser/logger.goservices/explorer/graphql/server/graph/fetcher.goservices/rfq/api/db/sql/mysql/store.goethergo/parser/hardhat/logger.goethergo/signer/nonce/logger.goethergo/submitter/submitter.goethergo/signer/signer/awssigner/kmsmock/logger.goethergo/listener/listener.goethergo/deployer/logger.goethergo/client/methods.goethergo/forker/logger.goethergo/chain/chainwatcher/log.goethergo/chain/watcher/log.goethergo/backends/geth/logger.goethergo/backends/simulated/simulated.goethergo/chain/logger.goethergo/chain/gas/gas_price.goethergo/backends/base/base.goethergo/chain/client/logger.goethergo/backends/anvil/anvil.gotools/abigen/internal/etherscan/logger.gocontrib/screener-api/screener/screener.gocontrib/screener-api/db/sql/sqlite/sqlite.gocontrib/screener-api/db/sql/mysql/store.gocore/server/logger.gocore/metrics/logger.gocore/metrics/localmetrics/logger.gocore/dbcommon/gormlog.gocore/ginhelper/wait_test.gocore/dockerutil/docker.gocore/ginhelper/suite_test.gocore/dbcommon/enum_test.gocore/commandline/logger.gocore/commandline/loglevel_example_test.gocore/commandline/loglevel.gocore/ginhelper/server.gocontrib/promexporter/exporters/exporter.gocontrib/promexporter/internal/gql/explorer/contrib/main.gocontrib/opbot/sql/sqlite/sqlite.gocontrib/opbot/sql/mysql/store.gocontrib/opbot/sql/sqlite/sqlite.goagents/agents/executor/logger.goagents/agents/guard/logger.goagents/agents/guard/api/server.goagents/agents/executor/db/sql/sqlite/logger.goagents/agents/executor/db/sql/mysql/logger.goagents/agents/executor/api/server.goagents/agents/notary/logger.goagents/agents/notary/api/server.goagents/agents/notary/db/sql/mysql/store.goagents/agents/notary/db/sql/sqlite/sqlite.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new dependencies in the codebase. # Test: Search for usage of new dependencies. Expect: Usage in the codebase. rg --type go 'github.com/ipfs/go-log|github.com/synapsecns/sanguine/services/cctp-relayer|github.com/synapsecns/sanguine/services/omnirpc|gorm.io/driver/mysql|gorm.io/driver/sqlite|gorm.io/gorm|github.com/go-openapi/jsonpointer|github.com/go-openapi/jsonreference|github.com/go-openapi/spec|github.com/go-openapi/swag|github.com/go-sql-driver/mysql|github.com/jellydator/ttlcache/v3|github.com/mattn/go-sqlite3|github.com/swaggo/files|github.com/swaggo/gin-swagger|github.com/swaggo/swag'Length of output: 35458
| bot.rpcClient = omnirpcClient.NewOmnirpcClient(cfg.OmniRPCURL, handler, omnirpcClient.WithCaptureReqRes()) | ||
|
|
There was a problem hiding this comment.
Add tests for the new functionality.
The rpcClient initialization lacks test coverage. Ensure that tests are added to cover this initialization.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 46-47: contrib/opbot/botmd/botmd.go#L46-L47
Added lines #L46 - L47 were not covered by tests
| bot.addMiddleware(bot.tracingMiddleware(), bot.metricsMiddleware()) | ||
| bot.addCommands(bot.traceCommand(), bot.rfqLookupCommand()) | ||
| bot.addCommands(bot.traceCommand(), bot.rfqLookupCommand(), bot.rfqRefund()) |
There was a problem hiding this comment.
Add tests for the new functionality.
The addition of the rfqRefund command lacks test coverage. Ensure that tests are added to cover this functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 49-49: contrib/opbot/botmd/botmd.go#L49
Added line #L49 was not covered by tests
| var err error | ||
| b.signer, err = signerConfig.SignerFromConfig(ctx, b.cfg.Signer) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create signer: %w", err) | ||
| } |
There was a problem hiding this comment.
Add tests for the new functionality.
The initialization of the signer lacks test coverage. Ensure that tests are added to cover this initialization.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 69-73: contrib/opbot/botmd/botmd.go#L69-L73
Added lines #L69 - L73 were not covered by tests
| dbType, err := dbcommon.DBTypeFromString(b.cfg.Database.Type) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get db type: %w", err) | ||
| } |
There was a problem hiding this comment.
Add tests for the new functionality.
The database connection lacks test coverage. Ensure that tests are added to cover this functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 75-78: contrib/opbot/botmd/botmd.go#L75-L78
Added lines #L75 - L78 were not covered by tests
| store, err := cctpSql.Connect(ctx, dbType, b.cfg.Database.DSN, b.handler) | ||
| if err != nil { | ||
| return fmt.Errorf("could not connect to database: %w", err) | ||
| } |
There was a problem hiding this comment.
Add tests for the new functionality.
The connection to the database lacks test coverage. Ensure that tests are added to cover this functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 80-83: contrib/opbot/botmd/botmd.go#L80-L83
Added lines #L80 - L83 were not covered by tests
| b.submitter = submitter.NewTransactionSubmitter(b.handler, b.signer, b.rpcClient, store.SubmitterDB(), &b.cfg.SubmitterConfig) | ||
|
|
||
| g, ctx := errgroup.WithContext(ctx) | ||
| g.Go(func() error { | ||
| return b.submitter.Start(ctx) | ||
| }) |
There was a problem hiding this comment.
Add tests for the new functionality.
The initialization and starting of the submitter lack test coverage. Ensure that tests are added to cover this functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 85-90: contrib/opbot/botmd/botmd.go#L85-L90
Added lines #L85 - L90 were not covered by tests
| g.Go(func() error { | ||
| return b.server.Listen(ctx) | ||
| }) |
There was a problem hiding this comment.
Add tests for the new functionality.
The starting of the server lacks test coverage. Ensure that tests are added to cover this functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 92-94: contrib/opbot/botmd/botmd.go#L92-L94
Added lines #L92 - L94 were not covered by tests
contrib/opbot/botmd/commands.go
Outdated
| } | ||
|
|
||
| nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(originChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| return fastBridgeHandle.Refund(transactor, common.Hex2Bytes(qr.QuoteRequestRaw)) |
There was a problem hiding this comment.
Wrap the error returned from the external package.
The error returned from fastBridgeHandle.Refund should be wrapped to provide more context.
- return fastBridgeHandle.Refund(transactor, common.Hex2Bytes(qr.QuoteRequestRaw))
+ tx, err := fastBridgeHandle.Refund(transactor, common.Hex2Bytes(qr.QuoteRequestRaw))
+ if err != nil {
+ return nil, fmt.Errorf("failed to refund: %w", err)
+ }
+ return tx, nilCommittable 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 fastBridgeHandle.Refund(transactor, common.Hex2Bytes(qr.QuoteRequestRaw)) | |
| tx, err := fastBridgeHandle.Refund(transactor, common.Hex2Bytes(qr.QuoteRequestRaw)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to refund: %w", err) | |
| } | |
| return tx, nil |
Tools
GitHub Check: Lint (contrib/opbot)
[failure] 308-308:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced
rfqRefundcommand for handling refund operations (contrib/opbot/botmd/commands.go) - Improved context propagation in middleware (
contrib/opbot/botmd/commands.go) - Renamed
OmniRPCURLfield for consistency (contrib/opbot/config/config.go) - Added
ChainNameToChainIDfunction for chain name conversion (ethergo/chaindata/chaindata.go) - Enhanced private key handling in
FromHexfunction (ethergo/signer/wallet/import.go)
7 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- contrib/opbot/botmd/commands.go (3 hunks)
- contrib/opbot/cmd/commands.go (2 hunks)
- contrib/opbot/config/config.go (2 hunks)
- ethergo/chaindata/chaindata.go (1 hunks)
- ethergo/signer/wallet/import.go (1 hunks)
- services/omnirpc/client/client.go (2 hunks)
- services/rfq/api/client/client.go (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- contrib/opbot/config/config.go
- services/rfq/api/client/client.go
Additional context used
GitHub Check: codecov/patch
contrib/opbot/cmd/commands.go
[warning] 27-27: contrib/opbot/cmd/commands.go#L27
Added line #L27 was not covered by testscontrib/opbot/botmd/commands.go
[warning] 249-259: contrib/opbot/botmd/commands.go#L249-L259
Added lines #L249 - L259 were not covered by tests
[warning] 261-263: contrib/opbot/botmd/commands.go#L261-L263
Added lines #L261 - L263 were not covered by tests
[warning] 268-269: contrib/opbot/botmd/commands.go#L268-L269
Added lines #L268 - L269 were not covered by tests
[warning] 271-271: contrib/opbot/botmd/commands.go#L271
Added line #L271 was not covered by tests
[warning] 281-281: contrib/opbot/botmd/commands.go#L281
Added line #L281 was not covered by tests
[warning] 289-289: contrib/opbot/botmd/commands.go#L289
Added line #L289 was not covered by tests
[warning] 291-296: contrib/opbot/botmd/commands.go#L291-L296
Added lines #L291 - L296 were not covered by tests
[warning] 299-303: contrib/opbot/botmd/commands.go#L299-L303
Added lines #L299 - L303 were not covered by tests
[warning] 305-311: contrib/opbot/botmd/commands.go#L305-L311
Added lines #L305 - L311 were not covered by tests
[warning] 314-318: contrib/opbot/botmd/commands.go#L314-L318
Added lines #L314 - L318 were not covered by tests
[warning] 320-321: contrib/opbot/botmd/commands.go#L320-L321
Added lines #L320 - L321 were not covered by tests
[warning] 324-324: contrib/opbot/botmd/commands.go#L324
Added line #L324 was not covered by tests
[warning] 332-332: contrib/opbot/botmd/commands.go#L332
Added line #L332 was not covered by tests
[warning] 334-337: contrib/opbot/botmd/commands.go#L334-L337
Added lines #L334 - L337 were not covered by tests
[warning] 340-344: contrib/opbot/botmd/commands.go#L340-L344
Added lines #L340 - L344 were not covered by tests
GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 249-249:
cognitive complexity 46 of func(*Bot).rfqRefundis high (> 30) (gocognit)
[failure] 333-333:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
[failure] 377-377:
Comment should end in a period (godot)
Additional comments not posted (2)
contrib/opbot/cmd/commands.go (1)
27-27: Verify the usage ofcore.ExpandOrReturnPathand add tests.The modification to use
core.ExpandOrReturnPathfor reading the config file path seems correct. Ensure that this function is correctly used throughout the codebase, and add tests to cover this change.Would you like me to generate the unit testing code or open a GitHub issue to track this task?
Verification successful
Verified: The usage of
core.ExpandOrReturnPathincontrib/opbot/cmd/commands.goaligns with the standardized approach across the codebase. The change is correct and no further issues are found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `core.ExpandOrReturnPath` in the codebase. # Test: Search for the usage of `core.ExpandOrReturnPath`. Expect: Correct usage in relevant files. rg --type go $'core.ExpandOrReturnPath'Length of output: 2973
Tools
GitHub Check: codecov/patch
[warning] 27-27: contrib/opbot/cmd/commands.go#L27
Added line #L27 was not covered by testscontrib/opbot/botmd/commands.go (1)
6-22: LGTM!The new imports are appropriate for the
rfqRefundfunction.
contrib/opbot/botmd/commands.go
Outdated
| func (b *Bot) rfqRefund() *slacker.CommandDefinition { | ||
| return &slacker.CommandDefinition{ | ||
| Command: "refund <tx> <origin_chainid>", | ||
| Description: "refund a quote request", | ||
| Examples: []string{"refund 0x1234"}, | ||
| Handler: func(ctx *slacker.CommandContext) { | ||
| client, err := rfqClient.NewUnauthenticatedClient(b.handler, b.cfg.RFQApiURL) | ||
| if err != nil { | ||
| log.Printf("error creating rfq client: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| contracts, err := client.GetRFQContracts(ctx.Context()) | ||
| if err != nil { | ||
| log.Printf("error fetching rfq contracts: %v\n", err) | ||
| _, err = ctx.Response().Reply("error fetching rfq contracts") | ||
| if err != nil { | ||
| return | ||
| } | ||
| return | ||
| } | ||
|
|
||
| tx := stripLinks(ctx.Request().Param("tx")) | ||
|
|
||
| if len(tx) == 0 { | ||
| _, err := ctx.Response().Reply("please provide a tx hash") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| originChainIDStr := ctx.Request().Param("origin_chainid") | ||
| if len(originChainIDStr) == 0 { | ||
| _, err := ctx.Response().Reply("please provide an origin chain id") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| originChainID, err := strconv.Atoi(convertChainName(originChainIDStr)) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("origin_chainid must be a number") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| chainClient, err := b.rpcClient.GetChainClient(ctx.Context(), originChainID) | ||
| if err != nil { | ||
| log.Printf("error getting chain client: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| contractAddress, ok := contracts.Contracts[uint32(originChainID)] | ||
| if !ok { | ||
| _, err := ctx.Response().Reply("contract address not found") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| fastBridgeHandle, err := fastbridge.NewFastBridge(common.HexToAddress(contractAddress), chainClient) | ||
| if err != nil { | ||
| log.Printf("error creating fast bridge: %v\n", err) | ||
| return | ||
| } | ||
|
|
||
| for _, relayer := range b.cfg.RelayerURLS { | ||
| 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 | ||
| } | ||
|
|
||
| nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(originChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| return fastBridgeHandle.Refund(transactor, rawRequest) | ||
| }) | ||
| if err != nil { | ||
| log.Printf("error submitting refund: %v\n", err) | ||
| continue | ||
| } | ||
|
|
||
| _, 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.
Reduce the cognitive complexity of the rfqRefund function.
The function has a high cognitive complexity (46). Consider refactoring to reduce complexity.
One way to reduce complexity is to break down the function into smaller, more manageable functions. For example:
func (b *Bot) rfqRefund() *slacker.CommandDefinition {
return &slacker.CommandDefinition{
Command: "refund <tx> <origin_chainid>",
Description: "refund a quote request",
Examples: []string{"refund 0x1234"},
Handler: func(ctx *slacker.CommandContext) {
if err := b.handleRFQRefund(ctx); err != nil {
log.Println(err)
}
},
}
}
func (b *Bot) handleRFQRefund(ctx *slacker.CommandContext) error {
client, err := rfqClient.NewUnauthenticatedClient(b.handler, b.cfg.RFQApiURL)
if err != nil {
return fmt.Errorf("error creating rfq client: %w", err)
}
contracts, err := client.GetRFQContracts(ctx.Context())
if err != nil {
return b.respondWithError(ctx, "error fetching rfq contracts", err)
}
tx := stripLinks(ctx.Request().Param("tx"))
if len(tx) == 0 {
return b.respondWithError(ctx, "please provide a tx hash", nil)
}
originChainID, err := b.getOriginChainID(ctx)
if err != nil {
return err
}
chainClient, err := b.rpcClient.GetChainClient(ctx.Context(), originChainID)
if err != nil {
return fmt.Errorf("error getting chain client: %w", err)
}
contractAddress, ok := contracts.Contracts[uint32(originChainID)]
if !ok {
return b.respondWithError(ctx, "contract address not found", nil)
}
fastBridgeHandle, err := fastbridge.NewFastBridge(common.HexToAddress(contractAddress), chainClient)
if err != nil {
return fmt.Errorf("error creating fast bridge: %w", err)
}
return b.submitRefund(ctx, fastBridgeHandle, tx, originChainID)
}
func (b *Bot) respondWithError(ctx *slacker.CommandContext, message string, err error) error {
if err != nil {
log.Printf("%s: %v\n", message, err)
}
_, respErr := ctx.Response().Reply(message)
if respErr != nil {
log.Println(respErr)
}
return err
}
func (b *Bot) getOriginChainID(ctx *slacker.CommandContext) (int, error) {
originChainIDStr := ctx.Request().Param("origin_chainid")
if len(originChainIDStr) == 0 {
return 0, b.respondWithError(ctx, "please provide an origin chain id", nil)
}
originChainID, err := strconv.Atoi(convertChainName(originChainIDStr))
if err != nil {
return 0, b.respondWithError(ctx, "origin_chainid must be a number", err)
}
return originChainID, nil
}
func (b *Bot) submitRefund(ctx *slacker.CommandContext, fastBridgeHandle *fastbridge.FastBridgeTransactor, tx string, originChainID int) error {
for _, relayer := range b.cfg.RelayerURLS {
relClient := relapi.NewRelayerClient(b.handler, relayer)
rawRequest, err := getQuoteRequest(ctx.Context(), relClient, tx)
if err != nil {
return b.respondWithError(ctx, "error fetching quote request", err)
}
nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(originChainID)), func(transactor *bind.TransactOpts) (*types.Transaction, error) {
return fastBridgeHandle.Refund(transactor, rawRequest)
})
if err != nil {
log.Printf("error submitting refund: %v\n", err)
continue
}
if _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)); err != nil {
log.Println(err)
}
return nil
}
return nil
}Tools
GitHub Check: codecov/patch
[warning] 249-259: contrib/opbot/botmd/commands.go#L249-L259
Added lines #L249 - L259 were not covered by tests
[warning] 261-263: contrib/opbot/botmd/commands.go#L261-L263
Added lines #L261 - L263 were not covered by tests
[warning] 268-269: contrib/opbot/botmd/commands.go#L268-L269
Added lines #L268 - L269 were not covered by tests
[warning] 271-271: contrib/opbot/botmd/commands.go#L271
Added line #L271 was not covered by tests
[warning] 281-281: contrib/opbot/botmd/commands.go#L281
Added line #L281 was not covered by tests
[warning] 289-289: contrib/opbot/botmd/commands.go#L289
Added line #L289 was not covered by tests
[warning] 291-296: contrib/opbot/botmd/commands.go#L291-L296
Added lines #L291 - L296 were not covered by tests
[warning] 299-303: contrib/opbot/botmd/commands.go#L299-L303
Added lines #L299 - L303 were not covered by tests
[warning] 305-311: contrib/opbot/botmd/commands.go#L305-L311
Added lines #L305 - L311 were not covered by tests
[warning] 314-318: contrib/opbot/botmd/commands.go#L314-L318
Added lines #L314 - L318 were not covered by tests
[warning] 320-321: contrib/opbot/botmd/commands.go#L320-L321
Added lines #L320 - L321 were not covered by tests
[warning] 324-324: contrib/opbot/botmd/commands.go#L324
Added line #L324 was not covered by tests
[warning] 332-332: contrib/opbot/botmd/commands.go#L332
Added line #L332 was not covered by tests
[warning] 334-337: contrib/opbot/botmd/commands.go#L334-L337
Added lines #L334 - L337 were not covered by tests
[warning] 340-344: contrib/opbot/botmd/commands.go#L340-L344
Added lines #L340 - L344 were not covered by tests
GitHub Check: Lint (contrib/opbot)
[failure] 249-249:
cognitive complexity 46 of func(*Bot).rfqRefundis high (> 30) (gocognit)
[failure] 333-333:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
contrib/opbot/botmd/commands.go
Outdated
| } | ||
|
|
||
| nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(originChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| return fastBridgeHandle.Refund(transactor, rawRequest) |
There was a problem hiding this comment.
Wrap the error returned from the external package.
The error returned from fastBridgeHandle.Refund should be wrapped to provide more context.
- return fastBridgeHandle.Refund(transactor, rawRequest)
+ tx, err := fastBridgeHandle.Refund(transactor, rawRequest)
+ if err != nil {
+ return nil, fmt.Errorf("failed to refund: %w", err)
+ }
+ return tx, nilCommittable 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 fastBridgeHandle.Refund(transactor, rawRequest) | |
| tx, err := fastBridgeHandle.Refund(transactor, rawRequest) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to refund: %w", err) | |
| } | |
| return tx, nil |
Tools
GitHub Check: Lint (contrib/opbot)
[failure] 333-333:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
| // Check for '0x' prefix and remove it if it exists | ||
| if len(privateKey) >= 2 && strings.EqualFold(privateKey[:2], "0x") { | ||
| privateKey = privateKey[2:] | ||
| } |
There was a problem hiding this comment.
LGTM! But add tests to cover the change.
The modification to handle the '0x' prefix in the FromHex function seems correct. Ensure that this change is covered by tests.
Would you like me to generate the unit testing code or open a GitHub issue to track this task?
services/omnirpc/client/client.go
Outdated
| func (c *rpcClient) GetEndpoint(chainID, confirmations int) (res string) { | ||
| defer func() { | ||
| res = strings.ReplaceAll(c.endpoint, "://", "TEMP_PROTOCOL") | ||
| res = strings.ReplaceAll(c.endpoint, "//", "/") | ||
| res = strings.ReplaceAll(c.endpoint, "TEMP_PROTOCOL", "://") | ||
| }() |
There was a problem hiding this comment.
LGTM! But add tests to cover the change.
The modification to manipulate the endpoint string in the GetEndpoint function seems correct. Ensure that this change is covered by tests.
Would you like me to generate the unit testing code or open a GitHub issue to track this task?
| // ChainNameToChainID converts the chain name to the chain id. | ||
| // It returns 0 if the chain name is not found. | ||
| func ChainNameToChainID(chainName string) uint64 { | ||
| for _, chainMetadata := range ChainMetadataList { | ||
| if strings.EqualFold(chainMetadata.ChainName, chainName) { | ||
| return uint64(chainMetadata.ChainID) | ||
| } | ||
| } | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
LGTM! But add tests to cover the new function.
The addition of the ChainNameToChainID function seems correct. Ensure that this function is covered by tests.
Would you like me to generate the unit testing code or open a GitHub issue to track this task?
Bundle ReportChanges will decrease total bundle size by 324.22kB ⬇️
|
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added new section on GitHub Actions in
CONTRIBUTING.md - Updated
docs/bridge/CHANGELOG.mdto version 0.1.5 - Added
codecovWebpackPluginand fixedbaseUrlindocs/bridge/docusaurus.config.ts - Updated
docs/bridge/package.jsonto version 0.1.5, added@codecov/webpack-plugin - Version bump to 0.2.10 in
packages/contracts-rfq/CHANGELOG.mdandpackage.json
27 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
rfqRefundcommand tocontrib/opbot/botmd/botmd.go - Modified
NewBotfunction to return a pointer incontrib/opbot/botmd/botmd.go - Introduced new fields (
rpcClient,signer,submitter) inBotstruct incontrib/opbot/botmd/botmd.go - Added
GetQuoteRequestByTxIDfunction inservices/rfq/relayer/relapi/handler.go - Minor comment correction in
services/rfq/api/rest/server.go
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
| b.signer, err = signerConfig.SignerFromConfig(ctx, b.cfg.Signer) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create signer: %w", err) |
There was a problem hiding this comment.
🧠 logic: Error handling for signer creation should be reviewed to ensure it covers all edge cases.
| dbType, err := dbcommon.DBTypeFromString(b.cfg.Database.Type) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get db type: %w", err) |
There was a problem hiding this comment.
🧠 logic: Error handling for database type conversion should be reviewed to ensure it covers all edge cases.
| store, err := cctpSql.Connect(ctx, dbType, b.cfg.Database.DSN, b.handler) | ||
| if err != nil { | ||
| return fmt.Errorf("could not connect to database: %w", err) |
There was a problem hiding this comment.
🧠 logic: Error handling for database connection should be reviewed to ensure it covers all edge cases.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- contrib/opbot/botmd/botmd.go (4 hunks)
- services/rfq/api/rest/server.go (3 hunks)
- services/rfq/relayer/relapi/handler.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- services/rfq/api/rest/server.go
- services/rfq/relayer/relapi/handler.go
Additional context used
GitHub Check: codecov/patch
contrib/opbot/botmd/botmd.go
[warning] 32-32: contrib/opbot/botmd/botmd.go#L32
Added line #L32 was not covered by tests
[warning] 46-47: contrib/opbot/botmd/botmd.go#L46-L47
Added lines #L46 - L47 were not covered by tests
[warning] 49-49: contrib/opbot/botmd/botmd.go#L49
Added line #L49 was not covered by tests
[warning] 51-51: contrib/opbot/botmd/botmd.go#L51
Added line #L51 was not covered by tests
[warning] 69-73: contrib/opbot/botmd/botmd.go#L69-L73
Added lines #L69 - L73 were not covered by tests
[warning] 75-78: contrib/opbot/botmd/botmd.go#L75-L78
Added lines #L75 - L78 were not covered by tests
[warning] 80-83: contrib/opbot/botmd/botmd.go#L80-L83
Added lines #L80 - L83 were not covered by tests
[warning] 85-90: contrib/opbot/botmd/botmd.go#L85-L90
Added lines #L85 - L90 were not covered by tests
[warning] 92-94: contrib/opbot/botmd/botmd.go#L92-L94
Added lines #L92 - L94 were not covered by tests
[warning] 97-97: contrib/opbot/botmd/botmd.go#L97
Added line #L97 was not covered by tests
Additional comments not posted (9)
contrib/opbot/botmd/botmd.go (9)
5-16: Imports look good.The new imports for database, signer, submitter, and RPC client functionalities are correctly added and used in the code.
26-28: Struct changes look good.The
Botstruct has been correctly enhanced with the new fields:rpcClient,signer, andsubmitter.
46-47: Initialization ofrpcClientlooks good.The
rpcClientis correctly initialized in theNewBotfunction.Tools
GitHub Check: codecov/patch
[warning] 46-47: contrib/opbot/botmd/botmd.go#L46-L47
Added lines #L46 - L47 were not covered by tests
49-49: Addition ofrfqRefundcommand looks good.The
rfqRefundcommand is correctly added in theNewBotfunction.Tools
GitHub Check: codecov/patch
[warning] 49-49: contrib/opbot/botmd/botmd.go#L49
Added line #L49 was not covered by tests
69-73: Initialization ofsignerlooks good.The
signeris correctly initialized in theStartmethod with appropriate error handling.Tools
GitHub Check: codecov/patch
[warning] 69-73: contrib/opbot/botmd/botmd.go#L69-L73
Added lines #L69 - L73 were not covered by tests
75-78: Database type conversion looks good.The database type conversion is correctly implemented with appropriate error handling.
Tools
GitHub Check: codecov/patch
[warning] 75-78: contrib/opbot/botmd/botmd.go#L75-L78
Added lines #L75 - L78 were not covered by tests
80-83: Database connection looks good.The connection to the database is correctly implemented with appropriate error handling.
Tools
GitHub Check: codecov/patch
[warning] 80-83: contrib/opbot/botmd/botmd.go#L80-L83
Added lines #L80 - L83 were not covered by tests
85-90: Initialization ofsubmitterlooks good.The
submitteris correctly initialized and started in theStartmethod.Tools
GitHub Check: codecov/patch
[warning] 85-90: contrib/opbot/botmd/botmd.go#L85-L90
Added lines #L85 - L90 were not covered by tests
92-94: Server start looks good.The server is correctly started in the
Startmethod.Tools
GitHub Check: codecov/patch
[warning] 92-94: contrib/opbot/botmd/botmd.go#L92-L94
Added lines #L92 - L94 were not covered by tests
| } | ||
|
|
||
| // NewBot creates a new bot server. | ||
| func NewBot(handler metrics.Handler, cfg config.Config) Bot { | ||
| func NewBot(handler metrics.Handler, cfg config.Config) *Bot { |
There was a problem hiding this comment.
Add tests for the new functionality.
The following functionalities lack test coverage:
rpcClientinitializationrfqRefundcommandsignerinitialization- Database connection
submitterinitialization- Server start
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Also applies to: 46-47, 49-49, 51-51, 69-73, 75-78, 80-83, 85-90, 92-94, 97-97
Tools
GitHub Check: codecov/patch
[warning] 32-32: contrib/opbot/botmd/botmd.go#L32
Added line #L32 was not covered by tests
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced
rfqRefundcommand incontrib/opbot/botmd/commands.gofor handling refund operations. - Updated submodule reference in
services/cctp-relayer/external/evm-cctp-contracts. - Improved URL formatting in
GetEndpointmethod inservices/omnirpc/client/client.go.
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Modified
contrib/opbot/botmd/commands.goto improverfqRefundandgetQuoteRequestfunctions for better data conversion and response handling. - Enhanced
services/rfq/relayer/relapi/handler.goby addingOriginChainID,DestChainID,OriginToken, andDestTokenfields toGetQuoteRequestByTxIDresponse. - Updated
services/rfq/relayer/relapi/model.goto include new fields inGetQuoteRequestResponsestruct for detailed quote request information.
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (11)
agents/go.sumis excluded by!**/*.sumcontrib/promexporter/go.sumis excluded by!**/*.sumcontrib/screener-api/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sumethergo/go.sumis excluded by!**/*.sumservices/cctp-relayer/go.sumis excluded by!**/*.sumservices/explorer/go.sumis excluded by!**/*.sumservices/omnirpc/go.sumis excluded by!**/*.sumservices/rfq/go.sumis excluded by!**/*.sumservices/scribe/go.sumis excluded by!**/*.sumtools/go.sumis excluded by!**/*.sum
Files selected for processing (5)
- contrib/opbot/botmd/commands.go (3 hunks)
- services/cctp-relayer/external/evm-cctp-contracts (1 hunks)
- services/omnirpc/client/client.go (2 hunks)
- services/rfq/relayer/relapi/handler.go (1 hunks)
- services/rfq/relayer/relapi/model.go (1 hunks)
Files skipped from review due to trivial changes (1)
- services/cctp-relayer/external/evm-cctp-contracts
Files skipped from review as they are similar to previous changes (3)
- services/omnirpc/client/client.go
- services/rfq/relayer/relapi/handler.go
- services/rfq/relayer/relapi/model.go
Additional context used
GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 249-259: contrib/opbot/botmd/commands.go#L249-L259
Added lines #L249 - L259 were not covered by tests
[warning] 261-268: contrib/opbot/botmd/commands.go#L261-L268
Added lines #L261 - L268 were not covered by tests
[warning] 271-278: contrib/opbot/botmd/commands.go#L271-L278
Added lines #L271 - L278 were not covered by tests
[warning] 281-287: contrib/opbot/botmd/commands.go#L281-L287
Added lines #L281 - L287 were not covered by tests
[warning] 290-296: contrib/opbot/botmd/commands.go#L290-L296
Added lines #L290 - L296 were not covered by tests
[warning] 299-303: contrib/opbot/botmd/commands.go#L299-L303
Added lines #L299 - L303 were not covered by tests
[warning] 305-311: contrib/opbot/botmd/commands.go#L305-L311
Added lines #L305 - L311 were not covered by tests
[warning] 314-318: contrib/opbot/botmd/commands.go#L314-L318
Added lines #L314 - L318 were not covered by tests
[warning] 320-329: contrib/opbot/botmd/commands.go#L320-L329
Added lines #L320 - L329 were not covered by tests
[warning] 332-337: contrib/opbot/botmd/commands.go#L332-L337
Added lines #L332 - L337 were not covered by tests
[warning] 341-345: contrib/opbot/botmd/commands.go#L341-L345
Added lines #L341 - L345 were not covered by tests
[warning] 379-384: contrib/opbot/botmd/commands.go#L379-L384
Added lines #L379 - L384 were not covered by tests
[warning] 387-393: contrib/opbot/botmd/commands.go#L387-L393
Added lines #L387 - L393 were not covered by tests
[warning] 396-399: contrib/opbot/botmd/commands.go#L396-L399
Added lines #L396 - L399 were not covered by tests
[warning] 401-401: contrib/opbot/botmd/commands.go#L401
Added line #L401 was not covered by tests
GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 249-249:
cognitive complexity 46 of func(*Bot).rfqRefundis high (> 30) (gocognit)
[failure] 333-333:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
[failure] 378-378:
Comment should end in a period (godot)
contrib/opbot/botmd/commands.go
Outdated
| func (b *Bot) rfqRefund() *slacker.CommandDefinition { | ||
| return &slacker.CommandDefinition{ | ||
| Command: "refund <tx> <origin_chainid>", | ||
| Description: "refund a quote request", | ||
| Examples: []string{"refund 0x1234"}, | ||
| Handler: func(ctx *slacker.CommandContext) { | ||
| client, err := rfqClient.NewUnauthenticatedClient(b.handler, b.cfg.RFQApiURL) | ||
| if err != nil { | ||
| log.Printf("error creating rfq client: %v\n", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
Reduce the cognitive complexity of the rfqRefund function.
The function has a high cognitive complexity (46). Consider refactoring to break down the function into smaller, more manageable functions.
func (b *Bot) rfqRefund() *slacker.CommandDefinition {
return &slacker.CommandDefinition{
Command: "refund <tx> <origin_chainid>",
Description: "refund a quote request",
Examples: []string{"refund 0x1234"},
Handler: func(ctx *slacker.CommandContext) {
if err := b.handleRFQRefund(ctx); err != nil {
log.Println(err)
}
},
}
}
func (b *Bot) handleRFQRefund(ctx *slacker.CommandContext) error {
client, err := rfqClient.NewUnauthenticatedClient(b.handler, b.cfg.RFQApiURL)
if err != nil {
log.Printf("error creating rfq client: %v\n", err)
return err
}
contracts, err := client.GetRFQContracts(ctx.Context())
if err != nil {
log.Printf("error fetching rfq contracts: %v\n", err)
_, err := ctx.Response().Reply("error fetching rfq contracts")
if err != nil {
return err
}
return err
}
tx := stripLinks(ctx.Request().Param("tx"))
if len(tx) == 0 {
_, err := ctx.Response().Reply("please provide a tx hash")
if err != nil {
log.Println(err)
}
return nil
}
originChainID, err := b.getOriginChainID(ctx)
if err != nil {
return err
}
chainClient, err := b.rpcClient.GetChainClient(ctx.Context(), originChainID)
if err != nil {
log.Printf("error getting chain client: %v\n", err)
return err
}
contractAddress, ok := contracts.Contracts[uint32(originChainID)]
if !ok {
_, err := ctx.Response().Reply("contract address not found")
if err != nil {
log.Println(err)
}
return nil
}
fastBridgeHandle, err := fastbridge.NewFastBridge(common.HexToAddress(contractAddress), chainClient)
if err != nil {
log.Printf("error creating fast bridge: %v\n", err)
return err
}
return b.submitRefund(ctx, fastBridgeHandle, tx, originChainID)
}
func (b *Bot) getOriginChainID(ctx *slacker.CommandContext) (int, error) {
originChainIDStr := ctx.Request().Param("origin_chainid")
if len(originChainIDStr) == 0 {
_, err := ctx.Response().Reply("please provide an origin chain id")
if err != nil {
log.Println(err)
}
return 0, nil
}
originChainID, err := strconv.Atoi(convertChainName(originChainIDStr))
if err != nil {
_, err := ctx.Response().Reply("origin_chainid must be a number")
if err != nil {
log.Println(err)
}
return 0, err
}
return originChainID, nil
}
func (b *Bot) submitRefund(ctx *slacker.CommandContext, fastBridgeHandle *fastbridge.FastBridgeTransactor, tx string, originChainID int) error {
for _, relayer := range b.cfg.RelayerURLS {
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 err
}
nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(originChainID)), func(transactor *bind.TransactOpts) (*types.Transaction, error) {
return fastBridgeHandle.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw))
})
if err != nil {
log.Printf("error submitting refund: %v\n", err)
continue
}
if _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)); err != nil {
log.Println(err)
}
return nil
}
return nil
}Tools
GitHub Check: codecov/patch
[warning] 249-259: contrib/opbot/botmd/commands.go#L249-L259
Added lines #L249 - L259 were not covered by tests
GitHub Check: Lint (contrib/opbot)
[failure] 249-249:
cognitive complexity 46 of func(*Bot).rfqRefundis high (> 30) (gocognit)
| qr, err := client.GetQuoteRequestByTXID(ctx, tx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error fetching quote request: %w", err) | ||
| } |
There was a problem hiding this comment.
Ensure error handling for GetQuoteRequestByTXID.
The error handling for GetQuoteRequestByTXID should be improved to provide more context.
- qr, err := client.GetQuoteRequestByTXID(ctx, tx)
- if err != nil {
- return nil, fmt.Errorf("error fetching quote request: %w", err)
+ qr, err := client.GetQuoteRequestByTXID(ctx, tx)
+ if err != nil {
+ return nil, fmt.Errorf("error fetching 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.
| qr, err := client.GetQuoteRequestByTXID(ctx, tx) | |
| if err != nil { | |
| return nil, fmt.Errorf("error fetching quote request: %w", err) | |
| } | |
| qr, err := client.GetQuoteRequestByTXID(ctx, tx) | |
| if err != nil { | |
| return nil, fmt.Errorf("error fetching quote request by tx id: %w", err) | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 396-399: contrib/opbot/botmd/commands.go#L396-L399
Added lines #L396 - L399 were not covered by tests
contrib/opbot/botmd/commands.go
Outdated
| } | ||
|
|
||
| nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(originChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| return fastBridgeHandle.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) |
There was a problem hiding this comment.
Wrap the error returned from the external package.
The error returned from fastBridgeHandle.Refund should be wrapped to provide more context.
- return fastBridgeHandle.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw))
+ tx, err := fastBridgeHandle.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw))
+ if err != nil {
+ return nil, fmt.Errorf("failed to refund: %w", err)
+ }
+ return tx, nilCommittable 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 fastBridgeHandle.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) | |
| tx, err := fastBridgeHandle.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to refund: %w", err) | |
| } | |
| return tx, nil |
Tools
GitHub Check: Lint (contrib/opbot)
[failure] 333-333:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
| } | ||
|
|
||
| // TODO: follow the lead of https://github.com/synapsecns/sanguine/pull/2845 | ||
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) |
There was a problem hiding this comment.
Fix ineffectual assignment to err.
The variable err is assigned but not used.
- _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce))
+ if _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)); err != nil {
+ log.Println(err)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err = ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)) | |
| if _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)); err != nil { | |
| log.Println(err) | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 341-345: contrib/opbot/botmd/commands.go#L341-L345
Added lines #L341 - L345 were not covered by tests
| func getQuoteRequest(ctx context.Context, client relapi.RelayerClient, tx string) (*relapi.GetQuoteRequestResponse, error) { | ||
| // at this point tx can be a txid or a has, we try both | ||
| txRequest, err := client.GetQuoteRequestStatusByTxHash(ctx, tx) | ||
| if err == nil { | ||
| // override tx with txid | ||
| tx = txRequest.TxID | ||
| } |
There was a problem hiding this comment.
Ensure error handling for GetQuoteRequestStatusByTxHash.
The error handling for GetQuoteRequestStatusByTxHash should be improved to provide more context.
- txRequest, err := client.GetQuoteRequestStatusByTxHash(ctx, tx)
- if err == nil {
+ txRequest, err := client.GetQuoteRequestStatusByTxHash(ctx, tx)
+ if err != nil {
+ return nil, fmt.Errorf("error fetching quote request status by tx hash: %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.
| func getQuoteRequest(ctx context.Context, client relapi.RelayerClient, tx string) (*relapi.GetQuoteRequestResponse, error) { | |
| // at this point tx can be a txid or a has, we try both | |
| txRequest, err := client.GetQuoteRequestStatusByTxHash(ctx, tx) | |
| if err == nil { | |
| // override tx with txid | |
| tx = txRequest.TxID | |
| } | |
| func getQuoteRequest(ctx context.Context, client relapi.RelayerClient, tx string) (*relapi.GetQuoteRequestResponse, error) { | |
| // at this point tx can be a txid or a has, we try both | |
| txRequest, err := client.GetQuoteRequestStatusByTxHash(ctx, tx) | |
| if err != nil { | |
| return nil, fmt.Errorf("error fetching quote request status by tx hash: %w", err) | |
| } | |
| // override tx with txid | |
| tx = txRequest.TxID |
Tools
GitHub Check: codecov/patch
[warning] 387-393: contrib/opbot/botmd/commands.go#L387-L393
Added lines #L387 - L393 were not covered by tests
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
PutBulkQuotesmethod in/services/rfq/api/client/client.gofor bulk quote requests. - Introduced
TestPutAndGetBulkQuotesin/services/rfq/api/client/client_test.goto test bulk quote functionality. - Added
UpsertQuotesmethod in/services/rfq/api/db/api_db.goand/services/rfq/api/db/sql/base/store.gofor batch database operations. - Updated
/services/rfq/api/docs/swagger.jsonand/services/rfq/api/docs/swagger.yamlwith new/bulk_quotesendpoint. - Enhanced
/services/rfq/api/rest/handler.gowithModifyBulkQuotesfunction for handling multiple quotes.
14 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced
rfqRefundcommand for handling refund operations related to quote requests (contrib/opbot/botmd/commands.go) - Added critical check to ensure origin chain ID matches before proceeding with refund transaction (
contrib/opbot/botmd/commands.go)
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)
- Added
rfqRefundcommand for handling refund operations (contrib/opbot/botmd/commands.go) - Introduced
makeFastBridgefunction for creating FastBridge contract instances (contrib/opbot/botmd/commands.go) - Refactored refund logic for improved error handling and modularization (
contrib/opbot/botmd/commands.go) - Added
nolintdirective to suppress security warning foros.MkdirAll(contrib/opbot/sql/sqlite/sqlite.go)
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (1)
contrib/opbot/botmd/commands.go (1)
6-21: Remove unused imports.The
errorsandregexpimports are not used in the provided code.- "errors" "fmt" - "regexp"
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
Files selected for processing (8)
- contrib/opbot/botmd/commands.go (3 hunks)
- contrib/opbot/sql/sqlite/sqlite.go (1 hunks)
- services/rfq/api/client/client.go (5 hunks)
- services/rfq/api/docs/docs.go (2 hunks)
- services/rfq/api/docs/swagger.json (2 hunks)
- services/rfq/api/docs/swagger.yaml (2 hunks)
- services/rfq/api/rest/handler.go (3 hunks)
- services/rfq/api/rest/server.go (3 hunks)
Files skipped from review as they are similar to previous changes (5)
- contrib/opbot/sql/sqlite/sqlite.go
- services/rfq/api/client/client.go
- services/rfq/api/docs/docs.go
- services/rfq/api/rest/handler.go
- services/rfq/api/rest/server.go
Additional context used
checkov
services/rfq/api/docs/swagger.yaml
[HIGH] 1-201: Ensure that security requirement defined in securityDefinitions - version 2.0 files
(CKV_OPENAPI_6)
[HIGH] 1-201: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[HIGH] 1-201: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[HIGH] 1-201: Ensure that securityDefinitions is defined and not empty - version 2.0 files
(CKV_OPENAPI_1)
GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 250-263: contrib/opbot/botmd/commands.go#L250-L263
Added lines #L250 - L263 were not covered by tests
[warning] 266-272: contrib/opbot/botmd/commands.go#L266-L272
Added lines #L266 - L272 were not covered by tests
[warning] 275-284: contrib/opbot/botmd/commands.go#L275-L284
Added lines #L275 - L284 were not covered by tests
[warning] 287-293: contrib/opbot/botmd/commands.go#L287-L293
Added lines #L287 - L293 were not covered by tests
[warning] 296-302: contrib/opbot/botmd/commands.go#L296-L302
Added lines #L296 - L302 were not covered by tests
[warning] 306-310: contrib/opbot/botmd/commands.go#L306-L310
Added lines #L306 - L310 were not covered by tests
[warning] 316-320: contrib/opbot/botmd/commands.go#L316-L320
Added lines #L316 - L320 were not covered by tests
[warning] 322-325: contrib/opbot/botmd/commands.go#L322-L325
Added lines #L322 - L325 were not covered by tests
[warning] 327-330: contrib/opbot/botmd/commands.go#L327-L330
Added lines #L327 - L330 were not covered by tests
[warning] 332-335: contrib/opbot/botmd/commands.go#L332-L335
Added lines #L332 - L335 were not covered by tests
[warning] 337-341: contrib/opbot/botmd/commands.go#L337-L341
Added lines #L337 - L341 were not covered by tests
[warning] 370-376: contrib/opbot/botmd/commands.go#L370-L376
Added lines #L370 - L376 were not covered by tests
[warning] 379-382: contrib/opbot/botmd/commands.go#L379-L382
Added lines #L379 - L382 were not covered by tests
[warning] 384-384: contrib/opbot/botmd/commands.go#L384
Added line #L384 was not covered by tests
GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 298-298:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
| model.GetContractsResponse: | ||
| properties: | ||
| contracts: | ||
| additionalProperties: | ||
| type: string | ||
| description: Contracts is a map of chain id to contract address | ||
| type: object | ||
| type: object |
There was a problem hiding this comment.
Correct the schema definition for GetContractsResponse.
The contracts property should be an array of strings, not an object with additional properties. The description should also be updated accordingly.
model.GetContractsResponse:
properties:
contracts:
description: Contracts is a list of contract addresses
type: array
items:
type: string
type: object| /contracts: | ||
| get: | ||
| consumes: | ||
| - application/json | ||
| description: get quotes from all relayers. | ||
| produces: | ||
| - application/json | ||
| responses: | ||
| "200": | ||
| description: OK | ||
| schema: | ||
| items: | ||
| $ref: '#/definitions/model.GetContractsResponse' | ||
| type: array | ||
| summary: Get contract addresses | ||
| tags: | ||
| - quotes |
There was a problem hiding this comment.
Correct the endpoint description and schema for /contracts.
The description and schema should accurately reflect the response structure.
/contracts:
get:
consumes:
- application/json
description: Get contract addresses from all relayers.
produces:
- application/json
responses:
"200":
description: OK
schema:
$ref: '#/definitions/model.GetContractsResponse'
summary: Get contract addresses
tags:
- contracts| "/contracts": { | ||
| "get": { | ||
| "description": "get quotes from all relayers.", | ||
| "consumes": [ | ||
| "application/json" | ||
| ], | ||
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "tags": [ | ||
| "quotes" | ||
| ], | ||
| "summary": "Get contract addresses", | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", | ||
| "schema": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/definitions/model.GetContractsResponse" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct the endpoint description and schema for /contracts.
The description and schema should accurately reflect the response structure.
"/contracts": {
"get": {
"description": "Get contract addresses from all relayers.",
"consumes": [
"application/json"
],
"produces": [
"application/json"
],
"tags": [
"contracts"
],
"summary": "Get contract addresses",
"responses": {
"200": {
"description": "OK",
"schema": {
"$ref": "#/definitions/model.GetContractsResponse"
}
}
}
}
}| "model.GetContractsResponse": { | ||
| "type": "object", | ||
| "properties": { | ||
| "contracts": { | ||
| "description": "Contracts is a map of chain id to contract address", | ||
| "type": "object", | ||
| "additionalProperties": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Correct the schema definition for GetContractsResponse.
The contracts property should be an array of strings, not an object with additional properties. The description should also be updated accordingly.
"model.GetContractsResponse": {
"type": "object",
"properties": {
"contracts": {
"description": "Contracts is a list of contract addresses",
"type": "array",
"items": {
"type": "string"
}
}
}
}
contrib/opbot/botmd/commands.go
Outdated
| // nolint: gocognit, cyclop. | ||
| func (b *Bot) rfqRefund() *slacker.CommandDefinition { | ||
| return &slacker.CommandDefinition{ | ||
| Command: "refund <tx> <origin_chainid>", | ||
| Description: "refund a quote request", | ||
| Examples: []string{"refund 0x1234"}, | ||
| Handler: func(ctx *slacker.CommandContext) { | ||
| tx := stripLinks(ctx.Request().Param("tx")) | ||
|
|
||
| if len(tx) == 0 { | ||
| _, err := ctx.Response().Reply("please provide a tx hash") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| originChainIDStr := ctx.Request().Param("origin_chainid") | ||
| if len(originChainIDStr) == 0 { | ||
| _, err := ctx.Response().Reply("please provide an origin chain id") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| for _, relayer := range b.cfg.RelayerURLS { | ||
| 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) { | ||
| //nolint: wrapcheck. | ||
| return fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw)) | ||
| }) | ||
| if err != nil { | ||
| log.Printf("error submitting refund: %v\n", err) | ||
| 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)) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
Refactor rfqRefund function to reduce cognitive complexity.
The function has a high cognitive complexity. Consider breaking it down into smaller, more manageable functions.
func (b *Bot) rfqRefund() *slacker.CommandDefinition {
return &slacker.CommandDefinition{
Command: "refund <tx> <origin_chainid>",
Description: "refund a quote request",
Examples: []string{"refund 0x1234"},
Handler: func(ctx *slacker.CommandContext) {
if err := b.handleRFQRefund(ctx); err != nil {
log.Println(err)
}
},
}
}
func (b *Bot) handleRFQRefund(ctx *slacker.CommandContext) error {
tx := stripLinks(ctx.Request().Param("tx"))
if len(tx) == 0 {
return b.respondWithError(ctx, "please provide a tx hash", nil)
}
originChainIDStr := ctx.Request().Param("origin_chainid")
if len(originChainIDStr) == 0 {
return b.respondWithError(ctx, "please provide an origin chain id", nil)
}
for _, relayer := range b.cfg.RelayerURLS {
relClient := relapi.NewRelayerClient(b.handler, relayer)
rawRequest, err := getQuoteRequest(ctx.Context(), relClient, tx)
if err != nil {
return b.respondWithError(ctx, "error fetching quote request", err)
}
fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest)
if err != nil {
return b.respondWithError(ctx, err.Error(), err)
}
nonce, err := b.submitter.SubmitTransaction(ctx.Context(), big.NewInt(int64(rawRequest.OriginChainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) {
//nolint: wrapcheck.
return fastBridgeContract.Refund(transactor, common.Hex2Bytes(rawRequest.QuoteRequestRaw))
})
if err != nil {
log.Printf("error submitting refund: %v\n", err)
continue
}
if _, err := ctx.Response().Reply(fmt.Sprintf("refund submitted with nonce %d", nonce)); err != nil {
log.Println(err)
}
return nil
}
return nil
}
func (b *Bot) respondWithError(ctx *slacker.CommandContext, message string, err error) error {
if err != nil {
log.Printf("%s: %v\n", message, err)
}
_, respErr := ctx.Response().Reply(message)
if respErr != nil {
log.Println(respErr)
}
return err
}Tools
GitHub Check: codecov/patch
[warning] 250-263: contrib/opbot/botmd/commands.go#L250-L263
Added lines #L250 - L263 were not covered by tests
[warning] 266-272: contrib/opbot/botmd/commands.go#L266-L272
Added lines #L266 - L272 were not covered by tests
[warning] 275-284: contrib/opbot/botmd/commands.go#L275-L284
Added lines #L275 - L284 were not covered by tests
[warning] 287-293: contrib/opbot/botmd/commands.go#L287-L293
Added lines #L287 - L293 were not covered by tests
[warning] 296-302: contrib/opbot/botmd/commands.go#L296-L302
Added lines #L296 - L302 were not covered by tests
[warning] 306-310: contrib/opbot/botmd/commands.go#L306-L310
Added lines #L306 - L310 were not covered by tests
GitHub Check: Lint (contrib/opbot)
[failure] 298-298:
error returned from external package is unwrapped: sig: func (*github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge.FastBridgeTransactor).Refund(opts *github.com/ethereum/go-ethereum/accounts/abi/bind.TransactOpts, request []byte) (*github.com/ethereum/go-ethereum/core/types.Transaction, error) (wrapcheck)
| func (b *Bot) makeFastBridge(ctx context.Context, req *relapi.GetQuoteRequestResponse) (*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.OriginChainID)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting chain client: %w", err) | ||
| } | ||
|
|
||
| contractAddress, ok := contracts.Contracts[req.OriginChainID] | ||
| if !ok { | ||
| return nil, errors.New("contract address not found") | ||
| } | ||
|
|
||
| fastBridgeHandle, err := fastbridge.NewFastBridge(common.HexToAddress(contractAddress), chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating fast bridge: %w", err) | ||
| } | ||
| return fastBridgeHandle, nil | ||
| } |
There was a problem hiding this comment.
Wrap the error returned from the external package.
The error returned from fastbridge.NewFastBridge should be wrapped to provide more context.
- return nil, fmt.Errorf("error creating fast bridge: %w", err)
+ return nil, fmt.Errorf("error creating fast bridge: %w", err)Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: codecov/patch
[warning] 316-320: contrib/opbot/botmd/commands.go#L316-L320
Added lines #L316 - L320 were not covered by tests
[warning] 322-325: contrib/opbot/botmd/commands.go#L322-L325
Added lines #L322 - L325 were not covered by tests
[warning] 327-330: contrib/opbot/botmd/commands.go#L327-L330
Added lines #L327 - L330 were not covered by tests
[warning] 332-335: contrib/opbot/botmd/commands.go#L332-L335
Added lines #L332 - L335 were not covered by tests
[warning] 337-341: contrib/opbot/botmd/commands.go#L337-L341
Added lines #L337 - L341 were not covered by tests
| func getQuoteRequest(ctx context.Context, client relapi.RelayerClient, tx string) (*relapi.GetQuoteRequestResponse, error) { | ||
| // at this point tx can be a txid or a has, we try both | ||
| txRequest, err := client.GetQuoteRequestStatusByTxHash(ctx, tx) | ||
| if err == nil { | ||
| // override tx with txid | ||
| tx = txRequest.TxID | ||
| } | ||
|
|
||
| // look up quote request | ||
| qr, err := client.GetQuoteRequestByTXID(ctx, tx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error fetching quote request: %w", err) | ||
| } | ||
|
|
||
| return qr, nil |
There was a problem hiding this comment.
Ensure error handling for GetQuoteRequestByTXID.
The error handling for GetQuoteRequestByTXID should be improved to provide more context.
- qr, err := client.GetQuoteRequestByTXID(ctx, tx)
- if err != nil {
- return nil, fmt.Errorf("error fetching quote request: %w", err)
+ qr, err := client.GetQuoteRequestByTXID(ctx, tx)
+ if err != nil {
+ return nil, fmt.Errorf("error fetching 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.
| func getQuoteRequest(ctx context.Context, client relapi.RelayerClient, tx string) (*relapi.GetQuoteRequestResponse, error) { | |
| // at this point tx can be a txid or a has, we try both | |
| txRequest, err := client.GetQuoteRequestStatusByTxHash(ctx, tx) | |
| if err == nil { | |
| // override tx with txid | |
| tx = txRequest.TxID | |
| } | |
| // look up quote request | |
| qr, err := client.GetQuoteRequestByTXID(ctx, tx) | |
| if err != nil { | |
| return nil, fmt.Errorf("error fetching quote request: %w", err) | |
| } | |
| return qr, nil | |
| func getQuoteRequest(ctx context.Context, client relapi.RelayerClient, tx string) (*relapi.GetQuoteRequestResponse, error) { | |
| // at this point tx can be a txid or a has, we try both | |
| txRequest, err := client.GetQuoteRequestStatusByTxHash(ctx, tx) | |
| if err == nil { | |
| // override tx with txid | |
| tx = txRequest.TxID | |
| } | |
| // look up quote request | |
| qr, err := client.GetQuoteRequestByTXID(ctx, tx) | |
| if err != nil { | |
| return nil, fmt.Errorf("error fetching quote request by tx id: %w", err) | |
| } | |
| return qr, nil | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 370-376: contrib/opbot/botmd/commands.go#L370-L376
Added lines #L370 - L376 were not covered by tests
[warning] 379-382: contrib/opbot/botmd/commands.go#L379-L382
Added lines #L379 - L382 were not covered by tests
[warning] 384-384: contrib/opbot/botmd/commands.go#L384
Added line #L384 was not covered by tests
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced
rfqRefundcommand for handling refund operations (contrib/opbot/botmd/commands.go) - Added interaction with RFQ API and FastBridge contract for refunds (
contrib/opbot/botmd/commands.go) - Added
nolint: wrapcheckdirective to suppress linting warnings (contrib/opbot/botmd/commands.go)
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)
- Introduced
rfqRefundcommand for handling refund operations (contrib/opbot/botmd/commands.go) - Improved error handling in
rfqRefundcommand (contrib/opbot/botmd/commands.go) - Enhanced context propagation in
rfqRefundcommand (contrib/opbot/botmd/commands.go)
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Description
adds refunder to opbot
b38de9c: docs preview link
2eaf09e: docs preview link
Summary by CodeRabbit
New Features
rfqRefundcommand to handle refund operations related to quote requests./contractsendpoint in the RFQ API.ChainNameToChainIDfunction to map chain names to their corresponding chain IDs.Improvements
Bug Fixes
FromHexfunction for private keys.bccc594: docs preview link
6ba2d65: docs preview link
0220b7a: docs preview link
d84b34b: docs preview link
b74114a: docs preview link
8baa679: docs preview link
34b1264: docs preview link
db98d42: docs preview link
aa09721: docs preview link
3b2e64f: docs preview link
4d675b8: docs preview link
9796c58: docs preview link