Add RFQ Guard #2840
Add RFQ Guard #2840
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 7 minutes and 50 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes introduce a new guard functionality to the RFQ service for monitoring and ensuring the integrity of Changes
Sequence Diagram(s)sequenceDiagram
participant Guard as Guard Service
participant DB as Database
participant Contract as FastBridge Contract
participant Client as Ethereum Client
Note over Guard, DB: Initialization
Guard->>DB: Initialize and connect
Guard->>Contract: Monitor `prove()` events
Note over Guard, Contract: Monitoring
Contract-->>Guard: Emit `prove()` event with details
Guard->>DB: Store `prove()` event
Note over Guard, Client: Verification
Guard->>Client: Verify corresponding event on destination chain
Client-->>Guard: Event exists / doesn't exist
Note over Guard, Contract: Dispute
Guard->>Contract: Call `dispute()` for invalid `prove()`
Contract-->>Guard: Confirm dispute
Guard->>DB: Update status to `Disputed`
Possibly related issues
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 (
|
![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
StoreBridgeRequestandGetBridgeRequestByIDfunctions inservices/rfq/guard/guarddb/base/bridge.go - Introduced
PendingProvenModelandBridgeRequestModelinservices/rfq/guard/guarddb/base/model.go - Implemented CRUD operations for 'PendingProven' records in
services/rfq/guard/guarddb/base/proven.go - Created
Storestruct andNewStorefunction inservices/rfq/guard/guarddb/base/store.go - Established database connections in
services/rfq/guard/guarddb/connect/sql.go
12 file(s) reviewed, 19 comment(s)
Edit PR Review Bot Settings
| } | ||
|
|
||
| if tx.Error != nil { | ||
| return nil, fmt.Errorf("could not get request") |
There was a problem hiding this comment.
🪶 style: Include the original error in the returned error message for better debugging.
|
|
||
| qr, err := modelResult.ToBridgeRequest() | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🪶 style: Wrap the error with additional context for better error tracing.
| @@ -0,0 +1,2 @@ | |||
| // Package base contains the base implementation for different sql driers. | |||
There was a problem hiding this comment.
📚 spelling: Typo in 'driers'. It should be 'drivers'.
| txID, err := hexutil.Decode(p.TransactionID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get transaction id: %w", err) |
There was a problem hiding this comment.
🧠 logic: Error handling for hexutil.Decode should be more descriptive.
| transactionID, err := sliceToArray(txID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not convert transaction id: %w", err) |
There was a problem hiding this comment.
🧠 logic: Consider adding more context to the error message for sliceToArray.
|
|
||
| var logger = log.Logger("mysql-logger") | ||
|
|
||
| // Store is the sqlite store. It extends the base store for sqlite specific queries. |
There was a problem hiding this comment.
📚 spelling: The comment should refer to MySQL instead of SQLite.
| // MaxIdleConns is exported here for testing. Tests execute too slowly with a reconnect each time. | ||
| var MaxIdleConns = 0 | ||
|
|
||
| // NamingStrategy is used to exported here for testing. |
There was a problem hiding this comment.
📚 spelling: Typo: 'is used to exported here' should be 'is exported here'.
| // create the directory to the store if it doesn't exist | ||
| 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 including the original error message for better debugging.
| return nil, fmt.Errorf("could not create sqlite store") | |
| return nil, fmt.Errorf("could not create sqlite store: %w", err) |
| 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: Include the original error message for better debugging.
| return nil, fmt.Errorf("could not connect to db %s: %w", dbPath, err) | |
| return nil, fmt.Errorf("could not connect to db %s: %w", dbPath, err) |
|
|
||
| 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: Include the original error message for better debugging.
| return nil, fmt.Errorf("could not migrate models: %w", err) | |
| return nil, fmt.Errorf("could not migrate models: %w", err) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2840 +/- ##
===================================================
- Coverage 25.80877% 17.44358% -8.36519%
===================================================
Files 713 534 -179
Lines 52827 43248 -9579
Branches 80 0 -80
===================================================
- Hits 13634 7544 -6090
+ Misses 37792 34663 -3129
+ Partials 1401 1041 -360
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
63e553c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cedd6e53.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-rfq-guard.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (3)
services/rfq/guard/guarddb/base/model.go (2)
42-56: Consider adding JSON tags to the struct fields.Adding JSON tags to the struct fields can improve serialization and deserialization when working with JSON data.
type PendingProvenModel struct { CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"` Origin uint32 `json:"origin"` TransactionID string `json:"transaction_id"` TxHash string `json:"tx_hash"` Status guarddb.PendingProvenStatus `json:"status"` }
110-144: Consider adding JSON tags to the struct fields.Adding JSON tags to the struct fields can improve serialization and deserialization when working with JSON data.
type BridgeRequestModel struct { CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"` TransactionID string `json:"transaction_id"` OriginChainID uint32 `json:"origin_chain_id"` DestChainID uint32 `json:"dest_chain_id"` OriginSender string `json:"origin_sender"` DestRecipient string `json:"dest_recipient"` OriginToken string `json:"origin_token"` DestToken string `json:"dest_token"` OriginAmount string `json:"origin_amount"` DestAmount string `json:"dest_amount"` Deadline time.Time `json:"deadline"` OriginNonce int `json:"origin_nonce"` RawRequest string `json:"raw_request"` SendChainGas bool `json:"send_chain_gas"` }services/rfq/guard/service/guard.go (1)
31-40: Consider adding comments for struct fields.Adding comments for each struct field can improve readability and maintainability.
type Guard struct { cfg relconfig.Config // Configuration for the Guard metrics metrics.Handler // Metrics handler db guarddb.Service // Database service client omniClient.RPCClient // RPC client chainListeners map[int]listener.ContractListener // Map of chain listeners contracts map[int]*fastbridge.FastBridgeRef // Map of FastBridge contracts txSubmitter submitter.TransactionSubmitter // Transaction submitter }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- services/rfq/guard/guarddb/base/bridge.go (1 hunks)
- services/rfq/guard/guarddb/base/doc.go (1 hunks)
- services/rfq/guard/guarddb/base/model.go (1 hunks)
- services/rfq/guard/guarddb/base/proven.go (1 hunks)
- services/rfq/guard/guarddb/base/store.go (1 hunks)
- services/rfq/guard/guarddb/connect/sql.go (1 hunks)
- services/rfq/guard/guarddb/db.go (1 hunks)
- services/rfq/guard/guarddb/doc.go (1 hunks)
- services/rfq/guard/guarddb/mysql/mysql.go (1 hunks)
- services/rfq/guard/guarddb/pendingprovenstatus_string.go (1 hunks)
- services/rfq/guard/guarddb/sqlite/sqlite.go (1 hunks)
- services/rfq/guard/service/guard.go (1 hunks)
Files skipped from review due to trivial changes (2)
- services/rfq/guard/guarddb/base/doc.go
- services/rfq/guard/guarddb/doc.go
Additional context used
GitHub Check: Build (1.22.x, ubuntu-latest)
services/rfq/guard/guarddb/base/store.go
[failure] 44-44:
cannot use &Store{} (value of type *Store) as guarddb.Service value in variable declaration: *Store does not implement guarddb.Service (wrong type for method GetPendingProvensByStatus)
GitHub Check: Lint (services/rfq)
services/rfq/guard/guarddb/mysql/mysql.go
[failure] 12-12:
could not import github.com/synapsecns/sanguine/services/rfq/guard/guarddb/base (-: # github.com/synapsecns/sanguine/services/rfq/guard/guarddb/baseservices/rfq/guard/guarddb/sqlite/sqlite.go
[failure] 10-10:
could not import github.com/synapsecns/sanguine/services/rfq/guard/guarddb/base (-: # github.com/synapsecns/sanguine/services/rfq/guard/guarddb/base
[failure] 62-62:
cannot use &Store{} (value of type *Store) as guarddb.Service value in variable declaration: *Store does not implement guarddb.Service (missing method GetBridgeRequestByID) (typecheck)
Additional comments not posted (33)
services/rfq/guard/guarddb/pendingprovenstatus_string.go (1)
1-27: LGTM!The
Stringmethod forPendingProvenStatusenum is correctly implemented and follows the standard pattern used by stringer.services/rfq/guard/guarddb/connect/sql.go (1)
1-39: LGTM!The
Connectfunction correctly handles database connections for MySQL, SQLite, and Clickhouse with appropriate error handling.services/rfq/guard/guarddb/base/bridge.go (2)
14-25: LGTM!The
StoreBridgeRequestfunction correctly handles storing bridge requests in the database with proper conflict resolution and error handling.
27-44: LGTM!The
GetBridgeRequestByIDfunction correctly handles retrieving bridge requests by ID with proper error handling for record not found and other errors.services/rfq/guard/guarddb/base/store.go (4)
19-24: LGTM!The
NewStorefunction correctly initializes theStorestruct with the necessary components.
26-29: LGTM!The
DBfunction correctly returns thegorm.DBobject.
31-34: LGTM!The
SubmitterDBfunction correctly returns thesubmitterDB.Serviceobject.
36-41: LGTM!The
GetAllModelsfunction correctly aggregates all models for migration.services/rfq/guard/guarddb/mysql/mysql.go (1)
12-12: Address the import issue.Static analysis reported an import issue with
github.com/synapsecns/sanguine/services/rfq/guard/guarddb/base.Ensure that the import path is correct and the package is available.
Verification successful
Import path is correct and the package is available.
- The
services/rfq/guard/guarddb/base/directory contains multiple.gofiles, confirming it is a valid package.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the import path for `base` package. # Test: Check if the `base` package is available. Expect: The package should be present. fd 'base' services/rfq/guard/guarddbLength of output: 69
Script:
#!/bin/bash # Description: Verify the presence of Go files in the `base` package. # Test: Check if there are any `.go` files in the `base` package directory. Expect: At least one `.go` file should be present. fd '.go' services/rfq/guard/guarddb/baseLength of output: 245
Tools
GitHub Check: Lint (services/rfq)
[failure] 12-12:
could not import github.com/synapsecns/sanguine/services/rfq/guard/guarddb/base (-: # github.com/synapsecns/sanguine/services/rfq/guard/guarddb/baseservices/rfq/guard/guarddb/sqlite/sqlite.go (1)
10-10: Address the import issue.Static analysis reported an import issue with
github.com/synapsecns/sanguine/services/rfq/guard/guarddb/base.Ensure that the import path is correct and the package is available.
Verification successful
Import path is correct.
The
basepackage is available at the specified path:services/rfq/guard/guarddb/base/.
- The import path
github.com/synapsecns/sanguine/services/rfq/guard/guarddb/baseis correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the import path for `base` package. # Test: Check if the `base` package is available. Expect: The package should be present. fd 'base' services/rfq/guard/guarddbLength of output: 69
Tools
GitHub Check: Lint (services/rfq)
[failure] 10-10:
could not import github.com/synapsecns/sanguine/services/rfq/guard/guarddb/base (-: # github.com/synapsecns/sanguine/services/rfq/guard/guarddb/baseservices/rfq/guard/guarddb/base/proven.go (1)
14-25: LGTM!The
StorePendingProvenmethod is well-structured and correctly handles storing pending proven events with conflict resolution.services/rfq/guard/guarddb/db.go (3)
24-32: LGTM!The
Writerinterface is well-defined and includes methods for managing bridge requests and pending proven events.
34-42: LGTM!The
Readerinterface is well-defined and includes methods for retrieving bridge requests and pending proven events by status and ID.
44-51: LGTM!The
Serviceinterface is well-defined and includes methods for managing bridge requests, pending proven events, and retrieving the submitter database service.services/rfq/guard/guarddb/base/model.go (7)
58-66: LGTM!The function correctly converts
PendingProventoPendingProvenModel.
70-78: LGTM!The function correctly converts a
common.Hashtosql.NullString.
80-88: LGTM!The function correctly converts a
stringtosql.NullString.
90-108: LGTM!The function correctly converts
PendingProvenModeltoPendingProven.
146-161: LGTM!The function correctly converts
BridgeRequesttoBridgeRequestModel.
164-205: LGTM!The function correctly converts
BridgeRequestModeltoBridgeRequest.
208-215: LGTM!The function correctly converts a slice to a fixed-size array.
services/rfq/guard/service/guard.go (12)
42-109: LGTM!The function correctly initializes a new
Guardinstance with proper error handling.
114-138: LGTM!The function correctly starts the
Guardwith proper error handling.
140-154: LGTM!The function correctly runs the database selector with proper error handling.
156-171: LGTM!The function correctly starts the chain indexers with proper error handling.
173-228: LGTM!The function correctly runs the chain indexer with proper error handling.
232-266: LGTM!The function correctly handles the
BridgeRequestedlog with proper error handling.
268-281: LGTM!The function correctly handles the
ProofProvidedlog with proper error handling.
283-290: LGTM!The function correctly handles the
ProofDisputedlog with proper error handling.
292-302: LGTM!The function correctly processes the database with proper error handling.
304-348: LGTM!The function correctly handles the
ProveCalledevent with proper error handling.
350-382: LGTM!The function correctly checks if the prove is valid with proper error handling.
384-408: LGTM!The function correctly checks if the relay matches the bridge request.
| return allModels | ||
| } | ||
|
|
||
| var _ guarddb.Service = &Store{} |
There was a problem hiding this comment.
Implement the missing method GetPendingProvensByStatus.
The Store struct does not fully implement the guarddb.Service interface. It lacks the GetPendingProvensByStatus method.
+ func (s Store) GetPendingProvensByStatus(ctx context.Context, matchStatuses ...guarddb.PendingProvenStatus) ([]guarddb.PendingProven, error) {
+ // Implementation here
+ }Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Build (1.22.x, ubuntu-latest)
[failure] 44-44:
cannot use &Store{} (value of type *Store) as guarddb.Service value in variable declaration: *Store does not implement guarddb.Service (wrong type for method GetPendingProvensByStatus)
| // Package mysql provides a common interface for starting sql-lite databases | ||
| package mysql |
There was a problem hiding this comment.
Correct the package description.
The package description mentions sql-lite databases, but this file is for MySQL databases.
- // Package mysql provides a common interface for starting sql-lite databases
+ // Package mysql provides a common interface for starting MySQL databasesCommittable 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.
| // Package mysql provides a common interface for starting sql-lite databases | |
| package mysql | |
| // Package mysql provides a common interface for starting MySQL databases | |
| package mysql |
| gdb, err := gorm.Open(mysql.Open(dbURL), &gorm.Config{ | ||
| Logger: dbcommon.GetGormLogger(logger), | ||
| FullSaveAssociations: true, | ||
| NamingStrategy: NamingStrategy, | ||
| NowFunc: time.Now, |
There was a problem hiding this comment.
Fix the error message for MySQL connection.
The error message on line 38 should specify that it is related to MySQL.
- return nil, fmt.Errorf("could not create mysql connection: %w", err)
+ return nil, fmt.Errorf("could not create MySQL connection: %w", err)Committable suggestion was skipped due to low confidence.
| // Store is the sqlite store. It extends the base store for sqlite specific queries. | ||
| type Store struct { | ||
| *base.Store | ||
| } | ||
|
|
||
| // MaxIdleConns is exported here for testing. Tests execute too slowly with a reconnect each time. | ||
| var MaxIdleConns = 0 | ||
|
|
||
| // NamingStrategy is used to exported here for testing. | ||
| var NamingStrategy = schema.NamingStrategy{} |
There was a problem hiding this comment.
Improve comments for clarity.
The comments for Store, MaxIdleConns, and NamingStrategy could be more descriptive.
- // Store is the sqlite store. It extends the base store for sqlite specific queries.
+ // Store is the MySQL store. It extends the base store for MySQL-specific queries.
- // MaxIdleConns is exported here for testing. Tests execute too slowly with a reconnect each time.
+ // MaxIdleConns specifies the maximum number of idle connections. Exported for testing purposes.
- // NamingStrategy is used to exported here for testing.
+ // NamingStrategy specifies the naming strategy for GORM. Exported for testing purposes.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.
| // Store is the sqlite store. It extends the base store for sqlite specific queries. | |
| type Store struct { | |
| *base.Store | |
| } | |
| // MaxIdleConns is exported here for testing. Tests execute too slowly with a reconnect each time. | |
| var MaxIdleConns = 0 | |
| // NamingStrategy is used to exported here for testing. | |
| var NamingStrategy = schema.NamingStrategy{} | |
| // Store is the MySQL store. It extends the base store for MySQL-specific queries. | |
| type Store struct { | |
| *base.Store | |
| } | |
| // MaxIdleConns specifies the maximum number of idle connections. Exported for testing purposes. | |
| var MaxIdleConns = 0 | |
| // NamingStrategy specifies the naming strategy for GORM. Exported for testing purposes. | |
| var NamingStrategy = schema.NamingStrategy{} |
| // create the directory to the store if it doesn't exist | ||
| 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.
Fix the error message for SQLite store creation.
The error message on line 38 should specify that it is related to SQLite.
- return nil, fmt.Errorf("could not create sqlite store")
+ return nil, fmt.Errorf("could not create SQLite store")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return nil, fmt.Errorf("could not create sqlite store") | |
| return nil, fmt.Errorf("could not create SQLite store") |
| // Package sqlite provides a common interface for starting sql-lite databases | ||
| package sqlite |
There was a problem hiding this comment.
Correct the package description.
The package description mentions sql-lite databases, which should be SQLite.
- // Package sqlite provides a common interface for starting sql-lite databases
+ // Package sqlite provides a common interface for starting SQLite databasesCommittable 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.
| // Package sqlite provides a common interface for starting sql-lite databases | |
| package sqlite | |
| // Package sqlite provides a common interface for starting SQLite databases | |
| package sqlite |
| // GetPendingProvensByStatus gets pending provens by status. | ||
| func (s Store) GetPendingProvensByStatus(ctx context.Context, matchStatuses ...guarddb.PendingProvenStatus) (res []guarddb.PendingProven, _ error) { | ||
| var provenResults []PendingProvenModel | ||
|
|
||
| inArgs := make([]int, len(matchStatuses)) | ||
| for i := range matchStatuses { | ||
| inArgs[i] = int(matchStatuses[i].Int()) | ||
| } | ||
|
|
||
| // TODO: consider pagination | ||
| tx := s.DB().WithContext(ctx).Model(&PendingProvenModel{}).Where(fmt.Sprintf("%s IN ?", statusFieldName), inArgs).Find(&provenResults) | ||
| if tx.Error != nil { | ||
| return []guarddb.PendingProven{}, fmt.Errorf("could not get db results: %w", tx.Error) | ||
| } | ||
|
|
||
| for _, result := range provenResults { | ||
| marshaled, err := result.ToPendingProven() | ||
| if err != nil { | ||
| return []guarddb.PendingProven{}, fmt.Errorf("could not get provens") | ||
| } | ||
| res = append(res, *marshaled) | ||
| } | ||
| return res, nil | ||
| } |
There was a problem hiding this comment.
Consider implementing pagination.
The TODO comment suggests considering pagination for large result sets. Implementing pagination would improve performance and prevent potential memory issues.
Do you want me to help implement pagination for this method?
| func (s Store) UpdatePendingProvenStatus(ctx context.Context, id [32]byte, status guarddb.PendingProvenStatus) error { | ||
| tx := s.DB().WithContext(ctx).Model(&PendingProvenModel{}). | ||
| Where(fmt.Sprintf("%s = ?", transactionIDFieldName), hexutil.Encode(id[:])). | ||
| Update(statusFieldName, status) | ||
| if tx.Error != nil { | ||
| return fmt.Errorf("could not update: %w", tx.Error) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Use parameterized query for WHERE clause.
The WHERE clause should use parameterized queries to prevent SQL injection.
- .Where(fmt.Sprintf("%s = ?", transactionIDFieldName), hexutil.Encode(id[:])).
+ .Where(fmt.Sprintf("%s = ?", transactionIDFieldName), hexutil.Encode(id[:])).Committable suggestion was skipped due to low confidence.
| if tx.Error != nil { | ||
| return nil, fmt.Errorf("could not get proven") |
There was a problem hiding this comment.
Improve error message for GetPendingProvenByID.
The error message on line 72 should be more descriptive.
- return nil, fmt.Errorf("could not get proven")
+ return nil, fmt.Errorf("could not get pending proven by ID: %w", tx.Error)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if tx.Error != nil { | |
| return nil, fmt.Errorf("could not get proven") | |
| if tx.Error != nil { | |
| return nil, fmt.Errorf("could not get pending proven by ID: %w", tx.Error) |
| // PendingProvenStatus is the status of a quote request in the db. | ||
| // This is the primary mechanism for moving data through the app. | ||
| // | ||
| // TODO: consider making this an interface and exporting that. | ||
| // | ||
| // EXTREMELY IMPORTANT: DO NOT ADD NEW VALUES TO THIS ENUM UNLESS THEY ARE AT THE END. | ||
| // | ||
| //go:generate go run golang.org/x/tools/cmd/stringer -type=PendingProvenStatus | ||
| type PendingProvenStatus uint8 | ||
|
|
||
| const ( | ||
| // ProveCalled means the prove() function has been called. | ||
| ProveCalled PendingProvenStatus = iota + 1 | ||
| // Validated means the prove() call has been properly validated on the dest chain. | ||
| Validated | ||
| // DisputePending means dispute() has been called in the event of an invalid prove(). | ||
| DisputePending | ||
| // Disputed means the dispute() call has been confirmed. | ||
| Disputed | ||
| ) |
There was a problem hiding this comment.
Consider making PendingProvenStatus an interface.
The TODO comment suggests considering making the PendingProvenStatus enum an interface. This would provide more flexibility and allow for easier extension in the future.
Do you want me to help refactor the PendingProvenStatus enum into an interface?
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
enable_embedded_guardconfiguration parameter indocs/bridge/docs/rfq/Relayer/Relayer.md - Updated submodule commit in
services/cctp-relayer/external/evm-cctp-contracts - Corrected spelling in
services/rfq/guard/guarddb/doc.goandservices/rfq/relayer/reldb/doc.go - Clarified package description in
services/rfq/guard/guarddb/mysql/mysql.go - Renamed
UseEmbeddedGuardtoenable_guardinservices/rfq/relayer/relconfig/config.go
6 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| // MaxIdleConns is exported here for testing. Tests execute too slowly with a reconnect each time. | ||
| var MaxIdleConns = 0 | ||
|
|
||
| // NamingStrategy is used to exported here for testing. |
There was a problem hiding this comment.
📚 spelling: Typo: 'is used to exported here' should be 'is exported here'.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Modified transaction submission logic in
services/rfq/guard/service/handlers.goforDisputefunction - Ensured
transactorparameter is correctly passed toDisputemethod inhandleProveCalled - No major changes found since last review
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
| } | ||
| err = g.db.StoreBridgeRequest(ctx, dbReq) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get db: %w", err) |
There was a problem hiding this comment.
🪶 style: The error message should specify that it failed to store the bridge request in the database.
| } | ||
|
|
||
| func relayMatchesBridgeRequest(event *fastbridge.FastBridgeBridgeRelayed, bridgeRequest *guarddb.BridgeRequest) bool { | ||
| //TODO: is this exhaustive? |
There was a problem hiding this comment.
🪶 style: Consider elaborating on the TODO comment to specify what additional checks might be needed.
Bundle ReportChanges will decrease total bundle size by 367.51kB ⬇️
|
| if !ok { | ||
| return fmt.Errorf("could not get contract for chain: %d", bridgeRequest.Transaction.DestChainId) | ||
| } | ||
| _, err = g.txSubmitter.SubmitTransaction(ctx, big.NewInt(int64(proven.Origin)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { |
There was a problem hiding this comment.
@dwasse test is failing WARN [07-05|17:06:42.900] Served eth_estimateGas conn=127.0.0.1:63211 reqid=77 duration="412.333µs" err="execution reverted" errdata=0xe2517d3f0000000000000000000000000000000000000000000000000000000000000000043c983c49d46f0e102151eaf8085d4a2e6571d5df2d47b013f39bddfd4a639d
There was a problem hiding this comment.
PR Summary
(updates since last review)
-
New Features
- Introduced RFQ Guard component in
contrib/opbot/botmd/botmd.goandcontrib/opbot/botmd/commands.go - Added SQLite and MySQL support in
contrib/opbot/sql/* - New API endpoints for bulk quotes and contract addresses in
services/rfq/api/*
- Introduced RFQ Guard component in
-
Improvements
- Enhanced configuration management in
contrib/opbot/config/config.go - Improved context propagation in
contrib/opbot/botmd/middleware.go
- Enhanced configuration management in
-
Dependency Updates
- Updated
github.com/gorilla/websocketto v1.5.3 across multiplego.modfiles
- Updated
53 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
Startedmethod and mutex in/ethergo/submitter/submitter.gofor thread-safe transaction submission. - Introduced
BridgeProofDisputedTopicin/services/rfq/contracts/fastbridge/events.gofor handling bridge disputes. - Added
BridgeDisputeEventto EventType enum in/services/rfq/contracts/fastbridge/eventtype_string.go. - Implemented parsing logic for
BridgeDisputeEventin/services/rfq/contracts/fastbridge/parser.go. - Integrated new
Guardcomponent in/services/rfq/guard/service/guard.gofor monitoring and verifying bridge transactions. - Modified chain ID references in
/services/rfq/guard/service/handlers.goto ensure correct dispute handling.
6 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| if !g.txSubmitter.Started() { | ||
| err = g.txSubmitter.Start(ctx) | ||
| // defensive coding against potential race. | ||
| if err != nil && !errors.Is(err, submitter.ErrSubmitterAlreadyStarted) { |
There was a problem hiding this comment.
🧠 logic: Potential race condition check for ErrSubmitterAlreadyStarted.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Enhanced
generateQuotesmethod inservices/rfq/relayer/quoter/quoter.goto use goroutines for parallel execution. - Added mutex for synchronization in
generateQuotesto ensure thread safety. - Improved performance and efficiency of quote generation process.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- docs/bridge/docs/rfq/Relayer/Relayer.md (1 hunks)
- ethergo/submitter/submitter.go (6 hunks)
- services/rfq/contracts/fastbridge/events.go (4 hunks)
- services/rfq/contracts/fastbridge/eventtype_string.go (1 hunks)
- services/rfq/contracts/fastbridge/parser.go (2 hunks)
- services/rfq/guard/guarddb/doc.go (1 hunks)
- services/rfq/guard/guarddb/mysql/mysql.go (1 hunks)
- services/rfq/guard/service/guard.go (1 hunks)
- services/rfq/guard/service/handlers.go (1 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/reldb/doc.go (1 hunks)
Files skipped from review due to trivial changes (2)
- services/rfq/guard/guarddb/doc.go
- services/rfq/relayer/reldb/doc.go
Files skipped from review as they are similar to previous changes (4)
- services/rfq/guard/guarddb/mysql/mysql.go
- services/rfq/guard/service/guard.go
- services/rfq/guard/service/handlers.go
- services/rfq/relayer/relconfig/config.go
Additional context used
GitHub Check: codecov/patch
services/rfq/contracts/fastbridge/events.go
[warning] 56-56: services/rfq/contracts/fastbridge/events.go#L56
Added line #L56 was not covered by tests
[warning] 68-68: services/rfq/contracts/fastbridge/events.go#L68
Added line #L68 was not covered by testsservices/rfq/contracts/fastbridge/parser.go
[warning] 87-92: services/rfq/contracts/fastbridge/parser.go#L87-L92
Added lines #L87 - L92 were not covered by tests
Additional comments not posted (22)
services/rfq/contracts/fastbridge/eventtype_string.go (2)
15-15: LGTM!The added check for
BridgeDisputeEventensures that the constant values remain unchanged.
18-20: LGTM!The added string representation and index for
BridgeDisputeEventare correctly implemented.services/rfq/contracts/fastbridge/events.go (4)
20-21: LGTM!The added topic for
BridgeProofDisputedis correctly implemented.
37-37: LGTM!The initialization of the
BridgeProofDisputedtopic is correctly implemented.
53-57: LGTM!The check for the existence of the
BridgeProofDisputedevent in the ABI is correctly implemented.Tools
GitHub Check: codecov/patch
[warning] 56-56: services/rfq/contracts/fastbridge/events.go#L56
Added line #L56 was not covered by tests
68-68: LGTM!The addition of the
BridgeDisputeEventto the topic map is correctly implemented.Tools
GitHub Check: codecov/patch
[warning] 68-68: services/rfq/contracts/fastbridge/events.go#L68
Added line #L68 was not covered by testsservices/rfq/contracts/fastbridge/parser.go (1)
23-24: LGTM!The addition of the
BridgeDisputeEventconstant is correctly implemented.docs/bridge/docs/rfq/Relayer/Relayer.md (1)
196-197: LGTM!The additions of the
enable_guardandsubmit_single_quotesconfiguration options are correctly implemented.ethergo/submitter/submitter.go (14)
116-121: LGTM!The
Startedfunction is simple and correctly uses a read lock to ensure thread safety.
142-152: LGTM!The
attemptMarkStartedfunction correctly uses a write lock to ensure thread safety and returns an error if the submitter is already started.
154-155: LGTM!The
ErrSubmitterAlreadyStartedvariable is appropriately defined as an error.
347-354: LGTM!The
triggerProcessQueuefunction correctly triggers the process queue without blocking if the channel is full.
349-350: LGTM!The
ErrNotStartedvariable is appropriately defined as an error.
Line range hint
351-387: LGTM! Verify error handling and tracing.The
SubmitTransactionfunction correctly submits a transaction to the chain, handling nonce and gas price settings with appropriate error handling and tracing. Ensure that all errors are properly handled and traced.Verification successful
Verified: Error handling and tracing are properly implemented in the
SubmitTransactionfunction.
- The function uses
fmt.Errorffor descriptive error messages.- Tracing is implemented with
trace.WithAttributesto capture relevant details.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling and tracing in the SubmitTransaction function. # Test: Search for error handling and tracing. Expect: Proper error handling and tracing in the function. rg --type go 'fmt\.Errorf|trace\.WithAttributes' ethergo/submitter/submitter.goLength of output: 2444
Line range hint
323-335: LGTM! Verify error handling and tracing.The
storeTXfunction correctly stores a transaction in the database with a given status and UUID. It includes context tracing and error handling. Ensure that all errors are properly handled and traced.Verification successful
Verification successful: Error handling and tracing are appropriately implemented in the
storeTXfunction.The
storeTXfunction includes proper error handling usingfmt.Errorfand context tracing withtrace.WithAttributes. No further action is required.
- Error Handling:
fmt.Errorf("could not put tx: %w", err)- Tracing:
ctx, span := t.metrics.Tracer().Start(ctx, "submitter.StoreTX", trace.WithAttributes(Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling and tracing in the storeTX function. # Test: Search for error handling and tracing. Expect: Proper error handling and tracing in the function. rg --type go 'fmt\.Errorf|trace\.WithAttributes' ethergo/submitter/submitter.goLength of output: 2444
Line range hint
287-321: LGTM! Verify error handling and tracing.The
getNoncefunction correctly retrieves the nonce for a given chain ID and address using both on-chain and database sources. It parallelizes the retrieval with an errgroup and includes error handling and context tracing. Ensure that all errors are properly handled and traced.Verification successful
The
getNoncefunction contains proper error handling and tracing through the use offmt.Errorfandtrace.WithAttributes. These implementations are consistent with the expected patterns for error reporting and tracing.
- Error Handling: Proper use of
fmt.Errorfto wrap errors with contextual information.- Tracing: Usage of
trace.WithAttributesto add attributes to spans for better observability.Verification Complete
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling and tracing in the getNonce function. # Test: Search for error handling and tracing. Expect: Proper error handling and tracing in the function. rg --type go 'fmt\.Errorf|trace\.WithAttributes' ethergo/submitter/submitter.goLength of output: 2444
Line range hint
491-511: LGTM! Verify context tracing.The
applyGasFloorfunction correctly applies the minimum gas price from the config if values are unset, considering dynamic gas pricing. It includes context tracing. Ensure that the context tracing is properly implemented.Verification successful
Context tracing verified in
applyGasFloorfunctionThe
applyGasFloorfunction includes proper context tracing, starting a span and setting attributes related to gas pricing before ending the span.
- Location:
ethergo/submitter/submitter.golines 535-558Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify context tracing in the applyGasFloor function. # Test: Search for context tracing. Expect: Proper context tracing in the function. rg --type go 'trace\.WithAttributes' ethergo/submitter/submitter.goLength of output: 841
Script:
#!/bin/bash # Description: Verify context tracing in the applyGasFloor function. # Test: Search for the applyGasFloor function and ensure it includes proper context tracing. ast-grep --lang go --pattern 'func (t *txSubmitterImpl) applyGasFloor(ctx context.Context, $$$) { $$$ }' ethergo/submitter/submitter.goLength of output: 1880
Line range hint
156-206: LGTM! Verify context cancellation handling.The
Startfunction correctly initializes the transaction submitter and starts reaper and distinct chain ID update processes using goroutines. Ensure that context cancellation is properly handled to avoid potential goroutine leaks.Verification successful
Context cancellation handling verified
The
Startfunction correctly handles context cancellation by calling thecancelfunction when theshouldExitcondition is met, ensuring proper termination of goroutines.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify context cancellation handling in the Start function. # Test: Search for context cancellation handling. Expect: Proper cancellation handling in all goroutines. rg --type go 'context\.WithCancel' ethergo/submitter/submitter.goLength of output: 113
Script:
#!/bin/bash # Description: Verify proper usage of the cancel function in the Start function. # Test: Search for the usage of the cancel function. Expect: Proper usage of cancel function to handle context cancellation. rg --type go '\bcancel\(\)' ethergo/submitter/submitter.goLength of output: 72
Script:
#!/bin/bash # Description: Extract the lines around the cancel function call to understand its context. # Test: Extract lines around the cancel function call. Expect: Proper context cancellation handling. rg --type go -C 5 '\bcancel\(\)' ethergo/submitter/submitter.goLength of output: 209
Line range hint
539-560: LGTM! Verify error handling and tracing.The
applyGasCeilfunction correctly evaluates current gas values versus the configured maximum and returns an error if they exceed the maximum. It includes context tracing and error handling. Ensure that all errors are properly handled and traced.
Line range hint
407-455: LGTM! Verify error handling and tracing.The
setGasPricefunction correctly sets the gas price for a transaction, considering dynamic gas pricing and applying gas floor and ceiling. It includes context tracing and error handling. Ensure that all errors are properly handled and traced.
Line range hint
577-599: LGTM! Verify error handling and tracing.The
getGasBlockfunction correctly retrieves the gas block for a given chain, using a retry mechanism and a cache as a fallback. It includes context tracing and error handling. Ensure that all errors are properly handled and traced.Verification successful
Verification of Error Handling and Tracing in
getGasBlockFunctionThe
getGasBlockfunction inethergo/submitter/submitter.gocorrectly implements error handling and tracing. The function uses context tracing (span) and manages errors usingfmt.Errorf. Additionally, it employs a retry mechanism and falls back to a cached value if the gas block cannot be retrieved.Key points:
- The function starts a trace span and ensures it ends with the error status.
- Errors are properly wrapped with context using
fmt.Errorf.- If the gas block retrieval fails, the function attempts to use a cached value and logs this event.
Code Locations:
ethergo/submitter/submitter.go:642-675Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling and tracing in the getGasBlock function. # Test: Search for error handling and tracing. Expect: Proper error handling and tracing in the function. rg --type go 'fmt\.Errorf|trace\.WithAttributes' ethergo/submitter/submitter.goLength of output: 2444
Script:
#!/bin/bash # Extract the getGasBlock function from ethergo/submitter/submitter.go to verify error handling and tracing. ast-grep --lang go --pattern 'func getGasBlock($_, $_) $_' ethergo/submitter/submitter.goLength of output: 91
Script:
#!/bin/bash # Search for the getGasBlock function in ethergo/submitter/submitter.go to verify its existence and locate it. rg 'func getGasBlock' ethergo/submitter/submitter.goLength of output: 54
Script:
#!/bin/bash # Search for all function definitions in ethergo/submitter/submitter.go to identify any relevant functions. rg --type go 'func ' ethergo/submitter/submitter.goLength of output: 2209
Script:
#!/bin/bash # Extract the full implementation of the getGasBlock function from ethergo/submitter/submitter.go to verify error handling and tracing. ast-grep --lang go --pattern 'func (t *txSubmitterImpl) getGasBlock(ctx context.Context, chainClient client.EVM, chainID int) (gasBlock *types.Header, err error) { $$$ }' ethergo/submitter/submitter.goLength of output: 2527
Line range hint
513-537: LGTM! Verify error handling and tracing.The
applyGasFromOraclefunction correctly fetches gas values from an RPC endpoint and attempts to set them, overriding existing values if the oracle values are higher. It includes context tracing and error handling. Ensure that all errors are properly handled and traced.Verification successful
Verification successful: Error handling and tracing in
applyGasFromOracleare properly implemented.The
applyGasFromOraclefunction includes appropriate error handling usingfmt.Errorfand context tracing withtrace.WithAttributes.
Error handling is present at:
- Line 572:
return fmt.Errorf("could not get gas fee cap: %w", err)- Line 577:
return fmt.Errorf("could not get gas tip cap: %w", err)- Line 589:
return fmt.Errorf("could not get gas price: %w", err)Tracing is implemented at:
- Line 563:
ctx, span := t.metrics.Tracer().Start(ctx, "submitter.applyGasFromOracle")- Line 566:
metrics.EndSpanWithErr(span, err)- Line 580:
span.SetAttributes(...)- Line 592:
span.SetAttributes(...)This confirms that the function is correctly using error handling and tracing mechanisms.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling and tracing in the applyGasFromOracle function. # Test: Search for error handling and tracing. Expect: Proper error handling and tracing in the function. rg --type go 'fmt\.Errorf|trace\.WithAttributes' ethergo/submitter/submitter.goLength of output: 2444
Script:
#!/bin/bash # Description: Verify error handling and tracing in the applyGasFromOracle function. # Find the applyGasFromOracle function definition and its scope ast-grep --lang go --pattern 'func (t *txSubmitterImpl) applyGasFromOracle(ctx context.Context, transactor *bind.TransactOpts, chainID int) error { $$$ }' ethergo/submitter/submitter.goLength of output: 187
Script:
#!/bin/bash # Description: Verify error handling and tracing in the applyGasFromOracle function. # Find the applyGasFromOracle function definition by its name rg --type go 'func.*applyGasFromOracle' ethergo/submitter/submitter.goLength of output: 219
Script:
#!/bin/bash # Description: Verify error handling and tracing in the applyGasFromOracle function. # Find the applyGasFromOracle function definition and its scope ast-grep --lang go --pattern 'func (t *txSubmitterImpl) applyGasFromOracle(ctx context.Context, transactor *bind.TransactOpts, client client.EVM, useDynamic bool) (err error) { $$$ }' ethergo/submitter/submitter.goLength of output: 2988
| case BridgeDisputeEvent: | ||
| disputed, err := p.filterer.ParseBridgeProofDisputed(log) | ||
| if err != nil { | ||
| return noOpEvent, nil, false | ||
| } | ||
| return eventType, disputed, true | ||
|
|
There was a problem hiding this comment.
LGTM! But add tests for the new case.
The addition of the BridgeDisputeEvent case in the ParseEvent method is correctly implemented.
However, the new lines are not covered by tests. 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] 87-92: services/rfq/contracts/fastbridge/parser.go#L87-L92
Added lines #L87 - L92 were not covered by tests
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Updated Docker image tag and specified platform as
linux/amd64in/ethergo/backends/anvil/anvil.go - Added error logging when submitter is not started in
/ethergo/submitter/submitter.go - Removed CI conditional skips and added guard service setup in
/services/rfq/e2e/rfq_test.go - Switched backend setup to Anvil and added guard wallet setup in
/services/rfq/e2e/setup_test.go
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- ethergo/backends/anvil/anvil.go (1 hunks)
- ethergo/submitter/submitter.go (6 hunks)
- services/rfq/e2e/rfq_test.go (7 hunks)
- services/rfq/e2e/setup_test.go (9 hunks)
Files skipped from review as they are similar to previous changes (3)
- ethergo/submitter/submitter.go
- services/rfq/e2e/rfq_test.go
- services/rfq/e2e/setup_test.go
Additional comments not posted (1)
ethergo/backends/anvil/anvil.go (1)
83-84: LGTM! The addition of thePlatformfield is appropriate.The
Platformfield in therunOptionsstruct ensures that the Docker container runs on the specified platform architecture ("linux/amd64"), which is a standard and necessary configuration.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced Guard service in
/services/rfq/e2e/rfq_test.go - Expanded integration tests for dispute handling and Guard setup in
/services/rfq/e2e/rfq_test.go - Minor formatting adjustments in
/ethergo/backends/anvil/anvil.go
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Addresses #2833
Summary by CodeRabbit
New Features
Bug Fixes
Tests
03bae19: docs preview link
47da4c8: docs preview link
27d978f: docs preview link
3104af9: docs preview link
0d653c4: docs preview link
277cac1: docs preview link
2cde201: docs preview link