feat(rfq): active quoting API #3128
Conversation
WalkthroughThis pull request introduces significant updates to the RFQ (Request for Quote) service, including modifications to request handling structures, the addition of new types for user quote requests and responses, and enhancements to WebSocket functionality. The changes improve the overall management of quote requests while maintaining existing functionalities across multiple files. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3128 +/- ##
===================================================
- Coverage 38.22938% 33.25636% -4.97302%
===================================================
Files 423 539 +116
Lines 24353 34646 +10293
Branches 119 82 -37
===================================================
+ Hits 9310 11522 +2212
- Misses 14301 22105 +7804
- Partials 742 1019 +277
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: |
526f2af
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bf62e6a3.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-active-rfq-api.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: 1
Outside diff range and nitpick comments (1)
services/rfq/api/rest/ws.go (1)
1-110: Excellent work on implementing the WebSocket client!The code follows a clear and modular structure, separating the interface definition from the concrete implementation. The use of channels for asynchronous communication allows for concurrent handling of requests and responses, and the
Runmethod effectively manages the WebSocket connection.Some suggestions for further improvement:
- Consider adding more detailed error messages in the
logger.Errorstatements to provide better context for debugging.- Address the TODO comment on line 104 and implement a mechanism to keep the connection alive, such as sending periodic ping messages.
Overall, the implementation is solid and well-structured. Great job!
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- services/rfq/api/model/request.go (2 hunks)
- services/rfq/api/model/response.go (2 hunks)
- services/rfq/api/rest/handler.go (2 hunks)
- services/rfq/api/rest/mocks/ws_client.go (1 hunks)
- services/rfq/api/rest/rfq.go (1 hunks)
- services/rfq/api/rest/server.go (9 hunks)
- services/rfq/api/rest/ws.go (1 hunks)
Files skipped from review due to trivial changes (2)
- services/rfq/api/model/request.go
- services/rfq/api/rest/mocks/ws_client.go
Additional comments not posted (24)
services/rfq/api/rest/rfq.go (3)
13-29: LGTM!The
getBestQuotefunction correctly compares two quotes and returns the one with the higher destination amount. It handles nil pointers and converts the destination amounts from strings to big integers for comparison.
31-82: LGTM!The
handleActiveRFQfunction correctly handles active RFQ by publishing the quote request to all connected clients, collecting responses within the expiration window, and returning the best quote. It uses appropriate synchronization primitives to ensure thread safety and correctness.
84-129: LGTM!The
handlePassiveRFQfunction correctly handles passive RFQ by retrieving quotes from the database, validating the origin amount, calculating the effective destination amount after applying fixed fees, and returning the best quote. The logic is sound and the implementation is correct.services/rfq/api/model/response.go (8)
52-56: LGTM!The
ActiveRFQMessagestruct is well-defined and serves its purpose of representing WebSocket messages for Active RFQ. TheContentfield being aninterface{}allows flexibility in the message content.
59-63: LGTM!The
PutUserQuoteRequeststruct is well-defined and captures the necessary information for a user quote request. TheQuoteDatafield being a separate struct allows for clean separation of concerns.
66-72: LGTM!The
PutUserQuoteResponsestruct is well-defined and captures the necessary information for a response to a user quote request. TheReasonfield allows for providing additional context in case of failure.
75-79: LGTM!The
QuoteRequeststruct is well-defined and captures the necessary information for a quote request. TheRequestIDfield allows for unique identification of the request, and theCreatedAtfield captures the timestamp of the request creation.
82-91: LGTM!The
QuoteDatastruct is well-defined and captures the necessary information for the data within a quote request. The use of pointers forDestAmountandRelayerAddressallows for optional fields.
94-98: LGTM!The
RelayerWsQuoteRequeststruct is well-defined and captures the necessary information for a quote request to a relayer. It is similar to theQuoteRequeststruct, but specifically tailored for relayer requests.
101-107: LGTM!The
NewRelayerWsQuoteRequestfunction is well-defined and serves its purpose of creating a newRelayerWsQuoteRequestwith a uniqueRequestIDand the current timestamp. It encapsulates the creation logic and provides a clean interface for creating new instances of the struct.
110-115: LGTM!The
RelayerWsQuoteResponsestruct is well-defined and captures the necessary information for a response to a quote request. TheRequestIDfield allows for matching the response to the corresponding request, and theQuoteIDfield provides a unique identifier for the quote. TheUpdatedAtfield captures the timestamp of the response.services/rfq/api/rest/handler.go (2)
66-66: LGTM!The change in request type from
*model.PutQuoteRequestto*model.PutRelayerQuoteRequestaligns with the updatedparseDBQuotefunction signature and indicates that the function is now specifically handling relayer quote requests. The rest of the function logic remains unchanged.
136-136: LGTM!The change in request type from
model.PutQuoteRequesttomodel.PutRelayerQuoteRequestaligns with the updatedModifyQuotefunction. The function is correctly parsing the fields from the new request type, and the rest of the function logic remains unchanged.services/rfq/api/rest/server.go (11)
13-13: Verify the necessity and security of thexsyncpackage.The
xsyncpackage is a third-party package that provides synchronization primitives. Please ensure that:
- The functionality provided by
xsyncis necessary and cannot be achieved using the standard library.- The package has been thoroughly vetted for security and performance.
- The package is actively maintained and has a good track record.
24-24: Verify the necessity and security of thewebsocketpackage.The
websocketpackage fromgithub.com/gorilla/websocketis a third-party package for WebSocket implementation. Please ensure that:
- The package is necessary for the WebSocket functionality required in the project.
- The package has been thoroughly vetted for security and performance.
- The package is actively maintained and has a good track record.
53-53: LGTM!The addition of the
upgraderfield of typewebsocket.Upgraderto theQuoterAPIServerstruct is necessary for handling WebSocket connections in the API server.
64-67: LGTM!The addition of the
wsClientsfield of type*xsync.MapOf[string, WsClient]to theQuoterAPIServerstruct is necessary for managing WebSocket connections and sending quote requests to the connected clients. The use ofxsync.MapOfprovides synchronization for concurrent access to the map.
139-139: LGTM!The initialization of the
wsClientsfield with a new instance ofxsync.MapOf[WsClient]()is necessary to ensure that it is ready to be used for managing WebSocket connections.
166-169: LGTM!The addition of the new API endpoints
QuoteRequestsRouteandPutQuoteRequestRouteis in line with the PR objectives of introducing active quoting API. These endpoints are necessary for handling WebSocket connections and user quote requests.
198-202: LGTM!The addition of the
activeRFQGetroute group with theQuoteRequestsRouteendpoint is necessary for handling active quote requests via WebSocket. The use ofAuthMiddlewareensures that only authenticated clients can access the endpoint, and theGetActiveRFQWebsocketmethod is responsible for upgrading the HTTP connection to a WebSocket connection and managing the connection.
208-210: Verify the security implications of allowing unauthenticated access to thePutUserQuoteRequestmethod.The addition of the route for handling RFQ requests without the
AuthMiddlewareallows users to submit quote requests without authentication. While this may be necessary for user convenience, it is important to ensure that thePutUserQuoteRequestmethod is secure and does not expose any sensitive information or allow unauthorized actions.Please thoroughly review the implementation of the
PutUserQuoteRequestmethod and ensure that it follows security best practices and does not introduce any vulnerabilities.
240-240: LGTM!The renaming of the
reqvariable frommodel.PutQuoteRequesttomodel.PutRelayerQuoteRequestis consistent with the alteration mentioned in the AI-generated summary. The renaming improves clarity and indicates that the request is specifically for relayer quotes.
400-438: LGTM!The implementation of the
GetActiveRFQWebsocketmethod follows the necessary steps for handling WebSocket connections for active quote requests. It upgrades the HTTP connection to a WebSocket connection, retrieves the relayer address from the context, ensures that only one connection per relayer is allowed, creates a newWsClientinstance, stores it in thewsClientsmap, and runs theWsClientinstance in a separate goroutine.The use of the relayer address as the connection ID ensures uniqueness and allows for easy identification of the connected relayer. The check for existing connections prevents multiple connections from the same relayer. The creation and storage of the
WsClientinstance allows for managing the connection and sending quote requests to the connected relayer. Running theWsClientinstance in a separate goroutine ensures that it can handle incoming messages concurrently.Overall, the implementation looks solid and follows the necessary steps for handling WebSocket connections for active quote requests.
440-443: LGTM!The use of constants
quoteTypeActiveandquoteTypePassiveprovides a clear and readable way to differentiate between active and passive quotes. Using constants helps avoid typos and ensures consistency throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (12)
services/rfq/api/client/client.go (4)
95-112: LGTM: Robust authentication header generationThe
getAuthHeaderfunction is well-implemented:
- It correctly uses the current timestamp as part of the authentication.
- The message signing process follows the Ethereum signed message format.
- The final header combines the timestamp and the signed message.
This implementation provides a secure way to authenticate requests.
Consider wrapping the error returned from
reqSigner.SignMessageto provide more context:if err != nil { - return "", fmt.Errorf("failed to sign request: %w", err) + return "", fmt.Errorf("failed to sign authentication message: %w", err) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-109: services/rfq/api/client/client.go#L108-L109
Added lines #L108 - L109 were not covered by tests
137-137: LGTM: Updated PutQuote parameter typeThe
PutQuotemethod has been correctly updated to accept a*model.PutRelayerQuoteRequest, which aligns with the changes made to theAuthenticatedClientinterface.Consider improving error handling by wrapping the returned error with additional context:
- return err + return fmt.Errorf("failed to put quote: %w", err)This will provide more informative error messages for debugging.
181-222: LGTM with suggestions: WebSocket subscription implementationThe
SubscribeActiveQuotesmethod is a solid implementation of WebSocket functionality for subscribing to active quotes. It handles connection, subscription, and concurrent message processing well.Consider the following improvements:
- Error handling: Wrap errors with more context to aid debugging.
- Resource management: Ensure proper cleanup in case of errors.
Apply this diff to enhance the implementation:
func (c *clientImpl) SubscribeActiveQuotes(ctx context.Context, req *model.SubscribeActiveRFQRequest, reqChan chan *model.ActiveRFQMessage) (respChan chan *model.ActiveRFQMessage, err error) { conn, err := c.connectWebsocket(ctx, req) if err != nil { - return nil, fmt.Errorf("failed to connect to websocket: %w", err) + return nil, fmt.Errorf("failed to connect to websocket for active quotes: %w", err) } + defer func() { + if err != nil { + conn.Close() + } + }() // ... (rest of the function) respChan = make(chan *model.ActiveRFQMessage) go func() { err = c.processWebsocket(ctx, conn, reqChan, respChan) if err != nil { - logger.Error("Error running websocket listener: %s", err) + logger.Errorf("Error running websocket listener for active quotes: %v", err) + close(respChan) } }() return respChan, nil }These changes improve error handling and ensure proper resource cleanup in case of errors.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 181-185: services/rfq/api/client/client.go#L181-L185
Added lines #L181 - L185 were not covered by tests
[warning] 188-201: services/rfq/api/client/client.go#L188-L201
Added lines #L188 - L201 were not covered by tests
[warning] 204-211: services/rfq/api/client/client.go#L204-L211
Added lines #L204 - L211 were not covered by tests
[warning] 213-218: services/rfq/api/client/client.go#L213-L218
Added lines #L213 - L218 were not covered by tests
[warning] 221-221: services/rfq/api/client/client.go#L221
Added line #L221 was not covered by tests
Line range hint
1-449: Overall assessment: Solid implementation with room for improvementThe changes to the RFQ API client introduce significant enhancements, particularly the new WebSocket functionality for active quote subscriptions. The implementation is generally well-structured and follows good practices. However, there are a few areas that could be improved:
- Error handling: Throughout the file, error messages could be more descriptive and context-specific.
- Resource management: Ensure proper cleanup of resources, especially in WebSocket-related functions.
- URL handling: The WebSocket URL conversion should properly handle both HTTP and HTTPS schemes.
- Context cancellation: Some methods could be more responsive to context cancellation.
Consider the following architectural improvements:
- Implement a more robust retry mechanism for WebSocket connections.
- Add telemetry or logging throughout the WebSocket lifecycle for better observability.
- Consider implementing a circuit breaker pattern for the WebSocket connection to handle temporary service unavailability.
Would you like assistance in implementing any of these suggestions or generating unit tests for the new functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 181-185: services/rfq/api/client/client.go#L181-L185
Added lines #L181 - L185 were not covered by tests
[warning] 188-201: services/rfq/api/client/client.go#L188-L201
Added lines #L188 - L201 were not covered by tests
[warning] 204-211: services/rfq/api/client/client.go#L204-L211
Added lines #L204 - L211 were not covered by tests
[warning] 213-218: services/rfq/api/client/client.go#L213-L218
Added lines #L213 - L218 were not covered by tests
[warning] 221-221: services/rfq/api/client/client.go#L221
Added line #L221 was not covered by tests
[warning] 224-227: services/rfq/api/client/client.go#L224-L227
Added lines #L224 - L227 were not covered by tests
[warning] 229-232: services/rfq/api/client/client.go#L229-L232
Added lines #L229 - L232 were not covered by tests
[warning] 234-242: services/rfq/api/client/client.go#L234-L242
Added lines #L234 - L242 were not covered by tests
[warning] 244-244: services/rfq/api/client/client.go#L244
Added line #L244 was not covered by tests
[warning] 247-259: services/rfq/api/client/client.go#L247-L259
Added lines #L247 - L259 were not covered by tests
[warning] 262-268: services/rfq/api/client/client.go#L262-L268
Added lines #L262 - L268 were not covered by tests
[warning] 271-294: services/rfq/api/client/client.go#L271-L294
Added lines #L271 - L294 were not covered by tests
[warning] 299-311: services/rfq/api/client/client.go#L299-L311
Added lines #L299 - L311 were not covered by tests
[warning] 315-323: services/rfq/api/client/client.go#L315-L323
Added lines #L315 - L323 were not covered by tests
[warning] 325-328: services/rfq/api/client/client.go#L325-L328
Added lines #L325 - L328 were not covered by tests
[warning] 333-338: services/rfq/api/client/client.go#L333-L338
Added lines #L333 - L338 were not covered by tests
[warning] 340-343: services/rfq/api/client/client.go#L340-L343
Added lines #L340 - L343 were not covered by tests
[warning] 345-345: services/rfq/api/client/client.go#L345
Added line #L345 was not covered by tests🪛 GitHub Check: Lint (services/rfq)
[failure] 273-273:
Error return value ofc.sendPingsis not checked (errcheck)services/rfq/api/rest/server.go (5)
56-56: New fields added to QuoterAPIServer structThe addition of
upgrader,wsClients, andpubSubManagerfields to the QuoterAPIServer struct indicates the implementation of WebSocket functionality for real-time communication. This is a good improvement for handling active quote requests.However, there are a couple of points to consider:
- The
upgraderfield is initialized later in the code. Consider initializing it in the NewAPI function for better encapsulation.- The use of
xsync.MapOfforwsClientsis a good choice for concurrent access, but make sure to properly handle concurrent read/write operations throughout the code.Also applies to: 67-71
171-178: New constants added for API routes and headersThe addition of new constants for API routes and headers improves code readability and maintainability. However, there are a few suggestions:
- Consider grouping related constants together (e.g., all route constants in one block, all header constants in another).
- The
ChainsHeaderandAuthorizationHeaderconstants are good, but ensure they are used consistently throughout the codebase.
210-215: New WebSocket route addedThe addition of a WebSocket route for handling active quote requests is a good improvement for real-time communication. However, there are a few points to consider:
- The use of an anonymous function for the route handler is fine for simple cases, but consider extracting it to a named method for better readability and testability if the logic becomes more complex.
- Ensure that proper error handling and logging are implemented within the
GetActiveRFQWebsocketmethod.
436-483: Implementation of GetActiveRFQWebsocket methodThe
GetActiveRFQWebsocketmethod handles WebSocket connections for active quote requests. Overall, the implementation looks good, but there are a few points to consider:
Error handling: The method returns early on errors without sending an error response to the client. Consider sending appropriate error messages back to the client.
Connection limit: The code correctly limits to one connection per relayer, which is good for preventing resource exhaustion.
Cleanup: The deferred cleanup of the WebSocket client is a good practice.
Logging: Consider adding more detailed logging throughout the method for easier debugging and monitoring.
Consider adding more detailed error responses and logging. For example:
if err != nil { logger.Error("Failed to set websocket upgrade", "error", err) c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to establish WebSocket connection"}) return }
504-577: Implementation of PutRFQRequest methodThe
PutRFQRequestmethod handles user requests for quotes. The implementation looks solid overall, but there are a few suggestions:
Input validation: Consider adding more robust input validation for the request payload.
Error handling: The error handling for
InsertActiveQuoteRequestonly logs the error. Consider returning an error response to the client in this case.Quote selection: The logic for selecting between active and passive quotes is good, but consider extracting it into a separate function for better readability and testability.
Response construction: The response construction logic is clear, but consider using a switch statement for better readability if more quote types are added in the future.
Consider extracting the quote selection logic into a separate function:
func selectBestQuote(activeQuote, passiveQuote *model.QuoteData) (*model.QuoteData, string) { if activeQuote == nil && passiveQuote == nil { return nil, "" } if activeQuote == nil { return passiveQuote, quoteTypePassive } if passiveQuote == nil { return activeQuote, quoteTypeActive } // Compare quotes and return the best one // ... }Then use this function in
PutRFQRequest:bestQuote, quoteType := selectBestQuote(activeQuote, passiveQuote)services/rfq/api/docs/docs.go (3)
295-319: Consider security implications and authentication for the WebSocket endpoint.Introducing a new WebSocket endpoint (
/rfq_stream) for receiving active quote requests is a significant change. While the documentation looks good, consider the following:
- Ensure proper authentication and authorization mechanisms are in place to restrict access to authorized clients only.
- Validate and sanitize incoming messages to prevent potential security vulnerabilities.
- Handle WebSocket connection lifecycle events (e.g.,
onConnect,onClose) appropriately.- Implement proper error handling and reconnection logic on the client-side.
424-443: Clarify the purpose and allowed values for thequote_typesfield.The
PutRFQRequestschema includes aquote_typesfield, which is an array of strings. However, the documentation doesn't specify the allowed values or the purpose of this field.To improve clarity, consider:
- Adding a description for the
quote_typesfield explaining its purpose.- Specifying the allowed values (e.g., "active", "passive") in the description or using an enum.
Apply this diff to add a description:
{ "type": "object", "properties": { ... "quote_types": { + "description": "Types of quotes to request (e.g., 'active', 'passive')", "type": "array", "items": { "type": "string" } }, ... } }
499-529: Add a description for theexpiration_windowfield.The
QuoteDataschema includes anexpiration_windowfield of type integer. To improve clarity, add a description specifying the unit of time (e.g., seconds, milliseconds) for the expiration window.Apply this diff to add a description:
{ "type": "object", "properties": { ... "expiration_window": { + "description": "Time until the quote expires, in seconds", "type": "integer" }, ... } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- services/rfq/api/client/client.go (8 hunks)
- services/rfq/api/docs/docs.go (7 hunks)
- services/rfq/api/docs/swagger.json (7 hunks)
- services/rfq/api/docs/swagger.yaml (7 hunks)
- services/rfq/api/model/request.go (3 hunks)
- services/rfq/api/model/response.go (2 hunks)
- services/rfq/api/rest/rfq.go (1 hunks)
- services/rfq/api/rest/server.go (12 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- services/rfq/api/docs/swagger.json
- services/rfq/api/docs/swagger.yaml
- services/rfq/api/model/request.go
- services/rfq/api/model/response.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/api/client/client.go
[warning] 82-82: services/rfq/api/client/client.go#L82
Added line #L82 was not covered by tests
[warning] 108-109: services/rfq/api/client/client.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 181-185: services/rfq/api/client/client.go#L181-L185
Added lines #L181 - L185 were not covered by tests
[warning] 188-201: services/rfq/api/client/client.go#L188-L201
Added lines #L188 - L201 were not covered by tests
[warning] 204-211: services/rfq/api/client/client.go#L204-L211
Added lines #L204 - L211 were not covered by tests
[warning] 213-218: services/rfq/api/client/client.go#L213-L218
Added lines #L213 - L218 were not covered by tests
[warning] 221-221: services/rfq/api/client/client.go#L221
Added line #L221 was not covered by tests
[warning] 224-227: services/rfq/api/client/client.go#L224-L227
Added lines #L224 - L227 were not covered by tests
[warning] 229-232: services/rfq/api/client/client.go#L229-L232
Added lines #L229 - L232 were not covered by tests
[warning] 234-242: services/rfq/api/client/client.go#L234-L242
Added lines #L234 - L242 were not covered by tests
[warning] 244-244: services/rfq/api/client/client.go#L244
Added line #L244 was not covered by tests
[warning] 247-259: services/rfq/api/client/client.go#L247-L259
Added lines #L247 - L259 were not covered by tests
[warning] 262-268: services/rfq/api/client/client.go#L262-L268
Added lines #L262 - L268 were not covered by tests
[warning] 271-294: services/rfq/api/client/client.go#L271-L294
Added lines #L271 - L294 were not covered by tests
[warning] 299-311: services/rfq/api/client/client.go#L299-L311
Added lines #L299 - L311 were not covered by tests
[warning] 315-323: services/rfq/api/client/client.go#L315-L323
Added lines #L315 - L323 were not covered by tests
[warning] 325-328: services/rfq/api/client/client.go#L325-L328
Added lines #L325 - L328 were not covered by tests
[warning] 333-338: services/rfq/api/client/client.go#L333-L338
Added lines #L333 - L338 were not covered by tests
[warning] 340-343: services/rfq/api/client/client.go#L340-L343
Added lines #L340 - L343 were not covered by tests
[warning] 345-345: services/rfq/api/client/client.go#L345
Added line #L345 was not covered by tests
[warning] 433-441: services/rfq/api/client/client.go#L433-L441
Added lines #L433 - L441 were not covered by tests
[warning] 443-445: services/rfq/api/client/client.go#L443-L445
Added lines #L443 - L445 were not covered by tests
[warning] 447-447: services/rfq/api/client/client.go#L447
Added line #L447 was not covered by tests
🪛 GitHub Check: Lint (services/rfq)
services/rfq/api/client/client.go
[failure] 273-273:
Error return value ofc.sendPingsis not checked (errcheck)services/rfq/api/rest/rfq.go
[failure] 159-159:
getBestQuote - result 1 (bool) is never used (unparam)
🔇 Additional comments (12)
services/rfq/api/client/client.go (4)
37-40: LGTM: Interface updates enhance functionalityThe changes to the
AuthenticatedClientinterface look good:
- The
PutQuotemethod now accepts a*model.PutRelayerQuoteRequestinstead of*model.PutQuoteRequest, which aligns with the intended changes.- The new
SubscribeActiveQuotesmethod introduces WebSocket functionality for real-time quote updates.These updates enhance the client's capabilities and provide more flexibility in handling quote requests.
50-50: LGTM: New method for unauthenticated quote requestsThe addition of the
PutRFQRequestmethod to theUnauthenticatedClientinterface is a good improvement. This allows users to submit quote requests without authentication, expanding the API's usability.
64-65: LGTM: Added signer for authenticationThe addition of the
reqSignerfield to theclientImplstruct is a good change. This signer is essential for generating authentication headers, which is used in the newgetAuthHeaderfunction. This change supports the enhanced authentication mechanism.Also applies to: 91-91
262-297:⚠️ Potential issueEnhance WebSocket processing robustness
The
processWebsocketmethod effectively manages the WebSocket communication loop. However, there are opportunities for improvement:
- Error handling: The error from
c.sendPingsis not checked.- Context cancellation: The method could be more responsive to context cancellation.
Apply this diff to address these issues:
func (c *clientImpl) processWebsocket(ctx context.Context, conn *websocket.Conn, reqChan, respChan chan *model.ActiveRFQMessage) (err error) { defer func() { close(respChan) err := conn.Close() if err != nil { logger.Warnf("error closing websocket connection: %v", err) } }() readChan := make(chan []byte) go c.listenWsMessages(ctx, conn, readChan) - go c.sendPings(ctx, reqChan) + pingErrChan := make(chan error, 1) + go func() { + pingErrChan <- c.sendPings(ctx, reqChan) + }() for { select { case <-ctx.Done(): - return nil + return ctx.Err() case msg, ok := <-reqChan: // ... existing code ... case msg, ok := <-readChan: // ... existing code ... + case pingErr := <-pingErrChan: + if pingErr != nil { + return fmt.Errorf("ping error: %w", pingErr) + } } } }These changes improve error handling for the ping goroutine and make the method more responsive to context cancellation.
To ensure that the
sendPingsfunction handles errors correctly, let's verify its implementation:🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 262-268: services/rfq/api/client/client.go#L262-L268
Added lines #L262 - L268 were not covered by tests
[warning] 271-294: services/rfq/api/client/client.go#L271-L294
Added lines #L271 - L294 were not covered by tests🪛 GitHub Check: Lint (services/rfq)
[failure] 273-273:
Error return value ofc.sendPingsis not checked (errcheck)services/rfq/api/rest/rfq.go (4)
159-175: Check for errors when parsingDestAmounttobig.IntingetBestQuote.The
SetStringmethod can fail to parse the string into abig.Int, returningfalseif the parsing fails. Ignoring this can lead to incorrect comparisons or runtime errors. Check the returnedokvalue and handle the error appropriately.Apply this diff to fix:
- aAmount, _ := new(big.Int).SetString(*a.DestAmount, 10) - bAmount, _ := new(big.Int).SetString(*b.DestAmount, 10) + aAmount, ok := new(big.Int).SetString(*a.DestAmount, 10) + if !ok { + logger.Errorf("Invalid DestAmount in quote a: %s", *a.DestAmount) + return b, true // Return b as the better quote + } + bAmount, ok := new(big.Int).SetString(*b.DestAmount, 10) + if !ok { + logger.Errorf("Invalid DestAmount in quote b: %s", *b.DestAmount) + return a, false // Keep a as the better quote + }🧰 Tools
🪛 GitHub Check: Lint (services/rfq)
[failure] 159-159:
getBestQuote - result 1 (bool) is never used (unparam)
189-197: Avoid side effects in the validation functionvalidateRelayerQuoteResponse.The function
validateRelayerQuoteResponseshould not modify the response object by settingresp.QuoteID. Validation functions are expected to perform checks without causing side effects. Assigning a newQuoteIDshould be done outside of this function.Apply this diff to fix:
func validateRelayerQuoteResponse(resp *model.WsRFQResponse) error { _, ok := new(big.Int).SetString(resp.DestAmount, 10) if !ok { return fmt.Errorf("dest amount is invalid") } - // TODO: compute quote ID from request - resp.QuoteID = uuid.New().String() return nil }Then, set
resp.QuoteIDafter validation in the calling function.
199-216: Ensure consistent error handling inrecordActiveQuote.In the
recordActiveQuotefunction, error handling is inconsistent. In some cases, errors are logged and the function continues execution; in other cases, errors are returned to the caller. This inconsistency can lead to confusion and makes error propagation unpredictable. Consider standardizing the error handling approach by consistently returning errors to the caller and handling them appropriately.
218-272: LGTM!The
handlePassiveRFQfunction looks good overall. It retrieves quotes from the database, validates the origin amount, and iterates through the quotes to find the best one based on the calculated destination amount after accounting for fixed fees. The function uses appropriate error handling and returns the best quote or an error to the caller.services/rfq/api/docs/docs.go (4)
124-155: LGTM!The new
/open_quote_requestsendpoint is well-documented, including the expected request and response formats. The description clearly explains the purpose of retrieving open quote requests in Received or Pending status.
335-363: LGTM!The
GetOpenQuoteRequestsResponseschema is well-defined, capturing all the necessary fields for an open quote request. The field names and types are clear and consistent.
444-465: LGTM!The
PutRFQResponseschema captures the essential fields for a quote response, including the success status, quote details, and error reason (if applicable). The field names and types are appropriate.
255-293: Verify the/rfqendpoint usage and response handling in the codebase.The new
/rfqendpoint for handling user quote requests is properly documented. However, ensure that:
- The server code correctly handles the
PutRFQRequestand returns aPutRFQResponse.- The client code (e.g., frontend or other services) is updated to use this new endpoint and process the response.
Run the following script to verify the endpoint usage:
services/rfq/api/rest/rfq.go
Outdated
| func (r *QuoterAPIServer) handleActiveRFQ(ctx context.Context, request *model.PutRFQRequest, requestID string) (quote *model.QuoteData) { | ||
| ctx, span := r.handler.Tracer().Start(ctx, "handleActiveRFQ", trace.WithAttributes( | ||
| attribute.String("user_address", request.UserAddress), | ||
| attribute.String("request_id", requestID), | ||
| )) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() | ||
|
|
||
| // publish the quote request to all connected clients | ||
| relayerReq := model.NewWsRFQRequest(request.Data, requestID) | ||
| r.wsClients.Range(func(relayerAddr string, client WsClient) bool { | ||
| sendCtx, sendSpan := r.handler.Tracer().Start(ctx, "sendQuoteRequest", trace.WithAttributes( | ||
| attribute.String("relayer_address", relayerAddr), | ||
| attribute.String("request_id", requestID), | ||
| )) | ||
| defer metrics.EndSpan(sendSpan) | ||
|
|
||
| subscribed := r.pubSubManager.IsSubscribed(relayerAddr, request.Data.OriginChainID, request.Data.DestChainID) | ||
| span.SetAttributes(attribute.Bool("subscribed", subscribed)) | ||
| if subscribed { | ||
| err := client.SendQuoteRequest(sendCtx, relayerReq) | ||
| if err != nil { | ||
| logger.Errorf("Error sending quote request to %s: %v", relayerAddr, err) | ||
| } | ||
| } | ||
| return true | ||
| }) | ||
| err := r.db.UpdateActiveQuoteRequestStatus(ctx, requestID, nil, db.Pending) | ||
| if err != nil { | ||
| logger.Errorf("Error updating active quote request status: %v", err) | ||
| } | ||
|
|
||
| // collect the responses and determine the best quote | ||
| responses := r.collectRelayerResponses(ctx, request, requestID) | ||
| for r, resp := range responses { | ||
| relayerAddr := r | ||
| quote, _ = getBestQuote(quote, getRelayerQuoteData(request, resp)) | ||
| quote.RelayerAddress = &relayerAddr | ||
| } | ||
| err = r.recordActiveQuote(ctx, quote, requestID) | ||
| if err != nil { | ||
| logger.Errorf("Error recording active quote: %v", err) | ||
| } | ||
|
|
||
| return quote | ||
| } |
There was a problem hiding this comment.
Ensure consistent error handling in handleActiveRFQ.
The handleActiveRFQ function handles errors inconsistently. In some cases, errors are logged and execution continues; in others, errors are ignored. This can lead to confusion and makes error propagation unpredictable.
Consider standardizing error handling by consistently returning errors to the caller and handling them appropriately. For example:
err := r.db.UpdateActiveQuoteRequestStatus(ctx, requestID, nil, db.Pending)
if err != nil {
return nil, fmt.Errorf("error updating active quote request status: %w", err)
}Apply this approach throughout the function for better error management.
Do you want me to generate a diff that standardizes error handling in this function?
| func (r *QuoterAPIServer) collectRelayerResponses(ctx context.Context, request *model.PutRFQRequest, requestID string) (responses map[string]*model.WsRFQResponse) { | ||
| ctx, span := r.handler.Tracer().Start(ctx, "collectRelayerResponses", trace.WithAttributes( | ||
| attribute.String("user_address", request.UserAddress), | ||
| attribute.String("request_id", requestID), | ||
| )) | ||
| defer metrics.EndSpan(span) | ||
|
|
||
| expireCtx, expireCancel := context.WithTimeout(ctx, time.Duration(request.Data.ExpirationWindow)*time.Millisecond) | ||
| defer expireCancel() | ||
|
|
||
| // don't cancel the collection context so that late responses can be collected in background | ||
| // nolint:govet | ||
| collectionCtx, _ := context.WithTimeout(ctx, time.Duration(request.Data.ExpirationWindow)*time.Millisecond+collectionTimeout) | ||
|
|
||
| wg := sync.WaitGroup{} | ||
| respMux := sync.Mutex{} | ||
| responses = map[string]*model.WsRFQResponse{} | ||
| r.wsClients.Range(func(relayerAddr string, client WsClient) bool { | ||
| wg.Add(1) | ||
| go func(client WsClient) { | ||
| var respStatus db.ActiveQuoteResponseStatus | ||
| var err error | ||
| _, clientSpan := r.handler.Tracer().Start(collectionCtx, "collectRelayerResponses", trace.WithAttributes( | ||
| attribute.String("relayer_address", relayerAddr), | ||
| attribute.String("request_id", requestID), | ||
| )) | ||
| defer func() { | ||
| clientSpan.SetAttributes(attribute.String("status", respStatus.String())) | ||
| metrics.EndSpanWithErr(clientSpan, err) | ||
| }() | ||
|
|
||
| defer wg.Done() | ||
| resp, err := client.ReceiveQuoteResponse(collectionCtx, requestID) | ||
| if err != nil { | ||
| logger.Errorf("Error receiving quote response: %v", err) | ||
| return | ||
| } | ||
| clientSpan.AddEvent("received quote response", trace.WithAttributes( | ||
| attribute.String("relayer_address", relayerAddr), | ||
| attribute.String("request_id", requestID), | ||
| attribute.String("dest_amount", resp.DestAmount), | ||
| )) | ||
|
|
||
| // validate the response | ||
| respStatus = getQuoteResponseStatus(expireCtx, resp) | ||
| if respStatus == db.Considered { | ||
| respMux.Lock() | ||
| responses[relayerAddr] = resp | ||
| respMux.Unlock() | ||
| } | ||
|
|
||
| // record the response | ||
| err = r.db.InsertActiveQuoteResponse(collectionCtx, resp, relayerAddr, respStatus) | ||
| if err != nil { | ||
| logger.Errorf("Error inserting active quote response: %v", err) | ||
| } | ||
| }(client) | ||
| return true | ||
| }) | ||
|
|
||
| // wait for all responses to be received, or expiration | ||
| select { | ||
| case <-expireCtx.Done(): | ||
| // request expired before all responses were received | ||
| case <-func() chan struct{} { | ||
| ch := make(chan struct{}) | ||
| go func() { | ||
| wg.Wait() | ||
| close(ch) | ||
| }() | ||
| return ch | ||
| }(): | ||
| // all responses received | ||
| } | ||
|
|
||
| return responses | ||
| } |
There was a problem hiding this comment.
Improve variable capture in goroutine.
When launching goroutines inside a loop, variables from the loop scope (like relayerAddr) may not be correctly captured due to how closures work in Go. To ensure that each goroutine uses the correct relayerAddr, pass it as a parameter to the closure.
Apply this diff to fix the issue:
r.wsClients.Range(func(relayerAddr string, client WsClient) bool {
wg.Add(1)
- go func(client WsClient) {
+ go func(relayerAddr string, client WsClient) {
// ...
- }(client)
+ }(relayerAddr, client)
return true
})📝 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 (r *QuoterAPIServer) collectRelayerResponses(ctx context.Context, request *model.PutRFQRequest, requestID string) (responses map[string]*model.WsRFQResponse) { | |
| ctx, span := r.handler.Tracer().Start(ctx, "collectRelayerResponses", trace.WithAttributes( | |
| attribute.String("user_address", request.UserAddress), | |
| attribute.String("request_id", requestID), | |
| )) | |
| defer metrics.EndSpan(span) | |
| expireCtx, expireCancel := context.WithTimeout(ctx, time.Duration(request.Data.ExpirationWindow)*time.Millisecond) | |
| defer expireCancel() | |
| // don't cancel the collection context so that late responses can be collected in background | |
| // nolint:govet | |
| collectionCtx, _ := context.WithTimeout(ctx, time.Duration(request.Data.ExpirationWindow)*time.Millisecond+collectionTimeout) | |
| wg := sync.WaitGroup{} | |
| respMux := sync.Mutex{} | |
| responses = map[string]*model.WsRFQResponse{} | |
| r.wsClients.Range(func(relayerAddr string, client WsClient) bool { | |
| wg.Add(1) | |
| go func(client WsClient) { | |
| var respStatus db.ActiveQuoteResponseStatus | |
| var err error | |
| _, clientSpan := r.handler.Tracer().Start(collectionCtx, "collectRelayerResponses", trace.WithAttributes( | |
| attribute.String("relayer_address", relayerAddr), | |
| attribute.String("request_id", requestID), | |
| )) | |
| defer func() { | |
| clientSpan.SetAttributes(attribute.String("status", respStatus.String())) | |
| metrics.EndSpanWithErr(clientSpan, err) | |
| }() | |
| defer wg.Done() | |
| resp, err := client.ReceiveQuoteResponse(collectionCtx, requestID) | |
| if err != nil { | |
| logger.Errorf("Error receiving quote response: %v", err) | |
| return | |
| } | |
| clientSpan.AddEvent("received quote response", trace.WithAttributes( | |
| attribute.String("relayer_address", relayerAddr), | |
| attribute.String("request_id", requestID), | |
| attribute.String("dest_amount", resp.DestAmount), | |
| )) | |
| // validate the response | |
| respStatus = getQuoteResponseStatus(expireCtx, resp) | |
| if respStatus == db.Considered { | |
| respMux.Lock() | |
| responses[relayerAddr] = resp | |
| respMux.Unlock() | |
| } | |
| // record the response | |
| err = r.db.InsertActiveQuoteResponse(collectionCtx, resp, relayerAddr, respStatus) | |
| if err != nil { | |
| logger.Errorf("Error inserting active quote response: %v", err) | |
| } | |
| }(client) | |
| return true | |
| }) | |
| // wait for all responses to be received, or expiration | |
| select { | |
| case <-expireCtx.Done(): | |
| // request expired before all responses were received | |
| case <-func() chan struct{} { | |
| ch := make(chan struct{}) | |
| go func() { | |
| wg.Wait() | |
| close(ch) | |
| }() | |
| return ch | |
| }(): | |
| // all responses received | |
| } | |
| return responses | |
| } | |
| func (r *QuoterAPIServer) collectRelayerResponses(ctx context.Context, request *model.PutRFQRequest, requestID string) (responses map[string]*model.WsRFQResponse) { | |
| ctx, span := r.handler.Tracer().Start(ctx, "collectRelayerResponses", trace.WithAttributes( | |
| attribute.String("user_address", request.UserAddress), | |
| attribute.String("request_id", requestID), | |
| )) | |
| defer metrics.EndSpan(span) | |
| expireCtx, expireCancel := context.WithTimeout(ctx, time.Duration(request.Data.ExpirationWindow)*time.Millisecond) | |
| defer expireCancel() | |
| // don't cancel the collection context so that late responses can be collected in background | |
| // nolint:govet | |
| collectionCtx, _ := context.WithTimeout(ctx, time.Duration(request.Data.ExpirationWindow)*time.Millisecond+collectionTimeout) | |
| wg := sync.WaitGroup{} | |
| respMux := sync.Mutex{} | |
| responses = map[string]*model.WsRFQResponse{} | |
| r.wsClients.Range(func(relayerAddr string, client WsClient) bool { | |
| wg.Add(1) | |
| go func(relayerAddr string, client WsClient) { | |
| var respStatus db.ActiveQuoteResponseStatus | |
| var err error | |
| _, clientSpan := r.handler.Tracer().Start(collectionCtx, "collectRelayerResponses", trace.WithAttributes( | |
| attribute.String("relayer_address", relayerAddr), | |
| attribute.String("request_id", requestID), | |
| )) | |
| defer func() { | |
| clientSpan.SetAttributes(attribute.String("status", respStatus.String())) | |
| metrics.EndSpanWithErr(clientSpan, err) | |
| }() | |
| defer wg.Done() | |
| resp, err := client.ReceiveQuoteResponse(collectionCtx, requestID) | |
| if err != nil { | |
| logger.Errorf("Error receiving quote response: %v", err) | |
| return | |
| } | |
| clientSpan.AddEvent("received quote response", trace.WithAttributes( | |
| attribute.String("relayer_address", relayerAddr), | |
| attribute.String("request_id", requestID), | |
| attribute.String("dest_amount", resp.DestAmount), | |
| )) | |
| // validate the response | |
| respStatus = getQuoteResponseStatus(expireCtx, resp) | |
| if respStatus == db.Considered { | |
| respMux.Lock() | |
| responses[relayerAddr] = resp | |
| respMux.Unlock() | |
| } | |
| // record the response | |
| err = r.db.InsertActiveQuoteResponse(collectionCtx, resp, relayerAddr, respStatus) | |
| if err != nil { | |
| logger.Errorf("Error inserting active quote response: %v", err) | |
| } | |
| }(relayerAddr, client) | |
| return true | |
| }) | |
| // wait for all responses to be received, or expiration | |
| select { | |
| case <-expireCtx.Done(): | |
| // request expired before all responses were received | |
| case <-func() chan struct{} { | |
| ch := make(chan struct{}) | |
| go func() { | |
| wg.Wait() | |
| close(ch) | |
| }() | |
| return ch | |
| }(): | |
| // all responses received | |
| } | |
| return responses | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
services/rfq/api/rest/rfq.go (3)
76-81: Add comments to explain context handling.The logic for handling different contexts (expiration and collection) is complex and could benefit from explanatory comments. Consider adding comments to clarify the purpose and behavior of each context:
+// expireCtx is used to determine when the quote request has expired expireCtx, expireCancel := context.WithTimeout(ctx, time.Duration(request.Data.ExpirationWindow)*time.Millisecond) defer expireCancel() -// don't cancel the collection context so that late responses can be collected in background +// collectionCtx allows for collection of late responses after the expiration window +// This ensures that we can still process responses that arrive slightly after the deadline // nolint:govet collectionCtx, _ := context.WithTimeout(ctx, time.Duration(request.Data.ExpirationWindow)*time.Millisecond+collectionTimeout)
194-195: Address TODO comment for QuoteID computation.The TODO comment suggests that the QuoteID should be computed from the request. Consider implementing this logic or creating a separate ticket to track this enhancement.
1-272: Summary of review findings for rfq.goOverall, the implementation of the RFQ functionality is well-structured and comprehensive. However, there are several areas for improvement:
- Address potential race conditions in goroutines by properly passing variables to closures.
- Enhance error handling, especially for database operations and big.Int parsing.
- Refactor longer functions (e.g.,
handlePassiveRFQ) into smaller, more focused functions for improved readability and maintainability.- Add explanatory comments for complex logic, particularly around context handling.
- Consider extracting magic numbers into named constants.
- Review and implement TODOs, such as the QuoteID computation logic.
Addressing these points will significantly improve the code's robustness, maintainability, and performance.
services/rfq/api/rest/server.go (5)
56-56: LGTM: New fields added for WebSocket and pub/sub functionality.The new fields
upgrader,wsClients, andpubSubManagerare appropriate for managing WebSocket connections and pub/sub functionality. The use ofxsync.MapOfforwsClientsensures thread-safe operations.Consider adding comments to explain the purpose of each new field, especially
pubSubManager, to improve code readability.Also applies to: 67-71
Line range hint
196-227: New routes added for RFQ and WebSocket, but security improvement needed.The new routes for RFQ and WebSocket are properly added and consistent with the new functionality. However, there's a security concern with the WebSocket upgrader initialization.
The
CheckOriginfunction in the WebSocket upgrader is set to always return true, which poses a security risk. This allows connections from any origin, potentially exposing your application to cross-site WebSocket hijacking attacks.Implement a proper origin check. For example:
r.upgrader = websocket.Upgrader{ CheckOrigin: func(r *http.Request) bool { origin := r.Header.Get("Origin") // Check if the origin is in the whitelist return origin == "https://trusted-origin.com" }, }Replace "https://trusted-origin.com" with your actual allowed origins.
Line range hint
244-287: LGTM: Authentication updated for new RFQ routes.The AuthMiddleware has been properly updated to handle the new RFQ routes. The parsing of the ChainsHeader allows for multiple chain IDs to be specified, which is consistent with the new functionality.
Consider extracting the chain ID parsing logic into a separate function to improve readability and maintainability. For example:
func parseChainIDs(chainsHeader string) ([]uint32, error) { var chainIDs []int err := json.Unmarshal([]byte(chainsHeader), &chainIDs) if err != nil { return nil, err } destChainIDs := make([]uint32, len(chainIDs)) for i, chainID := range chainIDs { destChainIDs[i] = uint32(chainID) } return destChainIDs, nil }
426-483: LGTM: WebSocket handling implemented correctly.The
GetActiveRFQWebsocketmethod properly handles WebSocket connections, including upgrading the connection, managing WebSocket clients, and implementing cleanup. The check to ensure only one connection per relayer is a good practice.Consider adding error logging for the case when a relayer attempts to connect more than once. This will help with debugging and monitoring. For example:
if ok { logger.Warn("Relayer attempted to connect more than once", "relayer_address", relayerAddr) c.JSON(http.StatusBadRequest, gin.H{"error": "relayer already connected"}) return }
485-577: LGTM: RFQ handling implemented comprehensively.The
PutRFQRequestmethod properly handles user quote requests, including both active and passive RFQ types. It uses tracing for performance monitoring and implements logic for choosing the best quote.Consider extracting the quote type checking logic into a separate function to improve readability. For example:
func isActiveRFQ(quoteTypes []string) bool { for _, quoteType := range quoteTypes { if quoteType == quoteTypeActive { return true } } return false }Then you can simplify the main function:
isActiveRFQ := isActiveRFQ(req.QuoteTypes) span.SetAttributes(attribute.Bool("is_active_rfq", isActiveRFQ)) var activeQuote *model.QuoteData if isActiveRFQ { activeQuote = r.handleActiveRFQ(ctx, &req, requestID) // ... rest of the code }This change would make the main function more concise and easier to read.
services/rfq/api/client/client.go (1)
80-84: Add unit tests to cover error handling when obtaining the auth headerCurrently, when
getAuthHeaderfails inOnBeforeRequest, the error is returned but not covered by unit tests. Consider adding tests to cover this scenario to improve code coverage and ensure that error handling works as expected.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-82: services/rfq/api/client/client.go#L82
Added line #L82 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- services/rfq/api/client/client.go (8 hunks)
- services/rfq/api/rest/rfq.go (1 hunks)
- services/rfq/api/rest/server.go (12 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/api/client/client.go
[warning] 82-82: services/rfq/api/client/client.go#L82
Added line #L82 was not covered by tests
[warning] 108-109: services/rfq/api/client/client.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 181-185: services/rfq/api/client/client.go#L181-L185
Added lines #L181 - L185 were not covered by tests
[warning] 188-201: services/rfq/api/client/client.go#L188-L201
Added lines #L188 - L201 were not covered by tests
[warning] 204-211: services/rfq/api/client/client.go#L204-L211
Added lines #L204 - L211 were not covered by tests
[warning] 213-218: services/rfq/api/client/client.go#L213-L218
Added lines #L213 - L218 were not covered by tests
[warning] 221-221: services/rfq/api/client/client.go#L221
Added line #L221 was not covered by tests
[warning] 224-227: services/rfq/api/client/client.go#L224-L227
Added lines #L224 - L227 were not covered by tests
[warning] 229-232: services/rfq/api/client/client.go#L229-L232
Added lines #L229 - L232 were not covered by tests
[warning] 234-242: services/rfq/api/client/client.go#L234-L242
Added lines #L234 - L242 were not covered by tests
[warning] 244-244: services/rfq/api/client/client.go#L244
Added line #L244 was not covered by tests
[warning] 247-259: services/rfq/api/client/client.go#L247-L259
Added lines #L247 - L259 were not covered by tests
[warning] 262-268: services/rfq/api/client/client.go#L262-L268
Added lines #L262 - L268 were not covered by tests
[warning] 271-294: services/rfq/api/client/client.go#L271-L294
Added lines #L271 - L294 were not covered by tests
[warning] 300-310: services/rfq/api/client/client.go#L300-L310
Added lines #L300 - L310 were not covered by tests
[warning] 315-323: services/rfq/api/client/client.go#L315-L323
Added lines #L315 - L323 were not covered by tests
[warning] 325-328: services/rfq/api/client/client.go#L325-L328
Added lines #L325 - L328 were not covered by tests
[warning] 333-338: services/rfq/api/client/client.go#L333-L338
Added lines #L333 - L338 were not covered by tests
[warning] 340-343: services/rfq/api/client/client.go#L340-L343
Added lines #L340 - L343 were not covered by tests
[warning] 345-345: services/rfq/api/client/client.go#L345
Added line #L345 was not covered by tests
[warning] 433-441: services/rfq/api/client/client.go#L433-L441
Added lines #L433 - L441 were not covered by tests
[warning] 443-445: services/rfq/api/client/client.go#L443-L445
Added lines #L443 - L445 were not covered by tests
[warning] 447-447: services/rfq/api/client/client.go#L447
Added line #L447 was not covered by tests
🔇 Additional comments (5)
services/rfq/api/rest/rfq.go (3)
147-157: LGTM: Clean and straightforward implementation.The
getRelayerQuoteDatafunction correctly maps the request and response data to aQuoteDatastruct. The implementation is clean and easy to understand.
177-187: LGTM: Well-structured status determination.The
getQuoteResponseStatusfunction effectively determines the status of a quote response. It appropriately handles validation errors and checks for request expiration using context cancellation.
32-48:⚠️ Potential issueFix potential race condition in goroutine.
The
relayerAddrvariable is captured by the goroutine and may lead to a race condition. Pass it as a parameter to the closure to ensure each goroutine uses the correct value.r.wsClients.Range(func(relayerAddr string, client WsClient) bool { - go func(client WsClient) { + go func(relayerAddr string, client WsClient) { // ... existing code ... - }(client) + }(relayerAddr, client) return true })Likely invalid or redundant comment.
services/rfq/api/rest/server.go (2)
6-6: LGTM: New imports and constants added for WebSocket and RFQ functionality.The new imports and constants are appropriate for the added WebSocket and RFQ functionality. They provide necessary tools for JSON handling, UUID generation, concurrent map operations, and WebSocket communication.
Also applies to: 13-15, 21-21, 27-27, 171-178
143-144: LGTM: Proper initialization of new WebSocket and pub/sub fields.The initialization of
wsClientsandpubSubManageris consistent with the new fields added to the QuoterAPIServer struct. The use ofxsync.NewMapOf[WsClient]()ensures thread-safe operations for WebSocket clients.
| func (r *QuoterAPIServer) handlePassiveRFQ(ctx context.Context, request *model.PutRFQRequest) (*model.QuoteData, error) { | ||
| ctx, span := r.handler.Tracer().Start(ctx, "handlePassiveRFQ", trace.WithAttributes( | ||
| attribute.String("user_address", request.UserAddress), | ||
| )) | ||
| defer metrics.EndSpan(span) | ||
|
|
||
| quotes, err := r.db.GetQuotesByOriginAndDestination(ctx, uint64(request.Data.OriginChainID), request.Data.OriginTokenAddr, uint64(request.Data.DestChainID), request.Data.DestTokenAddr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get quotes: %w", err) | ||
| } | ||
|
|
||
| originAmount, ok := new(big.Int).SetString(request.Data.OriginAmount, 10) | ||
| if !ok { | ||
| return nil, errors.New("invalid origin amount") | ||
| } | ||
|
|
||
| var bestQuote *model.QuoteData | ||
| for _, quote := range quotes { | ||
| quoteOriginAmount, ok := new(big.Int).SetString(quote.MaxOriginAmount.String(), 10) | ||
| if !ok { | ||
| continue | ||
| } | ||
| if quoteOriginAmount.Cmp(originAmount) < 0 { | ||
| continue | ||
| } | ||
| quotePrice := new(big.Float).Quo( | ||
| new(big.Float).SetInt(quote.DestAmount.BigInt()), | ||
| new(big.Float).SetInt(quote.MaxOriginAmount.BigInt()), | ||
| ) | ||
|
|
||
| rawDestAmount := new(big.Float).Mul( | ||
| new(big.Float).SetInt(originAmount), | ||
| quotePrice, | ||
| ) | ||
|
|
||
| rawDestAmountInt, _ := rawDestAmount.Int(nil) | ||
| if rawDestAmountInt.Cmp(quote.FixedFee.BigInt()) < 0 { | ||
| continue | ||
| } | ||
| destAmount := new(big.Int).Sub(rawDestAmountInt, quote.FixedFee.BigInt()).String() | ||
| //nolint:gosec | ||
| quoteData := &model.QuoteData{ | ||
| OriginChainID: int(quote.OriginChainID), | ||
| DestChainID: int(quote.DestChainID), | ||
| OriginTokenAddr: quote.OriginTokenAddr, | ||
| DestTokenAddr: quote.DestTokenAddr, | ||
| OriginAmount: quote.MaxOriginAmount.String(), | ||
| DestAmount: &destAmount, | ||
| RelayerAddress: "e.RelayerAddr, | ||
| } | ||
| bestQuote = getBestQuote(bestQuote, quoteData) | ||
| } | ||
|
|
||
| return bestQuote, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor handlePassiveRFQ for improved readability and error handling.
The function is well-structured but quite long. Consider breaking it down into smaller, more focused functions. Also, improve error handling for big.Int operations and use constants for magic numbers. Here are some suggestions:
- Extract the quote filtering and calculation logic into a separate function.
- Use constants for the base in SetString operations.
- Improve error handling for big.Int operations.
Example refactoring:
const (
bigIntBase = 10
)
func (r *QuoterAPIServer) handlePassiveRFQ(ctx context.Context, request *model.PutRFQRequest) (*model.QuoteData, error) {
// ... existing code ...
return r.findBestPassiveQuote(quotes, originAmount)
}
func (r *QuoterAPIServer) findBestPassiveQuote(quotes []db.Quote, originAmount *big.Int) (*model.QuoteData, error) {
var bestQuote *model.QuoteData
for _, quote := range quotes {
quoteData, err := r.calculateQuoteData(quote, originAmount)
if err != nil {
logger.Errorf("Error calculating quote data: %v", err)
continue
}
bestQuote = getBestQuote(bestQuote, quoteData)
}
return bestQuote, nil
}
func (r *QuoterAPIServer) calculateQuoteData(quote db.Quote, originAmount *big.Int) (*model.QuoteData, error) {
// ... implement calculation logic here ...
}This refactoring improves readability and makes the code easier to maintain and test.
| // TODO: compute quote ID from request | ||
| resp.QuoteID = uuid.New().String() |
There was a problem hiding this comment.
Remove QuoteID generation from validation function.
Generating a new QuoteID in a validation function is a side effect that might be unexpected. Consider moving this logic to a separate function or to the caller of validateRelayerQuoteResponse.
-// TODO: compute quote ID from request
-resp.QuoteID = uuid.New().String()📝 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.
| // TODO: compute quote ID from request | |
| resp.QuoteID = uuid.New().String() | |
| // This space intentionally left blank |
| err := r.db.UpdateActiveQuoteRequestStatus(ctx, requestID, nil, db.Pending) | ||
| if err != nil { | ||
| logger.Errorf("Error updating active quote request status: %v", err) | ||
| } |
There was a problem hiding this comment.
Improve error handling for database update.
The error from UpdateActiveQuoteRequestStatus is only logged. Consider returning this error to the caller or implementing a retry mechanism for database operations.
err := r.db.UpdateActiveQuoteRequestStatus(ctx, requestID, nil, db.Pending)
if err != nil {
- logger.Errorf("Error updating active quote request status: %v", err)
+ return nil, fmt.Errorf("failed to update active quote request status: %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.
| err := r.db.UpdateActiveQuoteRequestStatus(ctx, requestID, nil, db.Pending) | |
| if err != nil { | |
| logger.Errorf("Error updating active quote request status: %v", err) | |
| } | |
| err := r.db.UpdateActiveQuoteRequestStatus(ctx, requestID, nil, db.Pending) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to update active quote request status: %w", err) | |
| } |
| err = r.recordActiveQuote(ctx, quote, requestID) | ||
| if err != nil { | ||
| logger.Errorf("Error recording active quote: %v", err) | ||
| } |
There was a problem hiding this comment.
Improve error handling for recordActiveQuote.
The error from recordActiveQuote is only logged. Consider returning this error to the caller or implementing a retry mechanism.
err = r.recordActiveQuote(ctx, quote, requestID)
if err != nil {
- logger.Errorf("Error recording active quote: %v", err)
+ return nil, fmt.Errorf("failed to record active quote: %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.
| err = r.recordActiveQuote(ctx, quote, requestID) | |
| if err != nil { | |
| logger.Errorf("Error recording active quote: %v", err) | |
| } | |
| err = r.recordActiveQuote(ctx, quote, requestID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to record active quote: %w", err) | |
| } |
| func getBestQuote(a, b *model.QuoteData) *model.QuoteData { | ||
| if a == nil && b == nil { | ||
| return nil | ||
| } | ||
| if a == nil { | ||
| return b | ||
| } | ||
| if b == nil { | ||
| return a | ||
| } | ||
| aAmount, _ := new(big.Int).SetString(*a.DestAmount, 10) | ||
| bAmount, _ := new(big.Int).SetString(*b.DestAmount, 10) | ||
| if aAmount.Cmp(bAmount) > 0 { | ||
| return a | ||
| } | ||
| return b | ||
| } |
There was a problem hiding this comment.
Improve error handling for big.Int parsing.
The function doesn't check if the SetString operation succeeds when parsing DestAmount. This could lead to unexpected behavior if the strings are invalid. Consider adding error checking:
-aAmount, _ := new(big.Int).SetString(*a.DestAmount, 10)
-bAmount, _ := new(big.Int).SetString(*b.DestAmount, 10)
+aAmount, ok := new(big.Int).SetString(*a.DestAmount, 10)
+if !ok {
+ logger.Errorf("Invalid DestAmount in quote a: %s", *a.DestAmount)
+ return b
+}
+bAmount, ok := new(big.Int).SetString(*b.DestAmount, 10)
+if !ok {
+ logger.Errorf("Invalid DestAmount in quote b: %s", *b.DestAmount)
+ return a
+}📝 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 getBestQuote(a, b *model.QuoteData) *model.QuoteData { | |
| if a == nil && b == nil { | |
| return nil | |
| } | |
| if a == nil { | |
| return b | |
| } | |
| if b == nil { | |
| return a | |
| } | |
| aAmount, _ := new(big.Int).SetString(*a.DestAmount, 10) | |
| bAmount, _ := new(big.Int).SetString(*b.DestAmount, 10) | |
| if aAmount.Cmp(bAmount) > 0 { | |
| return a | |
| } | |
| return b | |
| } | |
| func getBestQuote(a, b *model.QuoteData) *model.QuoteData { | |
| if a == nil && b == nil { | |
| return nil | |
| } | |
| if a == nil { | |
| return b | |
| } | |
| if b == nil { | |
| return a | |
| } | |
| aAmount, ok := new(big.Int).SetString(*a.DestAmount, 10) | |
| if !ok { | |
| logger.Errorf("Invalid DestAmount in quote a: %s", *a.DestAmount) | |
| return b | |
| } | |
| bAmount, ok := new(big.Int).SetString(*b.DestAmount, 10) | |
| if !ok { | |
| logger.Errorf("Invalid DestAmount in quote b: %s", *b.DestAmount) | |
| return a | |
| } | |
| if aAmount.Cmp(bAmount) > 0 { | |
| return a | |
| } | |
| return b | |
| } |
| go func(client WsClient) { | ||
| var respStatus db.ActiveQuoteResponseStatus | ||
| var err error | ||
| _, clientSpan := r.handler.Tracer().Start(collectionCtx, "collectRelayerResponses", trace.WithAttributes( | ||
| attribute.String("relayer_address", relayerAddr), | ||
| attribute.String("request_id", requestID), | ||
| )) | ||
| defer func() { | ||
| clientSpan.SetAttributes(attribute.String("status", respStatus.String())) | ||
| metrics.EndSpanWithErr(clientSpan, err) | ||
| }() | ||
|
|
||
| defer wg.Done() | ||
| resp, err := client.ReceiveQuoteResponse(collectionCtx, requestID) | ||
| if err != nil { | ||
| logger.Errorf("Error receiving quote response: %v", err) | ||
| return | ||
| } | ||
| clientSpan.AddEvent("received quote response", trace.WithAttributes( | ||
| attribute.String("relayer_address", relayerAddr), | ||
| attribute.String("request_id", requestID), | ||
| attribute.String("dest_amount", resp.DestAmount), | ||
| )) | ||
|
|
||
| // validate the response | ||
| respStatus = getQuoteResponseStatus(expireCtx, resp) | ||
| if respStatus == db.Considered { | ||
| respMux.Lock() | ||
| responses[relayerAddr] = resp | ||
| respMux.Unlock() | ||
| } | ||
|
|
||
| // record the response | ||
| err = r.db.InsertActiveQuoteResponse(collectionCtx, resp, relayerAddr, respStatus) | ||
| if err != nil { | ||
| logger.Errorf("Error inserting active quote response: %v", err) | ||
| } | ||
| }(client) |
There was a problem hiding this comment.
Fix potential race condition in goroutine and improve error handling.
The relayerAddr variable is captured by the goroutine and may lead to a race condition. Also, the error from InsertActiveQuoteResponse is only logged. Consider the following improvements:
- go func(client WsClient) {
+ go func(relayerAddr string, client WsClient) {
// ... existing code ...
err = r.db.InsertActiveQuoteResponse(collectionCtx, resp, relayerAddr, respStatus)
if err != nil {
- logger.Errorf("Error inserting active quote response: %v", err)
+ logger.Errorf("Error inserting active quote response for relayer %s: %v", relayerAddr, err)
+ // Consider implementing a retry mechanism or a way to handle this error more gracefully
}
- }(client)
+ }(relayerAddr, client)📝 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.
| go func(client WsClient) { | |
| var respStatus db.ActiveQuoteResponseStatus | |
| var err error | |
| _, clientSpan := r.handler.Tracer().Start(collectionCtx, "collectRelayerResponses", trace.WithAttributes( | |
| attribute.String("relayer_address", relayerAddr), | |
| attribute.String("request_id", requestID), | |
| )) | |
| defer func() { | |
| clientSpan.SetAttributes(attribute.String("status", respStatus.String())) | |
| metrics.EndSpanWithErr(clientSpan, err) | |
| }() | |
| defer wg.Done() | |
| resp, err := client.ReceiveQuoteResponse(collectionCtx, requestID) | |
| if err != nil { | |
| logger.Errorf("Error receiving quote response: %v", err) | |
| return | |
| } | |
| clientSpan.AddEvent("received quote response", trace.WithAttributes( | |
| attribute.String("relayer_address", relayerAddr), | |
| attribute.String("request_id", requestID), | |
| attribute.String("dest_amount", resp.DestAmount), | |
| )) | |
| // validate the response | |
| respStatus = getQuoteResponseStatus(expireCtx, resp) | |
| if respStatus == db.Considered { | |
| respMux.Lock() | |
| responses[relayerAddr] = resp | |
| respMux.Unlock() | |
| } | |
| // record the response | |
| err = r.db.InsertActiveQuoteResponse(collectionCtx, resp, relayerAddr, respStatus) | |
| if err != nil { | |
| logger.Errorf("Error inserting active quote response: %v", err) | |
| } | |
| }(client) | |
| go func(relayerAddr string, client WsClient) { | |
| var respStatus db.ActiveQuoteResponseStatus | |
| var err error | |
| _, clientSpan := r.handler.Tracer().Start(collectionCtx, "collectRelayerResponses", trace.WithAttributes( | |
| attribute.String("relayer_address", relayerAddr), | |
| attribute.String("request_id", requestID), | |
| )) | |
| defer func() { | |
| clientSpan.SetAttributes(attribute.String("status", respStatus.String())) | |
| metrics.EndSpanWithErr(clientSpan, err) | |
| }() | |
| defer wg.Done() | |
| resp, err := client.ReceiveQuoteResponse(collectionCtx, requestID) | |
| if err != nil { | |
| logger.Errorf("Error receiving quote response: %v", err) | |
| return | |
| } | |
| clientSpan.AddEvent("received quote response", trace.WithAttributes( | |
| attribute.String("relayer_address", relayerAddr), | |
| attribute.String("request_id", requestID), | |
| attribute.String("dest_amount", resp.DestAmount), | |
| )) | |
| // validate the response | |
| respStatus = getQuoteResponseStatus(expireCtx, resp) | |
| if respStatus == db.Considered { | |
| respMux.Lock() | |
| responses[relayerAddr] = resp | |
| respMux.Unlock() | |
| } | |
| // record the response | |
| err = r.db.InsertActiveQuoteResponse(collectionCtx, resp, relayerAddr, respStatus) | |
| if err != nil { | |
| logger.Errorf("Error inserting active quote response for relayer %s: %v", relayerAddr, err) | |
| // Consider implementing a retry mechanism or a way to handle this error more gracefully | |
| } | |
| }(relayerAddr, client) |
services/rfq/api/client/client.go
Outdated
| if err != nil { | ||
| logger.Error("Error running websocket listener: %s", err) | ||
| } |
There was a problem hiding this comment.
Use logger.Errorf instead of logger.Error for formatted error messages
At line 217, you're using logger.Error("Error running websocket listener: %s", err). The logger.Error method may not support formatting verbs like %s. Consider using logger.Errorf to properly format the error message.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 213-218: services/rfq/api/client/client.go#L213-L218
Added lines #L213 - L218 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (15)
services/rfq/api/client/client.go (5)
32-33: Consider making pingPeriod configurableThe
pingPeriodconstant is a good addition for maintaining WebSocket connections. However, different environments or use cases might benefit from different ping intervals.Consider making
pingPerioda configurable parameter that can be set when creating the client. This would allow for more flexibility in different deployment scenarios.
64-65: Review necessity of rClient field in clientImplThe addition of the
reqSignerfield is appropriate for handling message signing in WebSocket communications.However, there might be redundancy with the
rClientfield. SinceclientImplembedsUnauthenticatedClient, which already has anrClientfield, consider removing therClientfield fromclientImplto avoid potential confusion or unintended shadowing. If a separaterClientinstance is necessary for authenticated requests, consider renaming it to clarify its purpose.
80-84: Improve error message in NewAuthenticatedClientThe addition of the
getAuthHeaderfunction and error handling for the authorization header is a good improvement.Consider making the error message more descriptive to aid in debugging. For example:
-return fmt.Errorf("failed to get auth header: %w", err) +return fmt.Errorf("failed to generate authorization header for authenticated client: %w", err)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-82: services/rfq/api/client/client.go#L82
Added line #L82 was not covered by tests
95-112: Approve getAuthHeader implementation with minor suggestionThe
getAuthHeaderfunction is well-implemented, using a combination of timestamp and signed message for authentication. This approach helps prevent replay attacks and provides a secure method of authentication.Consider wrapping the error returned from
reqSigner.SignMessageto provide more context:if err != nil { - return "", fmt.Errorf("failed to sign request: %w", err) + return "", fmt.Errorf("failed to sign authentication message: %w", err) }This change would make it easier to trace the source of the error if it occurs.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-109: services/rfq/api/client/client.go#L108-L109
Added lines #L108 - L109 were not covered by tests
305-320: Approve sendPings implementation with minor suggestionThe
sendPingsmethod is well-implemented. It correctly sends periodic ping messages to keep the WebSocket connection alive.Key points:
- Uses a ticker for regular intervals, which is efficient.
- Properly respects context cancellation.
- Consistently uses the
pingPeriodconstant.Consider adding error handling for the case where sending a ping message fails:
for { select { case <-pingTicker.C: pingMsg := model.ActiveRFQMessage{ Op: rest.PingOp, } - reqChan <- &pingMsg + select { + case reqChan <- &pingMsg: + case <-ctx.Done(): + return + default: + logger.Warn("Failed to send ping message: channel full") + } case <-ctx.Done(): return } }This change ensures that the method doesn't block indefinitely if the
reqChanis full and provides logging for debugging purposes.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 305-317: services/rfq/api/client/client.go#L305-L317
Added lines #L305 - L317 were not covered by testsservices/rfq/api/rest/server.go (6)
6-6: LGTM! Consider using a more descriptive prefix for the new constants.The new imports and constants are appropriate for implementing WebSocket functionality and handling RFQ requests. However, to improve code readability and maintainability, consider using a more descriptive prefix for the new constants, such as
RFQ_orWS_.For example:
const ( RFQ_STREAM_ROUTE = "/rfq_stream" RFQ_ROUTE = "/rfq" RFQ_CHAINS_HEADER = "Chains" RFQ_AUTHORIZATION_HEADER = "Authorization" )This naming convention would make it clearer that these constants are specifically related to the RFQ functionality.
Also applies to: 13-15, 21-21, 27-27, 171-178
56-56: LGTM! Consider adding documentation for the new fields.The new fields
upgrader,wsClients, andpubSubManagerare appropriate additions for implementing WebSocket functionality. To improve code readability and maintainability, consider adding documentation comments for these new fields explaining their purpose and usage.For example:
type QuoterAPIServer struct { // ... existing fields ... // upgrader is used for upgrading HTTP connections to WebSocket connections upgrader websocket.Upgrader // wsClients maintains a mapping of connection ID to WebSocket clients wsClients *xsync.MapOf[string, WsClient] // pubSubManager handles publish-subscribe operations for WebSocket communication pubSubManager PubSubManager }This documentation will help other developers understand the purpose of these fields at a glance.
Also applies to: 67-71
Line range hint
196-227: LGTM for new routes, but security concern in WebSocket upgrader configuration.The addition of new routes for RFQ and WebSocket functionality is well-implemented. However, there's a security concern in the WebSocket upgrader configuration:
r.upgrader = websocket.Upgrader{ CheckOrigin: func(_ *http.Request) bool { return true // TODO: Implement a more secure check }, }The
CheckOriginfunction is currently set to always return true, which poses a security risk. This allows connections from any origin, potentially exposing your application to cross-site WebSocket hijacking attacks.Implement a proper origin check. For example:
r.upgrader = websocket.Upgrader{ CheckOrigin: func(r *http.Request) bool { origin := r.Header.Get("Origin") // Check if the origin is in the whitelist return origin == "https://trusted-origin.com" }, }Replace "https://trusted-origin.com" with your actual allowed origins.
Line range hint
244-289: LGTM! Consider removing debug print statements.The changes to the
AuthMiddlewarefunction, including the new case forRFQRouteandRFQStreamRoute, and the parsing of theChainsHeader, are well-implemented and consistent with the new RFQ functionality.However, there are debug print statements that should be removed before deploying to production:
fmt.Println("AuthMiddleware - RFQRoute or RFQStreamRoute") fmt.Printf("AuthMiddleware - chainsHeader: %s\n", chainsHeader)Consider replacing these with proper logging statements using the project's logging framework, or remove them entirely if they're no longer needed.
428-491: LGTM! Remove debug prints and consider using constants for error messages.The
GetActiveRFQWebsocketfunction is well-implemented, handling the WebSocket lifecycle appropriately with proper error handling, client registration, and cleanup.However, there are a few improvements to consider:
Remove debug print statements or replace them with proper logging:
fmt.Println("GetActiveRFQWebsocket") fmt.Println("GetActiveRFQWebsocket - upgrading websocket") // ... other similar statementsConsider using constants for error messages to improve maintainability:
const ( ErrNoRelayerAddress = "No relayer address recovered from signature" ErrInvalidRelayerAddressType = "Invalid relayer address type" ErrRelayerAlreadyConnected = "relayer already connected" )Then use these constants in your error responses:
c.JSON(http.StatusBadRequest, gin.H{"error": ErrNoRelayerAddress})These changes will improve code quality and maintainability.
498-585: LGTM! Consider improving error handling for passive quote requests.The
PutRFQRequestfunction is well-implemented, handling both active and passive quote requests with proper error handling, tracing, and response construction. The logic for selecting the best quote is correct.One minor suggestion for improvement:
In the passive quote handling, consider adding more detailed error information to the response when an error occurs:
passiveQuote, err := r.handlePassiveRFQ(ctx, &req) if err != nil { logger.Error("Error handling passive RFQ", "error", err) // Consider adding this error information to the response resp = model.PutRFQResponse{ Success: false, Reason: fmt.Sprintf("Error handling passive RFQ: %v", err), } c.JSON(http.StatusInternalServerError, resp) return }This would provide more informative feedback to the client in case of errors during passive quote handling.
services/rfq/relayer/quoter/quoter.go (4)
316-319: Clarify the return ofnil, nilingenerateActiveRFQReturning
nil, nilingenerateActiveRFQwhenmsg.Opis notrest.RequestQuoteOpmay cause confusion about the function's behavior.Consider explicitly documenting this behavior or modifying the function to make it clearer. Alternatively, you could return an explicit error or a no-op response to improve code readability.
Line range hint
548-582: Handle potential errors when calculatingoriginAmountIn
generateQuote, ifgetOriginAmountreturns an error, the code checks forerrMinGasExceedsQuoteAmountand setsoriginAmountto zero. However, other errors are logged and then returned, but the function continues execution. This might lead to unintended behavior iforiginAmountis invalid.Consider returning the error immediately after logging to prevent using an invalid
originAmount:if errors.Is(err, errMinGasExceedsQuoteAmount) { originAmount = big.NewInt(0) } else if err != nil { logger.Error("Error getting quote amount", "error", err) - return nil, err + return nil, fmt.Errorf("error getting origin amount: %w", err) }
Line range hint
827-835: Check for empty quotes before submitting bulk quotesIn
submitBulkQuotes, if thequotesslice is empty, thePutBulkQuotesAPI call may be unnecessary and could potentially cause errors depending on the API's handling of empty inputs.Add a check to return early if
quotesis empty:func (m *Manager) submitBulkQuotes(ctx context.Context, quotes []model.PutRelayerQuoteRequest) error { + if len(quotes) == 0 { + return nil + } quoteCtx, quoteCancel := context.WithTimeout(ctx, m.config.GetQuoteSubmissionTimeout()) defer quoteCancel() req := model.PutBulkQuotesRequest{ Quotes: quotes, } err := m.rfqClient.PutBulkQuotes(quoteCtx, &req) if err != nil { return fmt.Errorf("error submitting bulk quotes: %w", err) } return nil }
316-319: Handle unknown message operations explicitlyIn
generateActiveRFQ, whenmsg.Opis notrest.RequestQuoteOp, the function returnsnil, nil. This might make it harder to debug if unexpected message operations occur.Consider logging a warning or handling other operations explicitly to improve observability:
if msg.Op != rest.RequestQuoteOp { span.AddEvent("unexpected message operation", trace.WithAttributes( attribute.String("operation", msg.Op), )) return nil, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- services/rfq/api/client/client.go (8 hunks)
- services/rfq/api/rest/server.go (12 hunks)
- services/rfq/relayer/quoter/quoter.go (13 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/api/client/client.go
[warning] 82-82: services/rfq/api/client/client.go#L82
Added line #L82 was not covered by tests
[warning] 108-109: services/rfq/api/client/client.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 181-181: services/rfq/api/client/client.go#L181
Added line #L181 was not covered by tests
[warning] 183-184: services/rfq/api/client/client.go#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 186-187: services/rfq/api/client/client.go#L186-L187
Added lines #L186 - L187 were not covered by tests
[warning] 190-192: services/rfq/api/client/client.go#L190-L192
Added lines #L190 - L192 were not covered by tests
[warning] 194-202: services/rfq/api/client/client.go#L194-L202
Added lines #L194 - L202 were not covered by tests
[warning] 204-205: services/rfq/api/client/client.go#L204-L205
Added lines #L204 - L205 were not covered by tests
[warning] 208-210: services/rfq/api/client/client.go#L208-L210
Added lines #L208 - L210 were not covered by tests
[warning] 212-213: services/rfq/api/client/client.go#L212-L213
Added lines #L212 - L213 were not covered by tests
[warning] 215-217: services/rfq/api/client/client.go#L215-L217
Added lines #L215 - L217 were not covered by tests
[warning] 219-220: services/rfq/api/client/client.go#L219-L220
Added lines #L219 - L220 were not covered by tests
[warning] 224-224: services/rfq/api/client/client.go#L224
Added line #L224 was not covered by tests
[warning] 227-227: services/rfq/api/client/client.go#L227
Added line #L227 was not covered by tests
[warning] 230-233: services/rfq/api/client/client.go#L230-L233
Added lines #L230 - L233 were not covered by tests
[warning] 235-238: services/rfq/api/client/client.go#L235-L238
Added lines #L235 - L238 were not covered by tests
[warning] 240-248: services/rfq/api/client/client.go#L240-L248
Added lines #L240 - L248 were not covered by tests
[warning] 250-250: services/rfq/api/client/client.go#L250
Added line #L250 was not covered by tests
[warning] 253-265: services/rfq/api/client/client.go#L253-L265
Added lines #L253 - L265 were not covered by tests
[warning] 268-274: services/rfq/api/client/client.go#L268-L274
Added lines #L268 - L274 were not covered by tests
[warning] 277-300: services/rfq/api/client/client.go#L277-L300
Added lines #L277 - L300 were not covered by tests
[warning] 305-317: services/rfq/api/client/client.go#L305-L317
Added lines #L305 - L317 were not covered by tests
[warning] 321-329: services/rfq/api/client/client.go#L321-L329
Added lines #L321 - L329 were not covered by tests
[warning] 331-334: services/rfq/api/client/client.go#L331-L334
Added lines #L331 - L334 were not covered by tests
[warning] 339-344: services/rfq/api/client/client.go#L339-L344
Added lines #L339 - L344 were not covered by tests
[warning] 346-349: services/rfq/api/client/client.go#L346-L349
Added lines #L346 - L349 were not covered by tests
[warning] 351-351: services/rfq/api/client/client.go#L351
Added line #L351 was not covered by tests
[warning] 437-447: services/rfq/api/client/client.go#L437-L447
Added lines #L437 - L447 were not covered by tests
[warning] 449-451: services/rfq/api/client/client.go#L449-L451
Added lines #L449 - L451 were not covered by tests
[warning] 453-453: services/rfq/api/client/client.go#L453
Added line #L453 was not covered by tests
🔇 Additional comments (8)
services/rfq/api/client/client.go (4)
49-51: Approve addition of PutRFQRequest methodThe addition of the
PutRFQRequestmethod to theUnauthenticatedClientinterface is a good improvement. It allows for unauthenticated quote requests, expanding the API's capabilities.The method signature follows good practices by returning both a response and an error.
253-266: Approve getWsHeaders implementationThe
getWsHeadersmethod is well-implemented. It correctly prepares the headers for the WebSocket connection, including the chain IDs and authentication information.Key points:
- Proper error handling is in place.
- The use of JSON for encoding chain IDs provides flexibility.
- The method reuses the
getAuthHeaderfunction, maintaining consistency in authentication.This implementation follows good practices and should work well for establishing authenticated WebSocket connections.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 253-265: services/rfq/api/client/client.go#L253-L265
Added lines #L253 - L265 were not covered by tests
137-137: Verify all calls to PutQuote methodThe change in the parameter type for the
PutQuotemethod is consistent with the earlier modification in theAuthenticatedClientinterface.Ensure that all calls to
PutQuotethroughout the codebase have been updated to use*model.PutRelayerQuoteRequest. Run the following script to find any potentially outdated calls:#!/bin/bash # Find calls to PutQuote that might still be using the old type rg --type go 'PutQuote\(' | rg 'PutQuoteRequest'
37-40: Verify the impact of PutQuote parameter type changeThe changes to the
AuthenticatedClientinterface look good. The newSubscribeActiveQuotesmethod adds valuable real-time functionality.However, changing the parameter type of
PutQuotefrom*model.PutQuoteRequestto*model.PutRelayerQuoteRequestmight impact existing code. Please verify that all calls toPutQuotehave been updated accordingly.✅ Verification successful
PutQuote Parameter Type Change Verified
All calls to
PutQuotehave been updated to use*model.PutRelayerQuoteRequest. The parameter type change does not impact existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to PutQuote and verify the argument type # Search for PutQuote method calls rg --type go 'PutQuote\(' -A 1Length of output: 144
Script:
#!/bin/bash # Description: Find all calls to PutQuote and verify the argument type # Search for PutQuote method calls and display the line with the argument rg --type go 'PutQuote\(' -A 1Length of output: 1054
Script:
#!/bin/bash # Description: Find the declarations of 'quote' and 'req' to verify their types # Find declaration of 'quote' in quoter.go rg --type go 'var\s+quote\s+' services/rfq/relayer/quoter/quoter.go # Find declaration of 'req' in client_test.go rg --type go 'var\s+req\s+' services/rfq/api/client/client_test.goLength of output: 138
Script:
#!/bin/bash # Description: Find the declarations of 'quote' and 'req' to verify their types using broader patterns # Search for 'quote' declarations using var, :=, or within function parameters rg --type go -e 'var\s+quote\s+' -e '\squote\s*:=\s*' -e '\squote\s+' services/rfq/relayer/quoter/quoter.go # Search for 'req' declarations using var, :=, or within function parameters rg --type go -e 'var\s+req\s+' -e '\sreq\s*:=\s*' -e '\sreq\s+' services/rfq/api/client/client_test.goLength of output: 2779
services/rfq/api/rest/server.go (3)
143-144: LGTM! Proper initialization of new fields.The initialization of
wsClientsandpubSubManagerin theNewAPIfunction is correct and consistent with the new fields added to theQuoterAPIServerstruct. This ensures that these components are properly set up when a newQuoterAPIServerinstance is created.
Line range hint
586-616: No changes in this function.The
recordLatestQuoteAgefunction remains unchanged, so no review is necessary.
Line range hint
1-616: Overall, well-implemented RFQ and WebSocket functionality with minor improvements needed.The changes to this file successfully implement the RFQ and WebSocket functionality with good code quality. The main areas for improvement are:
- Remove debug print statements or replace them with proper logging.
- Implement a secure origin check in the WebSocket upgrader.
- Use constants for error messages to improve maintainability.
- Consider adding more detailed error information in the response for passive quote handling.
These minor improvements will enhance the security, maintainability, and robustness of the code. Great job on implementing these new features!
services/rfq/relayer/quoter/quoter.go (1)
Line range hint
700-708: EnsureoffsetBpsis within valid range to prevent calculation errorsIn
applyOffset,offsetBpsis used to calculateoffsetFraction. IfoffsetBpsis greater than10000,offsetFractionbecomes greater than1, resulting in a negativeoffsetFactorand possibly negativeresult.Add validation to ensure
offsetBpsis between0and10000:func (m *Manager) applyOffset(parentCtx context.Context, offsetBps float64, target *big.Int) (result *big.Int) { + if offsetBps < 0 || offsetBps > 10000 { + offsetBps = math.Max(0, math.Min(offsetBps, 10000)) + } // Existing code... }Alternatively, log a warning if
offsetBpsis out of expected range.
| "github.com/ipfs/go-log" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating the logging package
The current implementation uses github.com/ipfs/go-log for logging. It's recommended to use a more modern and widely adopted logging library or the project's standard logging approach. This would help maintain consistency across the project and potentially avoid issues with outdated dependencies.
Consider replacing github.com/ipfs/go-log with a more standard logging library such as log from the standard library, or popular third-party options like github.com/sirupsen/logrus or go.uber.org/zap.
| func (c *clientImpl) handleWsMessage(ctx context.Context, msg []byte, respChan chan *model.ActiveRFQMessage) (err error) { | ||
| var rfqMsg model.ActiveRFQMessage | ||
| err = json.Unmarshal(msg, &rfqMsg) | ||
| if err != nil { | ||
| return fmt.Errorf("error unmarshaling message: %w", err) | ||
| } | ||
|
|
||
| select { | ||
| case respChan <- &rfqMsg: | ||
| case <-ctx.Done(): | ||
| return nil | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Enhance handleWsMessage implementation
The handleWsMessage method effectively unmarshals and handles incoming WebSocket messages. However, there are several areas that could be improved:
- Provide more informative error handling.
- Handle potential blocking when sending to the channel.
- Add validation for the unmarshaled message structure.
Here's a suggested improvement:
func (c *clientImpl) handleWsMessage(ctx context.Context, msg []byte, respChan chan *model.ActiveRFQMessage) (err error) {
var rfqMsg model.ActiveRFQMessage
err = json.Unmarshal(msg, &rfqMsg)
if err != nil {
- return fmt.Errorf("error unmarshaling message: %w", err)
+ return fmt.Errorf("failed to unmarshal WebSocket message: %w", err)
}
+ // Validate the message structure
+ if err := rfqMsg.Validate(); err != nil {
+ return fmt.Errorf("invalid message structure: %w", err)
+ }
+
select {
case respChan <- &rfqMsg:
+ logger.Debugf("Successfully sent message to response channel: %v", rfqMsg.Op)
case <-ctx.Done():
- return nil
+ return ctx.Err()
+ default:
+ // If the channel is full, log a warning and continue
+ logger.Warn("Response channel full, discarding message")
}
return nil
}These changes provide more detailed error messages, add message structure validation (assuming a Validate method exists on ActiveRFQMessage), and handle the case where the response channel might be full. The default case in the select statement prevents blocking if respChan is full, logging a warning instead.
Consider adding a Validate method to the ActiveRFQMessage struct if it doesn't already exist:
func (m *ActiveRFQMessage) Validate() error {
if m.Op == "" {
return errors.New("message operation is empty")
}
// Add more validation as needed
return nil
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 339-344: services/rfq/api/client/client.go#L339-L344
Added lines #L339 - L344 were not covered by tests
[warning] 346-349: services/rfq/api/client/client.go#L346-L349
Added lines #L346 - L349 were not covered by tests
[warning] 351-351: services/rfq/api/client/client.go#L351
Added line #L351 was not covered by tests
| func (c unauthenticatedClient) PutRFQRequest(ctx context.Context, q *model.PutRFQRequest) (*model.PutRFQResponse, error) { | ||
| var response model.PutRFQResponse | ||
| resp, err := c.rClient.R(). | ||
| SetContext(ctx). | ||
| SetBody(q). | ||
| SetResult(&response). | ||
| Put(rest.RFQRoute) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("error from server: %s: %w", getStatus(resp), err) | ||
| } | ||
|
|
||
| if resp.IsError() { | ||
| return nil, fmt.Errorf("error from server: %s", getStatus(resp)) | ||
| } | ||
|
|
||
| return &response, nil | ||
| } |
There was a problem hiding this comment.
Improve PutRFQRequest implementation
The PutRFQRequest method correctly implements the functionality for unauthenticated quote requests. However, there are several areas that could be improved:
- Provide more descriptive error messages.
- Add a nil check for the response before dereferencing.
- Handle potential empty responses for successful requests.
Here's a suggested improvement:
func (c unauthenticatedClient) PutRFQRequest(ctx context.Context, q *model.PutRFQRequest) (*model.PutRFQResponse, error) {
var response model.PutRFQResponse
resp, err := c.rClient.R().
SetContext(ctx).
SetBody(q).
SetResult(&response).
Put(rest.RFQRoute)
if err != nil {
- return nil, fmt.Errorf("error from server: %s: %w", getStatus(resp), err)
+ return nil, fmt.Errorf("failed to send RFQ request: %s: %w", getStatus(resp), err)
}
if resp.IsError() {
- return nil, fmt.Errorf("error from server: %s", getStatus(resp))
+ return nil, fmt.Errorf("server returned error for RFQ request: %s", getStatus(resp))
}
+ // Check for empty response on successful requests
+ if resp.IsSuccess() && resp.StatusCode() != http.StatusNoContent {
+ if response == (model.PutRFQResponse{}) {
+ return nil, fmt.Errorf("received empty response for successful RFQ request")
+ }
+ }
return &response, nil
}These changes provide more descriptive error messages and add a check for empty responses on successful requests, improving the robustness of the method.
Consider adding a validation step for the input q *model.PutRFQRequest to ensure all required fields are present before sending the request:
if err := q.Validate(); err != nil {
return nil, fmt.Errorf("invalid RFQ request: %w", err)
}This assumes a Validate method exists on the PutRFQRequest struct. If it doesn't, consider adding one to perform input validation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 437-447: services/rfq/api/client/client.go#L437-L447
Added lines #L437 - L447 were not covered by tests
[warning] 449-451: services/rfq/api/client/client.go#L449-L451
Added lines #L449 - L451 were not covered by tests
[warning] 453-453: services/rfq/api/client/client.go#L453
Added line #L453 was not covered by tests
| func (c *clientImpl) listenWsMessages(ctx context.Context, conn *websocket.Conn, readChan chan []byte) { | ||
| defer close(readChan) | ||
| for { | ||
| _, message, err := conn.ReadMessage() | ||
| if err != nil { | ||
| if websocket.IsUnexpectedCloseError(err, websocket.CloseGoingAway, websocket.CloseAbnormalClosure) { | ||
| logger.Warnf("websocket connection closed unexpectedly: %v", err) | ||
| } | ||
| return | ||
| } | ||
| select { | ||
| case readChan <- message: | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve listenWsMessages implementation
The listenWsMessages method effectively listens for incoming WebSocket messages. However, there are several areas that could be improved:
- Add a timeout for reading messages to prevent indefinite blocking.
- Enhance error handling with more informative messages.
- Respect context cancellation when writing to the channel.
Here's a suggested improvement:
func (c *clientImpl) listenWsMessages(ctx context.Context, conn *websocket.Conn, readChan chan []byte) {
defer close(readChan)
for {
+ err := conn.SetReadDeadline(time.Now().Add(60 * time.Second))
+ if err != nil {
+ logger.Errorf("Failed to set read deadline: %v", err)
+ return
+ }
_, message, err := conn.ReadMessage()
if err != nil {
if websocket.IsUnexpectedCloseError(err, websocket.CloseGoingAway, websocket.CloseAbnormalClosure) {
- logger.Warnf("websocket connection closed unexpectedly: %v", err)
+ logger.Errorf("WebSocket connection closed unexpectedly: %v", err)
+ } else if err == io.EOF {
+ logger.Info("WebSocket connection closed")
+ } else {
+ logger.Errorf("Error reading WebSocket message: %v", err)
}
return
}
select {
case readChan <- message:
case <-ctx.Done():
+ logger.Info("Context cancelled, stopping WebSocket listener")
return
+ default:
+ logger.Warn("Read channel full, discarding message")
}
}
}These changes add a read timeout, provide more detailed error logging, and ensure the method respects context cancellation. The default case in the select statement prevents blocking if the readChan is full.
📝 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 (c *clientImpl) listenWsMessages(ctx context.Context, conn *websocket.Conn, readChan chan []byte) { | |
| defer close(readChan) | |
| for { | |
| _, message, err := conn.ReadMessage() | |
| if err != nil { | |
| if websocket.IsUnexpectedCloseError(err, websocket.CloseGoingAway, websocket.CloseAbnormalClosure) { | |
| logger.Warnf("websocket connection closed unexpectedly: %v", err) | |
| } | |
| return | |
| } | |
| select { | |
| case readChan <- message: | |
| case <-ctx.Done(): | |
| return | |
| } | |
| } | |
| } | |
| func (c *clientImpl) listenWsMessages(ctx context.Context, conn *websocket.Conn, readChan chan []byte) { | |
| defer close(readChan) | |
| for { | |
| err := conn.SetReadDeadline(time.Now().Add(60 * time.Second)) | |
| if err != nil { | |
| logger.Errorf("Failed to set read deadline: %v", err) | |
| return | |
| } | |
| _, message, err := conn.ReadMessage() | |
| if err != nil { | |
| if websocket.IsUnexpectedCloseError(err, websocket.CloseGoingAway, websocket.CloseAbnormalClosure) { | |
| logger.Errorf("WebSocket connection closed unexpectedly: %v", err) | |
| } else if err == io.EOF { | |
| logger.Info("WebSocket connection closed") | |
| } else { | |
| logger.Errorf("Error reading WebSocket message: %v", err) | |
| } | |
| return | |
| } | |
| select { | |
| case readChan <- message: | |
| case <-ctx.Done(): | |
| logger.Info("Context cancelled, stopping WebSocket listener") | |
| return | |
| default: | |
| logger.Warn("Read channel full, discarding message") | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 321-329: services/rfq/api/client/client.go#L321-L329
Added lines #L321 - L329 were not covered by tests
[warning] 331-334: services/rfq/api/client/client.go#L331-L334
Added lines #L331 - L334 were not covered by tests
| func (c *clientImpl) processWebsocket(ctx context.Context, conn *websocket.Conn, reqChan, respChan chan *model.ActiveRFQMessage) (err error) { | ||
| defer func() { | ||
| close(respChan) | ||
| err := conn.Close() | ||
| if err != nil { | ||
| logger.Warnf("error closing websocket connection: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| readChan := make(chan []byte) | ||
| go c.listenWsMessages(ctx, conn, readChan) | ||
| go c.sendPings(ctx, reqChan) | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil | ||
| case msg, ok := <-reqChan: | ||
| if !ok { | ||
| return fmt.Errorf("error reading from reqChan: %w", ctx.Err()) | ||
| } | ||
| err := conn.WriteJSON(msg) | ||
| if err != nil { | ||
| return fmt.Errorf("error sending message to websocket: %w", err) | ||
| } | ||
| case msg, ok := <-readChan: | ||
| if !ok { | ||
| return nil | ||
| } | ||
| err = c.handleWsMessage(ctx, msg, respChan) | ||
| if err != nil { | ||
| return fmt.Errorf("error handling websocket message: %w", err) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve processWebsocket implementation
The processWebsocket method handles the main WebSocket communication loop effectively. However, there are several areas that could be improved:
- The error returned from the deferred function is not handled.
- There's no mechanism to stop the goroutines when the function returns.
- The error handling in the select statement could be more robust.
Here's a suggested improvement:
func (c *clientImpl) processWebsocket(ctx context.Context, conn *websocket.Conn, reqChan, respChan chan *model.ActiveRFQMessage) (err error) {
+ ctx, cancel := context.WithCancel(ctx)
+ defer cancel()
defer func() {
close(respChan)
- err := conn.Close()
- if err != nil {
+ if closeErr := conn.Close(); closeErr != nil {
logger.Warnf("error closing websocket connection: %v", err)
+ if err == nil {
+ err = closeErr
+ }
}
}()
readChan := make(chan []byte)
- go c.listenWsMessages(ctx, conn, readChan)
- go c.sendPings(ctx, reqChan)
+ go func() {
+ c.listenWsMessages(ctx, conn, readChan)
+ cancel()
+ }()
+ go func() {
+ c.sendPings(ctx, reqChan)
+ cancel()
+ }()
for {
select {
case <-ctx.Done():
- return nil
+ return ctx.Err()
case msg, ok := <-reqChan:
if !ok {
return fmt.Errorf("error reading from reqChan: %w", ctx.Err())
}
err := conn.WriteJSON(msg)
if err != nil {
- return fmt.Errorf("error sending message to websocket: %w", err)
+ return fmt.Errorf("error sending message to websocket: %w", err)
}
case msg, ok := <-readChan:
if !ok {
- return nil
+ return fmt.Errorf("websocket read channel closed unexpectedly")
}
err = c.handleWsMessage(ctx, msg, respChan)
if err != nil {
return fmt.Errorf("error handling websocket message: %w", err)
}
}
}
}These changes improve error handling, ensure proper cleanup of goroutines, and provide more context in error messages.
📝 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 (c *clientImpl) processWebsocket(ctx context.Context, conn *websocket.Conn, reqChan, respChan chan *model.ActiveRFQMessage) (err error) { | |
| defer func() { | |
| close(respChan) | |
| err := conn.Close() | |
| if err != nil { | |
| logger.Warnf("error closing websocket connection: %v", err) | |
| } | |
| }() | |
| readChan := make(chan []byte) | |
| go c.listenWsMessages(ctx, conn, readChan) | |
| go c.sendPings(ctx, reqChan) | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return nil | |
| case msg, ok := <-reqChan: | |
| if !ok { | |
| return fmt.Errorf("error reading from reqChan: %w", ctx.Err()) | |
| } | |
| err := conn.WriteJSON(msg) | |
| if err != nil { | |
| return fmt.Errorf("error sending message to websocket: %w", err) | |
| } | |
| case msg, ok := <-readChan: | |
| if !ok { | |
| return nil | |
| } | |
| err = c.handleWsMessage(ctx, msg, respChan) | |
| if err != nil { | |
| return fmt.Errorf("error handling websocket message: %w", err) | |
| } | |
| } | |
| } | |
| } | |
| func (c *clientImpl) processWebsocket(ctx context.Context, conn *websocket.Conn, reqChan, respChan chan *model.ActiveRFQMessage) (err error) { | |
| ctx, cancel := context.WithCancel(ctx) | |
| defer cancel() | |
| defer func() { | |
| close(respChan) | |
| if closeErr := conn.Close(); closeErr != nil { | |
| logger.Warnf("error closing websocket connection: %v", err) | |
| if err == nil { | |
| err = closeErr | |
| } | |
| } | |
| }() | |
| readChan := make(chan []byte) | |
| go func() { | |
| c.listenWsMessages(ctx, conn, readChan) | |
| cancel() | |
| }() | |
| go func() { | |
| c.sendPings(ctx, reqChan) | |
| cancel() | |
| }() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| case msg, ok := <-reqChan: | |
| if !ok { | |
| return fmt.Errorf("error reading from reqChan: %w", ctx.Err()) | |
| } | |
| err := conn.WriteJSON(msg) | |
| if err != nil { | |
| return fmt.Errorf("error sending message to websocket: %w", err) | |
| } | |
| case msg, ok := <-readChan: | |
| if !ok { | |
| return fmt.Errorf("websocket read channel closed unexpectedly") | |
| } | |
| err = c.handleWsMessage(ctx, msg, respChan) | |
| if err != nil { | |
| return fmt.Errorf("error handling websocket message: %w", err) | |
| } | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 268-274: services/rfq/api/client/client.go#L268-L274
Added lines #L268 - L274 were not covered by tests
[warning] 277-300: services/rfq/api/client/client.go#L277-L300
Added lines #L277 - L300 were not covered by tests
| func (c *clientImpl) connectWebsocket(ctx context.Context, req *model.SubscribeActiveRFQRequest) (conn *websocket.Conn, err error) { | ||
| if len(req.ChainIDs) == 0 { | ||
| return nil, fmt.Errorf("chain IDs are required") | ||
| } | ||
|
|
||
| header, err := c.getWsHeaders(ctx, req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get auth header: %w", err) | ||
| } | ||
|
|
||
| reqURL := strings.Replace(c.rClient.BaseURL, "http", "ws", 1) + rest.RFQStreamRoute | ||
| conn, httpResp, err := websocket.DefaultDialer.Dial(reqURL, header) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to connect to websocket: %w", err) | ||
| } | ||
| err = httpResp.Body.Close() | ||
| if err != nil { | ||
| logger.Warnf("error closing websocket connection: %v", err) | ||
| } | ||
|
|
||
| return conn, nil | ||
| } |
There was a problem hiding this comment.
Improve WebSocket connection handling
The connectWebsocket method establishes a WebSocket connection with proper headers. However, there are several areas that could be improved:
- The URL scheme conversion doesn't correctly handle HTTPS to WSS.
- The error from
httpResp.Body.Close()is logged but not returned. - There's no context timeout for the dial operation.
Here's a suggested improvement:
+import "net/url"
func (c *clientImpl) connectWebsocket(ctx context.Context, req *model.SubscribeActiveRFQRequest) (conn *websocket.Conn, err error) {
if len(req.ChainIDs) == 0 {
return nil, fmt.Errorf("chain IDs are required")
}
header, err := c.getWsHeaders(ctx, req)
if err != nil {
return nil, fmt.Errorf("failed to get auth header: %w", err)
}
- reqURL := strings.Replace(c.rClient.BaseURL, "http", "ws", 1) + rest.RFQStreamRoute
+ baseURL, err := url.Parse(c.rClient.BaseURL)
+ if err != nil {
+ return nil, fmt.Errorf("invalid base URL: %w", err)
+ }
+ switch baseURL.Scheme {
+ case "http":
+ baseURL.Scheme = "ws"
+ case "https":
+ baseURL.Scheme = "wss"
+ default:
+ return nil, fmt.Errorf("unsupported URL scheme: %s", baseURL.Scheme)
+ }
+ baseURL.Path = path.Join(baseURL.Path, rest.RFQStreamRoute)
+ reqURL := baseURL.String()
+ ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
- conn, httpResp, err := websocket.DefaultDialer.Dial(reqURL, header)
+ conn, httpResp, err := websocket.DefaultDialer.DialContext(ctx, reqURL, header)
if err != nil {
return nil, fmt.Errorf("failed to connect to websocket: %w", err)
}
- err = httpResp.Body.Close()
- if err != nil {
- logger.Warnf("error closing websocket connection: %v", err)
+ if httpResp != nil && httpResp.Body != nil {
+ if closeErr := httpResp.Body.Close(); closeErr != nil {
+ return conn, fmt.Errorf("error closing response body: %w", closeErr)
+ }
}
return conn, nil
}These changes ensure proper handling of HTTPS URLs, add a timeout for the dial operation, and improve error handling when closing the response body.
📝 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 (c *clientImpl) connectWebsocket(ctx context.Context, req *model.SubscribeActiveRFQRequest) (conn *websocket.Conn, err error) { | |
| if len(req.ChainIDs) == 0 { | |
| return nil, fmt.Errorf("chain IDs are required") | |
| } | |
| header, err := c.getWsHeaders(ctx, req) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get auth header: %w", err) | |
| } | |
| reqURL := strings.Replace(c.rClient.BaseURL, "http", "ws", 1) + rest.RFQStreamRoute | |
| conn, httpResp, err := websocket.DefaultDialer.Dial(reqURL, header) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to connect to websocket: %w", err) | |
| } | |
| err = httpResp.Body.Close() | |
| if err != nil { | |
| logger.Warnf("error closing websocket connection: %v", err) | |
| } | |
| return conn, nil | |
| } | |
| import ( | |
| "net/url" | |
| "time" | |
| ) | |
| func (c *clientImpl) connectWebsocket(ctx context.Context, req *model.SubscribeActiveRFQRequest) (conn *websocket.Conn, err error) { | |
| if len(req.ChainIDs) == 0 { | |
| return nil, fmt.Errorf("chain IDs are required") | |
| } | |
| header, err := c.getWsHeaders(ctx, req) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get auth header: %w", err) | |
| } | |
| baseURL, err := url.Parse(c.rClient.BaseURL) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid base URL: %w", err) | |
| } | |
| switch baseURL.Scheme { | |
| case "http": | |
| baseURL.Scheme = "ws" | |
| case "https": | |
| baseURL.Scheme = "wss" | |
| default: | |
| return nil, fmt.Errorf("unsupported URL scheme: %s", baseURL.Scheme) | |
| } | |
| baseURL.Path = path.Join(baseURL.Path, rest.RFQStreamRoute) | |
| reqURL := baseURL.String() | |
| ctx, cancel := context.WithTimeout(ctx, 30*time.Second) | |
| defer cancel() | |
| conn, httpResp, err := websocket.DefaultDialer.DialContext(ctx, reqURL, header) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to connect to websocket: %w", err) | |
| } | |
| if httpResp != nil && httpResp.Body != nil { | |
| if closeErr := httpResp.Body.Close(); closeErr != nil { | |
| return conn, fmt.Errorf("error closing response body: %w", closeErr) | |
| } | |
| } | |
| return conn, nil | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 230-233: services/rfq/api/client/client.go#L230-L233
Added lines #L230 - L233 were not covered by tests
[warning] 235-238: services/rfq/api/client/client.go#L235-L238
Added lines #L235 - L238 were not covered by tests
[warning] 240-248: services/rfq/api/client/client.go#L240-L248
Added lines #L240 - L248 were not covered by tests
[warning] 250-250: services/rfq/api/client/client.go#L250
Added line #L250 was not covered by tests
services/rfq/api/client/client.go
Outdated
| func (c *clientImpl) SubscribeActiveQuotes(ctx context.Context, req *model.SubscribeActiveRFQRequest, reqChan chan *model.ActiveRFQMessage) (respChan chan *model.ActiveRFQMessage, err error) { | ||
| fmt.Println("SubscribeActiveQuotes - starting") | ||
| conn, err := c.connectWebsocket(ctx, req) | ||
| if err != nil { | ||
| fmt.Printf("SubscribeActiveQuotes - failed to connect to websocket: %s\n", err) | ||
| return nil, fmt.Errorf("failed to connect to websocket: %w", err) | ||
| } | ||
| fmt.Println("SubscribeActiveQuotes - connected to websocket") | ||
| // first, subscrbe to the given chains | ||
| sub := model.SubscriptionParams{ | ||
| Chains: req.ChainIDs, | ||
| } | ||
| fmt.Printf("SubscribeActiveQuotes - sub: %v\n", sub) | ||
| subJSON, err := json.Marshal(sub) | ||
| if err != nil { | ||
| return respChan, fmt.Errorf("error marshaling subscription params: %w", err) | ||
| } | ||
| err = conn.WriteJSON(model.ActiveRFQMessage{ | ||
| Op: rest.SubscribeOp, | ||
| Content: json.RawMessage(subJSON), | ||
| }) | ||
| if err != nil { | ||
| fmt.Printf("SubscribeActiveQuotes - error sending subscribe message: %s\n", err) | ||
| return nil, fmt.Errorf("error sending subscribe message: %w", err) | ||
| } | ||
| fmt.Println("SubscribeActiveQuotes - subscribed to chains") | ||
| // make sure subscription is successful | ||
| var resp model.ActiveRFQMessage | ||
| err = conn.ReadJSON(&resp) | ||
| if err != nil { | ||
| fmt.Printf("SubscribeActiveQuotes - error reading subscribe response: %s\n", err) | ||
| return nil, fmt.Errorf("error reading subscribe response: %w", err) | ||
| } | ||
| fmt.Printf("SubscribeActiveQuotes - resp: %v\n", resp) | ||
| if !resp.Success || resp.Op != rest.SubscribeOp { | ||
| return nil, fmt.Errorf("subscription failed") | ||
| } | ||
|
|
||
| respChan = make(chan *model.ActiveRFQMessage) | ||
| go func() { | ||
| wsErr := c.processWebsocket(ctx, conn, reqChan, respChan) | ||
| if wsErr != nil { | ||
| logger.Error("Error running websocket listener: %s", wsErr) | ||
| } | ||
| }() | ||
|
|
||
| return respChan, nil | ||
| } |
There was a problem hiding this comment.
Improve SubscribeActiveQuotes implementation
The SubscribeActiveQuotes method is a good addition for real-time quote updates. However, there are several areas that could be improved:
- Replace
fmt.Printlncalls with proper logging using theloggervariable. - Add a timeout for the initial subscription message to prevent hanging.
- Ensure proper cleanup to prevent goroutine leaks.
Here's a suggested improvement:
func (c *clientImpl) SubscribeActiveQuotes(ctx context.Context, req *model.SubscribeActiveRFQRequest, reqChan chan *model.ActiveRFQMessage) (respChan chan *model.ActiveRFQMessage, err error) {
- fmt.Println("SubscribeActiveQuotes - starting")
+ logger.Info("SubscribeActiveQuotes - starting")
conn, err := c.connectWebsocket(ctx, req)
if err != nil {
- fmt.Printf("SubscribeActiveQuotes - failed to connect to websocket: %s\n", err)
+ logger.Errorf("SubscribeActiveQuotes - failed to connect to websocket: %s", err)
return nil, fmt.Errorf("failed to connect to websocket: %w", err)
}
- fmt.Println("SubscribeActiveQuotes - connected to websocket")
+ logger.Info("SubscribeActiveQuotes - connected to websocket")
+ // Add cleanup in case of early return
+ defer func() {
+ if err != nil {
+ conn.Close()
+ }
+ }()
// ... (rest of the code)
- err = conn.ReadJSON(&resp)
+ // Add timeout for initial subscription
+ err = conn.SetReadDeadline(time.Now().Add(10 * time.Second))
+ if err != nil {
+ return nil, fmt.Errorf("failed to set read deadline: %w", err)
+ }
+ err = conn.ReadJSON(&resp)
+ conn.SetReadDeadline(time.Time{}) // Reset deadline
if err != nil {
- fmt.Printf("SubscribeActiveQuotes - error reading subscribe response: %s\n", err)
+ logger.Errorf("SubscribeActiveQuotes - error reading subscribe response: %s", err)
return nil, fmt.Errorf("error reading subscribe response: %w", err)
}
- fmt.Printf("SubscribeActiveQuotes - resp: %v\n", resp)
+ logger.Infof("SubscribeActiveQuotes - resp: %v", resp)
// ... (rest of the code)
}These changes improve logging, add a timeout for the initial subscription, and ensure proper cleanup in case of errors.
📝 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 (c *clientImpl) SubscribeActiveQuotes(ctx context.Context, req *model.SubscribeActiveRFQRequest, reqChan chan *model.ActiveRFQMessage) (respChan chan *model.ActiveRFQMessage, err error) { | |
| fmt.Println("SubscribeActiveQuotes - starting") | |
| conn, err := c.connectWebsocket(ctx, req) | |
| if err != nil { | |
| fmt.Printf("SubscribeActiveQuotes - failed to connect to websocket: %s\n", err) | |
| return nil, fmt.Errorf("failed to connect to websocket: %w", err) | |
| } | |
| fmt.Println("SubscribeActiveQuotes - connected to websocket") | |
| // first, subscrbe to the given chains | |
| sub := model.SubscriptionParams{ | |
| Chains: req.ChainIDs, | |
| } | |
| fmt.Printf("SubscribeActiveQuotes - sub: %v\n", sub) | |
| subJSON, err := json.Marshal(sub) | |
| if err != nil { | |
| return respChan, fmt.Errorf("error marshaling subscription params: %w", err) | |
| } | |
| err = conn.WriteJSON(model.ActiveRFQMessage{ | |
| Op: rest.SubscribeOp, | |
| Content: json.RawMessage(subJSON), | |
| }) | |
| if err != nil { | |
| fmt.Printf("SubscribeActiveQuotes - error sending subscribe message: %s\n", err) | |
| return nil, fmt.Errorf("error sending subscribe message: %w", err) | |
| } | |
| fmt.Println("SubscribeActiveQuotes - subscribed to chains") | |
| // make sure subscription is successful | |
| var resp model.ActiveRFQMessage | |
| err = conn.ReadJSON(&resp) | |
| if err != nil { | |
| fmt.Printf("SubscribeActiveQuotes - error reading subscribe response: %s\n", err) | |
| return nil, fmt.Errorf("error reading subscribe response: %w", err) | |
| } | |
| fmt.Printf("SubscribeActiveQuotes - resp: %v\n", resp) | |
| if !resp.Success || resp.Op != rest.SubscribeOp { | |
| return nil, fmt.Errorf("subscription failed") | |
| } | |
| respChan = make(chan *model.ActiveRFQMessage) | |
| go func() { | |
| wsErr := c.processWebsocket(ctx, conn, reqChan, respChan) | |
| if wsErr != nil { | |
| logger.Error("Error running websocket listener: %s", wsErr) | |
| } | |
| }() | |
| return respChan, nil | |
| } | |
| func (c *clientImpl) SubscribeActiveQuotes(ctx context.Context, req *model.SubscribeActiveRFQRequest, reqChan chan *model.ActiveRFQMessage) (respChan chan *model.ActiveRFQMessage, err error) { | |
| logger.Info("SubscribeActiveQuotes - starting") | |
| conn, err := c.connectWebsocket(ctx, req) | |
| if err != nil { | |
| logger.Errorf("SubscribeActiveQuotes - failed to connect to websocket: %s", err) | |
| return nil, fmt.Errorf("failed to connect to websocket: %w", err) | |
| } | |
| logger.Info("SubscribeActiveQuotes - connected to websocket") | |
| // Add cleanup in case of early return | |
| defer func() { | |
| if err != nil { | |
| conn.Close() | |
| } | |
| }() | |
| // first, subscrbe to the given chains | |
| sub := model.SubscriptionParams{ | |
| Chains: req.ChainIDs, | |
| } | |
| fmt.Printf("SubscribeActiveQuotes - sub: %v\n", sub) | |
| subJSON, err := json.Marshal(sub) | |
| if err != nil { | |
| return respChan, fmt.Errorf("error marshaling subscription params: %w", err) | |
| } | |
| err = conn.WriteJSON(model.ActiveRFQMessage{ | |
| Op: rest.SubscribeOp, | |
| Content: json.RawMessage(subJSON), | |
| }) | |
| if err != nil { | |
| fmt.Printf("SubscribeActiveQuotes - error sending subscribe message: %s\n", err) | |
| return nil, fmt.Errorf("error sending subscribe message: %w", err) | |
| } | |
| fmt.Println("SubscribeActiveQuotes - subscribed to chains") | |
| // make sure subscription is successful | |
| var resp model.ActiveRFQMessage | |
| // Add timeout for initial subscription | |
| err = conn.SetReadDeadline(time.Now().Add(10 * time.Second)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to set read deadline: %w", err) | |
| } | |
| err = conn.ReadJSON(&resp) | |
| conn.SetReadDeadline(time.Time{}) // Reset deadline | |
| if err != nil { | |
| logger.Errorf("SubscribeActiveQuotes - error reading subscribe response: %s", err) | |
| return nil, fmt.Errorf("error reading subscribe response: %w", err) | |
| } | |
| logger.Infof("SubscribeActiveQuotes - resp: %v", resp) | |
| if !resp.Success || resp.Op != rest.SubscribeOp { | |
| return nil, fmt.Errorf("subscription failed") | |
| } | |
| respChan = make(chan *model.ActiveRFQMessage) | |
| go func() { | |
| wsErr := c.processWebsocket(ctx, conn, reqChan, respChan) | |
| if wsErr != nil { | |
| logger.Error("Error running websocket listener: %s", wsErr) | |
| } | |
| }() | |
| return respChan, nil | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 181-181: services/rfq/api/client/client.go#L181
Added line #L181 was not covered by tests
[warning] 183-184: services/rfq/api/client/client.go#L183-L184
Added lines #L183 - L184 were not covered by tests
[warning] 186-187: services/rfq/api/client/client.go#L186-L187
Added lines #L186 - L187 were not covered by tests
[warning] 190-192: services/rfq/api/client/client.go#L190-L192
Added lines #L190 - L192 were not covered by tests
[warning] 194-202: services/rfq/api/client/client.go#L194-L202
Added lines #L194 - L202 were not covered by tests
[warning] 204-205: services/rfq/api/client/client.go#L204-L205
Added lines #L204 - L205 were not covered by tests
[warning] 208-210: services/rfq/api/client/client.go#L208-L210
Added lines #L208 - L210 were not covered by tests
[warning] 212-213: services/rfq/api/client/client.go#L212-L213
Added lines #L212 - L213 were not covered by tests
[warning] 215-217: services/rfq/api/client/client.go#L215-L217
Added lines #L215 - L217 were not covered by tests
[warning] 219-220: services/rfq/api/client/client.go#L219-L220
Added lines #L219 - L220 were not covered by tests
[warning] 224-224: services/rfq/api/client/client.go#L224
Added line #L224 was not covered by tests
[warning] 227-227: services/rfq/api/client/client.go#L227
Added line #L227 was not covered by tests
| // SubscribeActiveRFQ subscribes to the RFQ websocket API. | ||
| // This function is blocking and will run until the context is canceled. | ||
| func (m *Manager) SubscribeActiveRFQ(ctx context.Context) (err error) { | ||
| fmt.Println("SubscribeActiveRFQ - starting") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use logger instead of fmt package for logging
Lines 261, 271, and 283 use fmt.Println and fmt.Printf for logging. It's recommended to use the logger provided by the logging framework to maintain consistent log formatting and routing.
Apply this diff to replace fmt statements with logger:
- fmt.Println("SubscribeActiveRFQ - starting")
+ logger.Info("SubscribeActiveRFQ - starting")
- fmt.Printf("SubscribeActiveRFQ - chainIDs: %v\n", chainIDs)
+ logger.Infof("SubscribeActiveRFQ - chainIDs: %v", chainIDs)
- fmt.Println("SubscribeActiveRFQ - subscribed to active quotes")
+ logger.Info("SubscribeActiveRFQ - subscribed to active quotes")Also applies to: 271-271, 283-283
| @@ -362,7 +474,7 @@ | |||
| // generate quotes in parallel | |||
| g, gctx := errgroup.WithContext(ctx) | |||
| quoteMtx := &sync.Mutex{} | |||
| quotes = []model.PutQuoteRequest{} | |||
| quotes = []model.PutRelayerQuoteRequest{} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Protect currentQuotes with a mutex to prevent data races
The currentQuotes slice is updated in generateQuotes without synchronization (m.currentQuotes = quotes at line 477). Since currentQuotes is also read in recordQuoteAmounts, which may be called concurrently by the metrics system, this can lead to data races.
Consider adding a mutex to synchronize access to m.currentQuotes. For example:
+ m.currentQuotesMutex.Lock()
m.currentQuotes = quotes
+ m.currentQuotesMutex.Unlock()Also, initialize the mutex in the Manager struct:
type Manager struct {
// Existing fields...
// quoteAmountGauge stores a histogram of quote amounts.
quoteAmountGauge metric.Float64ObservableGauge
// currentQuotes is used for recording quote metrics.
currentQuotes []model.PutRelayerQuoteRequest
+ currentQuotesMutex sync.RWMutex
}And use the mutex in recordQuoteAmounts:
func (m *Manager) recordQuoteAmounts(_ context.Context, observer metric.Observer) (err error) {
if m.meter == nil || m.quoteAmountGauge == nil {
return nil
}
+ m.currentQuotesMutex.RLock()
quotes := m.currentQuotes
+ m.currentQuotesMutex.RUnlock()
if quotes == nil {
return nil
}
for _, quote := range quotes {
// Existing code...
}
return nil
}Also applies to: 88-88
| if err != nil { | ||
| return fmt.Errorf("error generating active RFQ message: %w", err) | ||
| } | ||
| reqChan <- resp |
There was a problem hiding this comment.
Check for nil before sending on channel to prevent panics
When sending resp to reqChan, there is a possibility that resp is nil, especially if generateActiveRFQ returns nil, nil. Sending a nil value on a channel might lead to unexpected behavior.
Apply this diff to ensure resp is not nil before sending:
- reqChan <- resp
+ if resp != nil {
+ reqChan <- resp
+ }📝 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.
| reqChan <- resp | |
| if resp != nil { | |
| reqChan <- resp | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
services/rfq/api/rest/server.go (6)
13-15: LGTM! New imports and struct fields for WebSocket support.The additions of WebSocket-related imports and struct fields are appropriate for the new functionality. Consider adding comments to explain the purpose of each new field in the
QuoterAPIServerstruct for better code documentation.Also applies to: 56-56, 67-71
Line range hint
196-227: New routes added, but WebSocket security needs improvement.The addition of new routes for RFQ and WebSocket connections is well-integrated. However, the WebSocket upgrader's
CheckOriginfunction always returnstrue, which is a security risk. This allows connections from any origin, potentially exposing your application to cross-site WebSocket hijacking attacks.Implement a proper origin check. For example:
r.upgrader = websocket.Upgrader{ CheckOrigin: func(r *http.Request) bool { origin := r.Header.Get("Origin") // Check if the origin is in the whitelist return origin == "https://trusted-origin.com" }, }Replace "https://trusted-origin.com" with your actual allowed origins.
Line range hint
244-324: Authentication for new routes added, but logging needs improvement.The addition of authentication handling for RFQ routes and the parsing of the Chains header is appropriate. However, there are several issues to address:
- Remove or replace
fmt.Printfstatements with proper logging using theloggervariable.- Consider using a switch statement instead of if-else for checking different routes.
- The error handling for JSON unmarshaling of the Chains header could be more robust.
Example improvement for chains header parsing:
var chainIDs []int if err := json.Unmarshal([]byte(chainsHeader), &chainIDs); err != nil { logger.Warn("Failed to parse Chains header", "error", err) } else { for _, chainID := range chainIDs { destChainIDs = append(destChainIDs, uint32(chainID)) } }
433-496: WebSocket handling implemented, but needs refinement.The
GetActiveRFQWebsocketmethod successfully implements WebSocket connection handling for active quote requests. However, there are several areas for improvement:
- Replace
fmt.Printfstatements with proper logging using theloggervariable.- Implement more robust error handling, especially for the WebSocket upgrade process.
- Consider adding a timeout mechanism for inactive connections.
- Ensure proper cleanup of resources in case of errors during the setup process.
Example improvement for error handling:
ws, err := r.upgrader.Upgrade(c.Writer, c.Request, nil) if err != nil { logger.Error("Failed to upgrade WebSocket connection", "error", err) c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to establish WebSocket connection"}) return } defer ws.Close()
503-590: Quote request handling implemented well, but could be refactored for clarity.The
PutRFQRequestmethod successfully handles user quote requests for both active and passive RFQs. The implementation includes proper error handling, tracing, and response construction. However, the method is quite long and could benefit from refactoring:
- Consider extracting the active and passive RFQ handling logic into separate functions.
- The quote type determination could be simplified using a helper function.
- Response construction could be moved to a separate function for better readability.
Example refactoring:
func (r *QuoterAPIServer) PutRFQRequest(c *gin.Context) { // ... (request parsing and context setup) activeQuote := r.handleActiveRFQ(ctx, &req, requestID) passiveQuote, err := r.handlePassiveRFQ(ctx, &req) if err != nil { logger.Error("Error handling passive RFQ", "error", err) } bestQuote := getBestQuote(activeQuote, passiveQuote) resp := r.constructRFQResponse(bestQuote) c.JSON(http.StatusOK, resp) } func (r *QuoterAPIServer) constructRFQResponse(quote *model.QuoteData) model.PutRFQResponse { // ... (response construction logic) }This refactoring would improve the readability and maintainability of the code.
Line range hint
1-624: Overall, good implementation of RFQ and WebSocket functionality with room for improvement.The changes introduce robust support for RFQ (Request for Quote) handling and WebSocket connections, enhancing the server's capabilities. Key improvements include:
- New routes and handlers for RFQ and WebSocket connections.
- Support for both active and passive quote requests.
- Integration of tracing and metrics for better observability.
Main areas for improvement:
- Enhance WebSocket security by implementing proper origin checks.
- Replace debug print statements with structured logging.
- Refactor long methods (e.g.,
PutRFQRequest) for better readability and maintainability.- Improve error handling, especially in WebSocket connection management.
These changes significantly expand the server's functionality while maintaining good coding practices. Addressing the suggested improvements will further enhance the code's quality and security.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- services/rfq/api/rest/server.go (14 hunks)
🔇 Additional comments (1)
services/rfq/api/rest/server.go (1)
171-178: LGTM! New constants for API routes and headers.The addition of new constants for API routes and headers is appropriate and follows the existing naming conventions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
services/rfq/api/client/client.go (5)
64-65: Consider removing redundantrClientfieldThe
clientImplstruct now includes both an embeddedUnauthenticatedClientand an explicitrClientfield. This could lead to field shadowing and potential confusion. Consider removing the explicitrClientfield if it's already provided by the embeddedUnauthenticatedClient, or clarify the need for both if they serve different purposes.
95-112: LGTM: NewgetAuthHeaderfunction with minor suggestionThe
getAuthHeaderfunction is well-implemented, correctly using thereqSignerto generate a signed authentication header. The inclusion of a timestamp in both the signed message and the final header is a good practice for preventing replay attacks. The use of the Ethereum personal_sign standard is also appropriate.Consider wrapping the error returned from
reqSigner.SignMessageto provide more context:if err != nil { return "", fmt.Errorf("failed to sign authentication header: %w", err) }This will make debugging easier if an error occurs during signing.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-109: services/rfq/api/client/client.go#L108-L109
Added lines #L108 - L109 were not covered by tests
137-137: LGTM: UpdatedPutQuotemethod signatureThe
PutQuotemethod has been correctly updated to use*model.PutRelayerQuoteRequestinstead of*model.PutQuoteRequest. This change likely reflects a modification in the quote submission process, possibly to differentiate between relayer and user quotes.Don't forget to update any related documentation or comments that might reference the old
PutQuoteRequesttype to maintain consistency across the codebase.
181-220: LGTM with suggestions: NewSubscribeActiveQuotesmethod implementationThe
SubscribeActiveQuotesmethod is well-implemented, correctly establishing a WebSocket connection, subscribing to specified chains, and processing messages asynchronously. However, there are a few areas for improvement:
- Consider using a context with cancellation to manage the goroutine lifecycle:
ctx, cancel := context.WithCancel(ctx) defer cancel() go func() { wsErr := c.processWebsocket(ctx, conn, reqChan, respChan) if wsErr != nil { logger.Error("Error running websocket listener: %s", wsErr) } cancel() // Signal that the goroutine has finished }()
- Add error handling for the
conn.WriteJSONcall:if err := conn.WriteJSON(model.ActiveRFQMessage{ Op: rest.SubscribeOp, Content: json.RawMessage(subJSON), }); err != nil { return nil, fmt.Errorf("error sending subscribe message: %w", err) }
- Consider adding a timeout for the initial subscription response:
err = conn.SetReadDeadline(time.Now().Add(10 * time.Second)) if err != nil { return nil, fmt.Errorf("error setting read deadline: %w", err) } err = conn.ReadJSON(&resp) conn.SetReadDeadline(time.Time{}) // Reset deadline if err != nil { return nil, fmt.Errorf("error reading subscribe response: %w", err) }These changes will improve the robustness and error handling of the method.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 181-209: services/rfq/api/client/client.go#L181-L209
Added lines #L181 - L209 were not covered by tests
[warning] 211-216: services/rfq/api/client/client.go#L211-L216
Added lines #L211 - L216 were not covered by tests
[warning] 219-219: services/rfq/api/client/client.go#L219
Added line #L219 was not covered by tests
429-446: LGTM with suggestion: NewPutRFQRequestmethod implementationThe
PutRFQRequestmethod is well-implemented, correctly sending a PUT request to the RFQ API and handling the response. However, there's one area for improvement:Consider adding a nil check for the response before returning it:
if resp.IsError() { return nil, fmt.Errorf("error from server: %s", getStatus(resp)) } if response == nil { return nil, fmt.Errorf("received nil response from server") } return &response, nilThis change will prevent potential nil pointer dereferences if the server returns an unexpected nil response.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 429-439: services/rfq/api/client/client.go#L429-L439
Added lines #L429 - L439 were not covered by tests
[warning] 441-443: services/rfq/api/client/client.go#L441-L443
Added lines #L441 - L443 were not covered by tests
[warning] 445-445: services/rfq/api/client/client.go#L445
Added line #L445 was not covered by testsservices/rfq/api/rest/server.go (4)
436-483: LGTM: GetActiveRFQWebsocket implementation with a suggestionThe GetActiveRFQWebsocket function is well-implemented, handling WebSocket upgrades, client registration, and cleanup appropriately. It correctly ensures only one connection per relayer and includes proper error handling.
Suggestion for improvement:
Consider adding more detailed error responses for WebSocket upgrade failures. For example:ws, err := r.upgrader.Upgrade(c.Writer, c.Request, nil) if err != nil { logger.Error("Failed to set websocket upgrade", "error", err) c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to establish WebSocket connection"}) return }This will provide more informative feedback to the client in case of connection failures.
504-577: LGTM: PutRFQRequest implementation with refactoring suggestionThe PutRFQRequest function is well-implemented, handling both active and passive quote requests and constructing appropriate responses. The use of tracing for performance monitoring is a good practice.
Suggestion for improvement:
Consider refactoring this function to improve readability and maintainability. You can extract the active and passive quote handling logic into separate functions. For example:func (r *QuoterAPIServer) PutRFQRequest(c *gin.Context) { // ... (existing code for request binding and context setup) activeQuote := r.handleActiveRFQ(ctx, &req, requestID) passiveQuote, err := r.handlePassiveRFQ(ctx, &req) if err != nil { logger.Error("Error handling passive RFQ", "error", err) } quote := getBestQuote(activeQuote, passiveQuote) resp := r.constructRFQResponse(quote) c.JSON(http.StatusOK, resp) } func (r *QuoterAPIServer) handleActiveRFQ(ctx context.Context, req *model.PutRFQRequest, requestID string) *model.QuoteData { // ... (active quote handling logic) } func (r *QuoterAPIServer) handlePassiveRFQ(ctx context.Context, req *model.PutRFQRequest) (*model.QuoteData, error) { // ... (passive quote handling logic) } func (r *QuoterAPIServer) constructRFQResponse(quote *model.QuoteData) model.PutRFQResponse { // ... (response construction logic) }This refactoring would make the main function more concise and easier to understand, while also improving testability of individual components.
Line range hint
244-287: LGTM: AuthMiddleware updates with suggestion for improved error handlingThe changes to the AuthMiddleware function appropriately handle the new RFQ routes and correctly parse the ChainsHeader for RFQ and RFQStream routes. This update is necessary to support the new RFQ functionality.
Suggestion for improvement:
Consider enhancing the error handling when parsing the ChainsHeader. Currently, if there's an error in JSON unmarshaling, it's silently ignored. A more robust approach would be to return an error response in this case. For example:case RFQRoute, RFQStreamRoute: chainsHeader := c.GetHeader(ChainsHeader) if chainsHeader != "" { var chainIDs []int err = json.Unmarshal([]byte(chainsHeader), &chainIDs) if err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid Chains header format"}) c.Abort() return } for _, chainID := range chainIDs { destChainIDs = append(destChainIDs, uint32(chainID)) } }This change would provide clearer feedback when the Chains header is present but incorrectly formatted.
Line range hint
1-608: Overall assessment: Significant enhancements with some areas for improvementThe changes introduced in this file significantly enhance the RFQ system by adding WebSocket support and improving quote request handling. The new functionality is well-integrated with the existing codebase, and the naming conventions are consistent.
Key improvements:
- Addition of WebSocket support for real-time communication.
- Implementation of both active and passive quote handling in the PutRFQRequest function.
- Updates to the AuthMiddleware to support new RFQ routes.
Areas for further improvement:
- Implement a proper origin check for WebSocket connections to enhance security.
- Refactor the PutRFQRequest function to improve readability and maintainability.
- Enhance error handling in various parts of the code, particularly in the AuthMiddleware and WebSocket upgrade process.
Overall, these changes represent a significant step forward in the system's capabilities, with some opportunities for refinement to further improve security and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- services/rfq/api/client/client.go (8 hunks)
- services/rfq/api/rest/server.go (12 hunks)
- services/rfq/relayer/quoter/quoter.go (13 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/api/client/client.go
[warning] 82-82: services/rfq/api/client/client.go#L82
Added line #L82 was not covered by tests
[warning] 108-109: services/rfq/api/client/client.go#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 181-209: services/rfq/api/client/client.go#L181-L209
Added lines #L181 - L209 were not covered by tests
[warning] 211-216: services/rfq/api/client/client.go#L211-L216
Added lines #L211 - L216 were not covered by tests
[warning] 219-219: services/rfq/api/client/client.go#L219
Added line #L219 was not covered by tests
[warning] 222-225: services/rfq/api/client/client.go#L222-L225
Added lines #L222 - L225 were not covered by tests
[warning] 227-230: services/rfq/api/client/client.go#L227-L230
Added lines #L227 - L230 were not covered by tests
[warning] 232-240: services/rfq/api/client/client.go#L232-L240
Added lines #L232 - L240 were not covered by tests
[warning] 242-242: services/rfq/api/client/client.go#L242
Added line #L242 was not covered by tests
[warning] 245-257: services/rfq/api/client/client.go#L245-L257
Added lines #L245 - L257 were not covered by tests
[warning] 260-266: services/rfq/api/client/client.go#L260-L266
Added lines #L260 - L266 were not covered by tests
[warning] 269-292: services/rfq/api/client/client.go#L269-L292
Added lines #L269 - L292 were not covered by tests
[warning] 297-309: services/rfq/api/client/client.go#L297-L309
Added lines #L297 - L309 were not covered by tests
[warning] 313-321: services/rfq/api/client/client.go#L313-L321
Added lines #L313 - L321 were not covered by tests
[warning] 323-326: services/rfq/api/client/client.go#L323-L326
Added lines #L323 - L326 were not covered by tests
[warning] 331-336: services/rfq/api/client/client.go#L331-L336
Added lines #L331 - L336 were not covered by tests
[warning] 338-341: services/rfq/api/client/client.go#L338-L341
Added lines #L338 - L341 were not covered by tests
[warning] 343-343: services/rfq/api/client/client.go#L343
Added line #L343 was not covered by tests
[warning] 429-439: services/rfq/api/client/client.go#L429-L439
Added lines #L429 - L439 were not covered by tests
[warning] 441-443: services/rfq/api/client/client.go#L441-L443
Added lines #L441 - L443 were not covered by tests
[warning] 445-445: services/rfq/api/client/client.go#L445
Added line #L445 was not covered by tests
🔇 Additional comments (16)
services/rfq/api/client/client.go (3)
40-40: LGTM: NewSubscribeActiveQuotesmethod added toAuthenticatedClientinterfaceThe addition of the
SubscribeActiveQuotesmethod to theAuthenticatedClientinterface is a good enhancement. It provides a way for clients to subscribe to active quotes using WebSocket communication. The method signature is well-designed, using channels for bidirectional communication and returning an error for proper error handling.
50-50: LGTM: NewPutRFQRequestmethod added toUnauthenticatedClientinterfaceThe addition of the
PutRFQRequestmethod to theUnauthenticatedClientinterface is a good improvement. It provides a way for unauthenticated clients to submit RFQ requests. The method signature is well-designed, using context for cancellation and timeout handling, and returning both a response and an error for proper error handling.
Line range hint
70-91: LGTM: UpdatedNewAuthenticatedClientfunctionThe
NewAuthenticatedClientfunction has been appropriately updated to include areqSignerparameter, which is then correctly stored in theclientImplstruct. This change is consistent with the addition of thereqSignerfield to theclientImplstruct and supports the new WebSocket functionality for signing requests.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-82: services/rfq/api/client/client.go#L82
Added line #L82 was not covered by tests
[warning] 108-109: services/rfq/api/client/client.go#L108-L109
Added lines #L108 - L109 were not covered by testsservices/rfq/api/rest/server.go (2)
13-15: LGTM: New imports and struct fields for WebSocket supportThe addition of new imports (uuid, xsync, websocket) and struct fields (upgrader, wsClients, pubSubManager) are appropriate for implementing WebSocket functionality and managing client connections. These changes lay the groundwork for real-time communication in the RFQ system.
Also applies to: 56-56, 67-71
171-178: LGTM: New constants for RFQ routes and headersThe addition of new constants (RFQStreamRoute, RFQRoute, ChainsHeader, AuthorizationHeader) is appropriate for defining the new RFQ-related routes and headers. The naming is clear and consistent with the existing codebase style.
services/rfq/relayer/quoter/quoter.go (11)
Line range hint
1-37: LGTM: Import statements and package declaration look good.The new imports for "encoding/json" and "github.com/synapsecns/sanguine/services/rfq/api/rest" are appropriate for the added functionality.
47-48: LGTM: New method added to Quoter interface.The
SubscribeActiveRFQmethod has been added to the Quoter interface, which aligns with the PR objectives to introduce an active quoting API.
88-88: LGTM: Updated currentQuotes field type.The
currentQuotesfield in the Manager struct has been updated from[]model.PutQuoteRequestto[]model.PutRelayerQuoteRequest, which is consistent with the changes mentioned in the summary.
130-130: LGTM: Updated currentQuotes initialization.The initialization of
currentQuoteshas been correctly updated to use the new type[]model.PutRelayerQuoteRequest{}, which is consistent with the changes in the Manager struct.
260-299: LGTM: New SubscribeActiveRFQ method implemented correctly.The new
SubscribeActiveRFQmethod is well-implemented, with proper error handling, context cancellation, and tracing/metrics setup. It correctly sets up a subscription to active quotes and processes incoming messages in a loop.
301-361: LGTM: New generateActiveRFQ method implemented correctly.The
generateActiveRFQmethod is well-implemented, with proper error handling and tracing. It correctly unmarshals the incoming message, generates a quote, and prepares the response. The logic for processing the active RFQ message and generating the response is appropriate.
455-455: LGTM: Updated generateQuotes method signature and initialization.The
generateQuotesmethod has been correctly updated to use[]model.PutRelayerQuoteRequestas the return type, and the initialization of thequotesslice has been changed accordingly. These modifications are consistent with the changes made throughout the file.Also applies to: 474-474
Line range hint
812-820: LGTM: Updated submitQuote method parameter type.The
submitQuotemethod has been correctly updated to usemodel.PutRelayerQuoteRequestas the parameter type. This change is consistent with the modifications made throughout the file, and the rest of the method's logic remains unchanged.
Line range hint
824-835: LGTM: Updated submitBulkQuotes method parameter type.The
submitBulkQuotesmethod has been correctly updated to use[]model.PutRelayerQuoteRequestas the parameter type. This change is consistent with the modifications made throughout the file, and the rest of the method's logic remains unchanged.
296-296:⚠️ Potential issueCheck for nil before sending on channel to prevent panics.
When sending
resptoreqChan, there is a possibility thatrespisnil, especially ifgenerateActiveRFQreturnsnil, nil. Sending anilvalue on a channel might lead to unexpected behavior.Apply this diff to ensure
respis notnilbefore sending:- reqChan <- resp + if resp != nil { + reqChan <- resp + }Likely invalid or redundant comment.
545-545: LGTM: Updated generateQuote method signature and quote initialization.The
generateQuotemethod has been correctly updated to use*model.PutRelayerQuoteRequestas the return type, and the initialization of thequotevariable has been changed accordingly. These modifications are consistent with the changes made throughout the file.Please verify that all required fields of the
PutRelayerQuoteRequeststruct are being set correctly. Run the following command to check the struct definition:Also applies to: 579-589
| r.upgrader = websocket.Upgrader{ | ||
| CheckOrigin: func(_ *http.Request) bool { | ||
| return true // TODO: Implement a more secure check | ||
| }, | ||
| } |
There was a problem hiding this comment.
Security risk: Implement proper origin check for WebSocket connections
The current WebSocket upgrader configuration allows connections from any origin, which poses a security risk. This can lead to cross-site WebSocket hijacking attacks.
Implement a proper origin check by replacing the current CheckOrigin function with a more secure version. For example:
r.upgrader = websocket.Upgrader{
CheckOrigin: func(r *http.Request) bool {
origin := r.Header.Get("Origin")
// Check if the origin is in the whitelist of allowed origins
allowedOrigins := []string{"https://trusted-origin.com"} // Add your allowed origins
for _, allowed := range allowedOrigins {
if origin == allowed {
return true
}
}
return false
},
}Replace "https://trusted-origin.com" with your actual allowed origins.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation