feat(rfq-relayer): relayer supports active quoting #3198
Conversation
|
Warning Rate limit exceeded@dwasse has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce enhancements to the WebSocket subscription functionality within the RFQ service. Key updates include improved logging and error handling across various methods in both the 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
services/rfq/relayer/relconfig/config.go (1)
70-71: LGTM! Consider enhancing the comment slightly.The addition of the
SupportsActiveRFQfield is well-implemented. It follows the existing coding style and naming conventions. The boolean type is appropriate for this feature flag.Consider slightly expanding the comment to provide more context:
- // SupportsActiveRFQ indicates whether the chain supports active RFQ. + // SupportsActiveRFQ indicates whether the relayer supports active Request for Quote (RFQ) functionality.This minor change provides a bit more clarity about what "RFQ" stands for and that it's a feature of the relayer.
services/rfq/relayer/service/relayer.go (1)
222-230: LGTM with suggestions for improvementThe implementation of active RFQ support looks good. However, I have a few suggestions to enhance the code:
Consider adding a comment explaining the purpose and functionality of active RFQ for better code documentation.
It might be beneficial to implement a graceful shutdown mechanism for this new goroutine. For example:
if r.cfg.SupportsActiveRFQ { g.Go(func() error { subscriptionDone := make(chan struct{}) go func() { defer close(subscriptionDone) err = r.quoter.SubscribeActiveRFQ(ctx) if err != nil { logger.Errorf("Active RFQ subscription error: %v", err) } }() select { case <-ctx.Done(): return ctx.Err() case <-subscriptionDone: return fmt.Errorf("could not subscribe to active RFQ: %w", err) } }) }
- Consider improving error handling. The current implementation will cause the entire
Startmethod to return if the subscription fails. You might want to log the error and continue running other parts of the system, depending on your requirements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- services/rfq/relayer/quoter/quoter.go (4 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/service/relayer.go (1 hunks)
🔇 Additional comments (1)
services/rfq/relayer/relconfig/config.go (1)
70-71: Verify handling of the new field in related codeWhile the addition of the
SupportsActiveRFQfield is straightforward, it's important to ensure that this new configuration option is properly handled throughout the codebase.Please run the following script to check for potential areas that might need updates:
Based on the results, you may need to:
- Update the
LoadConfigfunction to handle the new field.- Modify the
Validatemethod if any specific validation is required for this field.- Update any code that uses this configuration to implement the active RFQ functionality.
✅ Verification successful
SupportsActiveRFQ is properly handled in the relayer service
The
SupportsActiveRFQfield is correctly defined and utilized within therelayerservice. No additional updates are required in other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential areas that might need updates due to the new SupportsActiveRFQ field # Search for uses of the Config struct echo "Searching for uses of the Config struct:" rg --type go "Config\s+struct" -A 10 # Search for places where config is loaded or validated echo "Searching for config loading and validation:" rg --type go "LoadConfig|Validate.*Config" # Search for places where active RFQ functionality might be implemented echo "Searching for potential active RFQ implementations:" rg --type go "active.*RFQ|RFQ.*active"Length of output: 48967
Script:
#!/bin/bash # Description: Verify handling of the SupportsActiveRFQ field in configuration loading and validation. # Search for references to SupportsActiveRFQ in the codebase echo "Searching for references to SupportsActiveRFQ:" rg --type go "SupportsActiveRFQ"Length of output: 411
| // SubscribeActiveRFQ subscribes to the RFQ websocket API. | ||
| SubscribeActiveRFQ(ctx context.Context) (err error) |
There was a problem hiding this comment.
Adding a new method to public interface Quoter may introduce breaking changes
Adding SubscribeActiveRFQ(ctx context.Context) (err error) to the Quoter interface can break existing implementations. Any external packages implementing Quoter will now fail to compile until they implement this new method. Consider the impact on external dependencies and possibly introduce a new interface to extend Quoter.
| case msg := <-respChan: | ||
| resp, err := m.generateActiveRFQ(ctx, msg) | ||
| if err != nil { | ||
| return fmt.Errorf("error generating active RFQ message: %w", err) | ||
| } | ||
| respChan <- resp |
There was a problem hiding this comment.
Potential deadlock: Sending responses to respChan instead of reqChan
In the select loop, you are sending responses back to respChan (line 283), which is the channel from which you receive messages. This could cause a deadlock or unexpected behavior. You should send responses to reqChan instead.
Apply this diff to fix the issue:
case msg := <-respChan:
resp, err := m.generateActiveRFQ(ctx, msg)
if err != nil {
return fmt.Errorf("error generating active RFQ message: %w", err)
}
- respChan <- resp
+ 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.
| case msg := <-respChan: | |
| resp, err := m.generateActiveRFQ(ctx, msg) | |
| if err != nil { | |
| return fmt.Errorf("error generating active RFQ message: %w", err) | |
| } | |
| respChan <- resp | |
| case msg := <-respChan: | |
| resp, err := m.generateActiveRFQ(ctx, msg) | |
| if err != nil { | |
| return fmt.Errorf("error generating active RFQ message: %w", err) | |
| } | |
| reqChan <- resp |
| OriginBalance: inv[rfqRequest.Data.OriginChainID][common.HexToAddress(rfqRequest.Data.OriginTokenAddr)], | ||
| DestBalance: inv[rfqRequest.Data.DestChainID][common.HexToAddress(rfqRequest.Data.DestTokenAddr)], |
There was a problem hiding this comment.
Possible nil map access without existence checks
Accessing inv maps without checking if the keys exist can lead to a panic if the keys are missing. Before accessing inv[rfqRequest.Data.OriginChainID][...] and inv[rfqRequest.Data.DestChainID][...], ensure that both the chain ID and token address keys exist in the maps.
Apply this diff to safely access the map entries:
quoteInput := QuoteInput{
OriginChainID: rfqRequest.Data.OriginChainID,
DestChainID: rfqRequest.Data.DestChainID,
OriginTokenAddr: common.HexToAddress(rfqRequest.Data.OriginTokenAddr),
DestTokenAddr: common.HexToAddress(rfqRequest.Data.DestTokenAddr),
- OriginBalance: inv[rfqRequest.Data.OriginChainID][common.HexToAddress(rfqRequest.Data.OriginTokenAddr)],
- DestBalance: inv[rfqRequest.Data.DestChainID][common.HexToAddress(rfqRequest.Data.DestTokenAddr)],
+ OriginBalance: func() *big.Int {
+ if chainBalances, ok := inv[rfqRequest.Data.OriginChainID]; ok {
+ return chainBalances[common.HexToAddress(rfqRequest.Data.OriginTokenAddr)]
+ }
+ return nil
+ }(),
+ DestBalance: func() *big.Int {
+ if chainBalances, ok := inv[rfqRequest.Data.DestChainID]; ok {
+ return chainBalances[common.HexToAddress(rfqRequest.Data.DestTokenAddr)]
+ }
+ return nil
+ }(),
}
if quoteInput.OriginBalance == nil || quoteInput.DestBalance == nil {
return nil, fmt.Errorf("insufficient inventory balances for the provided chain IDs and token addresses")
}📝 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.
| OriginBalance: inv[rfqRequest.Data.OriginChainID][common.HexToAddress(rfqRequest.Data.OriginTokenAddr)], | |
| DestBalance: inv[rfqRequest.Data.DestChainID][common.HexToAddress(rfqRequest.Data.DestTokenAddr)], | |
| OriginBalance: func() *big.Int { | |
| if chainBalances, ok := inv[rfqRequest.Data.OriginChainID]; ok { | |
| return chainBalances[common.HexToAddress(rfqRequest.Data.OriginTokenAddr)] | |
| } | |
| return nil | |
| }(), | |
| DestBalance: func() *big.Int { | |
| if chainBalances, ok := inv[rfqRequest.Data.DestChainID]; ok { | |
| return chainBalances[common.HexToAddress(rfqRequest.Data.DestTokenAddr)] | |
| } | |
| return nil | |
| }(), |
Deploying sanguine-fe with
|
| Latest commit: |
ab15e5d
|
| Status: | ⚡️ Build in progress... |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/active-rfq-api #3198 +/- ##
=============================================================
+ Coverage 33.16580% 33.17728% +0.01148%
=============================================================
Files 539 539
Lines 34421 34391 -30
Branches 82 82
=============================================================
- Hits 11416 11410 -6
+ Misses 21987 21964 -23
+ Partials 1018 1017 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
services/rfq/relayer/service/relayer.go (2)
133-133: LGTM: WebSocket URL support added for RFQ API clientThe addition of
cfg.GetRFQWsURL()as a parameter toNewAuthenticatedClientis a good step towards supporting active quoting. This change aligns with the PR objectives.Consider adding a check to ensure that at least one of
cfg.GetRFQAPIURL()orcfg.GetRFQWsURL()is not nil before callingNewAuthenticatedClient. This would prevent potential issues if both URLs are mistakenly left unconfigured.
222-230: LGTM: Active RFQ subscription implementedThe addition of the conditional block to subscribe to active RFQs when a WebSocket URL is provided is a good implementation. This change directly supports the PR objective of enabling active quoting.
Consider the following improvements:
- Add logging when the subscription starts and ends (or encounters an error).
- Implement a retry mechanism with backoff for the subscription in case of temporary failures.
- Consider moving the error handling logic outside the goroutine to ensure errors are properly propagated to the errgroup.
Example:
if r.cfg.GetRFQWsURL() != nil { g.Go(func() error { logger.Info("Starting active RFQ subscription") err := retry.Do( func() error { return r.quoter.SubscribeActiveRFQ(ctx) }, retry.WithContext(ctx), retry.WithMaxRetries(3), retry.WithDelay(time.Second), retry.WithErrorInLog(logger), ) if err != nil { logger.Error("Failed to subscribe to active RFQ", "error", err) return fmt.Errorf("could not subscribe to active RFQ: %w", err) } logger.Info("Active RFQ subscription ended") return nil }) }This approach provides better visibility into the subscription process and improves resilience.
services/rfq/relayer/relconfig/getters.go (1)
549-551: Approved: New function added for RFQ WebSocket URL.The
GetRFQWsURLfunction is a good addition, providing access to the RFQ WebSocket URL configuration. Returning a pointer to a string allows for optional configuration.Consider adding a comment to explain the purpose of this function and why it returns a pointer. For example:
// GetRFQWsURL returns the RFQ WebSocket URL. // It returns a pointer to allow for an optional configuration. func (c Config) GetRFQWsURL() *string { return c.RFQWsURL }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- services/rfq/e2e/setup_test.go (1 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
- services/rfq/relayer/service/relayer.go (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/rfq/e2e/setup_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/relconfig/config.go
🔇 Additional comments (1)
services/rfq/relayer/relconfig/getters.go (1)
544-547: Approved: Function name change improves consistency.The renaming of
GetRfqAPIURLtoGetRFQAPIURLaligns better with Go naming conventions for acronyms. The functionality remains unchanged, which is good for backward compatibility.Please verify that all calls to this function throughout the codebase have been updated to use the new name. You can use the following command to check for any remaining occurrences of the old function name:
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
services/rfq/api/rest/server.go (1)
Line range hint
1-624: Overall assessment: Logging improvements neededThe changes introduced in this file are focused on adding debug log statements, which can be helpful during development. However, to maintain code quality and follow best practices, consider the following general recommendations for logging:
- Standardize logging: Use a consistent logging framework throughout the codebase.
- Use log levels: Implement proper log levels (DEBUG, INFO, WARN, ERROR) to categorize log messages appropriately.
- Add context: Include relevant context in log messages (e.g., function parameters, request IDs) to make debugging easier.
- Configure for environments: Implement a mechanism to adjust log verbosity based on the environment (development, staging, production).
- Performance consideration: Ensure that debug logs can be easily disabled in production to avoid any performance impact.
By following these practices, you can improve the overall quality and usefulness of your logging system while maintaining good performance in production.
services/rfq/api/client/client.go
Outdated
| fmt.Printf("dialing websocket: %s\n", reqURL) | ||
| fmt.Printf("headers: %v\n", header) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve logging approach for WebSocket connection details
While the added print statements can be helpful for debugging, using fmt.Printf directly is not ideal for production code. Consider the following improvements:
- Use the existing
loggervariable (which is an instance oflog.Logger) instead offmt.Printf. This allows for better log management and potential log level control. - Consider logging at a debug or trace level to avoid cluttering production logs.
- Be cautious about logging headers, as they may contain sensitive information.
Here's a suggested refactor:
- fmt.Printf("dialing websocket: %s\n", reqURL)
- fmt.Printf("headers: %v\n", header)
+ logger.Debugf("Dialing WebSocket: %s", reqURL)
+ // Log headers selectively or at a lower log level
+ logger.Tracef("WebSocket headers: %v", header)Also, consider adding a helper function to sanitize headers before logging to ensure sensitive information is not exposed.
📝 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.
| fmt.Printf("dialing websocket: %s\n", reqURL) | |
| fmt.Printf("headers: %v\n", header) | |
| logger.Debugf("Dialing WebSocket: %s", reqURL) | |
| // Log headers selectively or at a lower log level | |
| logger.Tracef("WebSocket headers: %v", header) |
services/rfq/api/rest/server.go
Outdated
| fmt.Println("RFQRoute, RFQStreamRoute") | ||
| chainsHeader := c.GetHeader(ChainsHeader) | ||
| fmt.Printf("got chains header: %s\n", chainsHeader) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a proper logging framework and removing debug logs.
While adding logs can be helpful for debugging, using fmt.Println and fmt.Printf directly is not ideal for production code. Consider the following improvements:
- Use a structured logging framework (e.g., logrus, zap) instead of fmt for better log management and performance.
- Use appropriate log levels (e.g., debug, info) instead of print statements.
- Consider making these logs conditional based on a debug flag to avoid unnecessary logging in production.
Example using the existing logger:
if debugMode {
logger.Debug("RFQRoute or RFQStreamRoute hit")
logger.Debugf("Got chains header: %s", chainsHeader)
}
services/rfq/api/rest/server.go
Outdated
| @@ -434,6 +436,7 @@ | |||
| // @Header 101 {string} X-Api-Version "API Version Number - See docs for more info" | |||
| // @Router /quote_requests [get]. | |||
| func (r *QuoterAPIServer) GetActiveRFQWebsocket(ctx context.Context, c *gin.Context) { | |||
| fmt.Println("GetActiveRFQWebsocket") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance logging with proper framework and more context.
The current debug print statement can be improved for better logging practices:
- Replace
fmt.Printlnwith a proper logging framework, preferably the same one used elsewhere in the codebase (e.g., theloggervariable). - Consider adding more context to the log, such as the relayer address or any relevant request parameters.
- Use appropriate log levels to allow for easy filtering in different environments.
Example improvement:
logger.WithFields(log.Fields{
"relayerAddr": relayerAddr,
"remoteAddr": c.Request.RemoteAddr,
}).Debug("Handling WebSocket connection for active quote requests")Also, consider wrapping this log in a debug flag check to avoid unnecessary logging in production environments.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
services/rfq/api/rest/ws.go (3)
Line range hint
235-255: Add logging tohandleUnsubscribefor consistency withhandleSubscribe.While no changes were made to the
handleUnsubscribemethod, it's worth noting that it doesn't include the same level of logging as thehandleSubscribemethod. For consistency and improved observability, consider adding similar logging statements to this method.Here's a suggested refactor to add logging to the
handleUnsubscribemethod:func (c *wsClient) handleUnsubscribe(ctx context.Context, content json.RawMessage) (resp model.ActiveRFQMessage) { log.Printf("Handling unsubscribe request: %s", content) ctx, span := c.handler.Tracer().Start(ctx, "handleUnsubscribe", trace.WithAttributes( attribute.String("relayer_address", c.relayerAddr), )) defer func() { metrics.EndSpan(span) }() var sub model.SubscriptionParams err := json.Unmarshal(content, &sub) if err != nil { log.Printf("Error unmarshaling subscription params: %v", err) return getErrorResponse(UnsubscribeOp, fmt.Errorf("could not unmarshal subscription params: %w", err)) } span.SetAttributes(attribute.IntSlice("chain_ids", sub.Chains)) err = c.pubsub.RemoveSubscription(c.relayerAddr, sub) if err != nil { log.Printf("Error removing subscription: %v", err) return getErrorResponse(UnsubscribeOp, err) } log.Printf("Successfully removed subscription for relayer %s, chain IDs: %v", c.relayerAddr, sub.Chains) return getSuccessResponse(UnsubscribeOp) }This change will bring the
handleUnsubscribemethod in line with the logging practices used inhandleSubscribe, improving the overall consistency and observability of the WebSocket client.
Line range hint
257-288: EnhancehandleSendQuotewith logging and improved error handling.While no changes were made to the
handleSendQuotemethod, it lacks logging statements and could benefit from improved error handling. Adding logging would improve observability, and enhancing error handling would provide more context for troubleshooting.Here's a suggested refactor to add logging and improve error handling in the
handleSendQuotemethod:func (c *wsClient) handleSendQuote(ctx context.Context, content json.RawMessage) (err error) { log.Printf("Handling send quote request: %s", content) ctx, span := c.handler.Tracer().Start(ctx, "handleSendQuote", trace.WithAttributes( attribute.String("relayer_address", c.relayerAddr), )) defer func() { metrics.EndSpan(span) }() var resp model.WsRFQResponse err = json.Unmarshal(content, &resp) if err != nil { log.Printf("Error unmarshaling websocket message: %v", err) return fmt.Errorf("error unmarshaling websocket message: %w", err) } span.SetAttributes( attribute.String("request_id", resp.RequestID), attribute.String("dest_amount", resp.DestAmount), ) responseChan, ok := c.responseChans.Load(resp.RequestID) if !ok { err := fmt.Errorf("no response channel for request %s", resp.RequestID) log.Printf("Error: %v", err) return err } log.Printf("Sending quote response for request ID: %s", resp.RequestID) responseChan <- &resp return nil }This refactor adds logging statements to track the flow of the method and improves error handling by logging errors before returning them. It also adds a log statement when successfully sending the quote response, which can be useful for debugging and monitoring the system's behavior.
Line range hint
1-324: Improve overall consistency and structure of the WebSocket client implementation.After reviewing the entire file, there are several opportunities to enhance the overall consistency and structure of the WebSocket client implementation:
Logging: Implement a consistent logging strategy across all methods. Currently, some methods have detailed logging (e.g.,
Run,handleSubscribe) while others have none (e.g.,handleUnsubscribe,handleSendQuote).Error Handling: Standardize error handling across all methods. Consider creating custom error types for different scenarios to provide more context and make error handling more consistent.
Context Usage: Ensure consistent use of context across all methods, particularly for cancellation and timeout handling.
Code Organization: Consider grouping related methods together and adding clear comments to delineate different sections of the file (e.g., main loop, message handlers, utility functions).
Configuration: Extract hard-coded values (like
PingPeriod) into a configuration struct that can be passed to the client on initialization.Here's a high-level outline of how you might restructure the file:
package rest import ( // ... (imports) ) // Configuration struct type WsClientConfig struct { PingPeriod time.Duration // ... other configuration options } // WsClient interface (as before) type wsClient struct { // ... (fields as before) config WsClientConfig } // Constructor func NewWsClient(relayerAddr string, conn *websocket.Conn, pubsub PubSubManager, handler metrics.Handler, config WsClientConfig) *wsClient { // ... (initialization) } // Main loop func (c *wsClient) Run(ctx context.Context) error { // ... (implementation with consistent logging and error handling) } // Message sending methods func (c *wsClient) SendQuoteRequest(ctx context.Context, quoteRequest *model.WsRFQRequest) error { // ... (implementation with consistent logging and error handling) } // Message handling methods func (c *wsClient) handleRelayerMessage(ctx context.Context, msg []byte) error { // ... (implementation with consistent logging and error handling) } func (c *wsClient) handleSubscribe(ctx context.Context, content json.RawMessage) model.ActiveRFQMessage { // ... (implementation with consistent logging and error handling) } func (c *wsClient) handleUnsubscribe(ctx context.Context, content json.RawMessage) model.ActiveRFQMessage { // ... (implementation with consistent logging and error handling) } func (c *wsClient) handleSendQuote(ctx context.Context, content json.RawMessage) error { // ... (implementation with consistent logging and error handling) } // Utility methods func (c *wsClient) trySendPing(lastPong time.Time) error { // ... (implementation with consistent logging and error handling) } // ... (other utility functions)By implementing these changes, you'll create a more consistent, maintainable, and robust WebSocket client implementation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- services/rfq/api/rest/server.go (4 hunks)
- services/rfq/api/rest/ws.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/api/rest/server.go
services/rfq/api/rest/ws.go
Outdated
| @@ -96,6 +96,7 @@ const ( | |||
|
|
|||
| // Run runs the WebSocket client. | |||
| func (c *wsClient) Run(ctx context.Context) (err error) { | |||
| fmt.Println("running ws client") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve logging practices for better observability and consistency.
While the added logging statements enhance visibility into the WebSocket client's operations, there are a few areas for improvement:
- Replace
fmt.Printfwith a proper logging framework for better control over log levels and output formatting. - Avoid duplicating error logs. At lines 121 and 128, you're using both
fmt.Printfandlogger.Error. Stick to one consistent logging method. - Consider adding log levels (e.g., DEBUG, INFO, ERROR) to differentiate between informational and error logs.
Here's a suggested refactor for improved logging:
import "log" // Or your preferred logging framework
// ...
func (c *wsClient) Run(ctx context.Context) (err error) {
log.Println("Running ws client")
// ...
case req := <-c.requestChan:
log.Printf("Sending quote request: %v", req)
err = c.sendRelayerRequest(ctx, req)
if err != nil {
log.Printf("Error sending quote request: %v", err)
}
case msg := <-messageChan:
log.Printf("Received message: %s", msg)
err = c.handleRelayerMessage(ctx, msg)
if err != nil {
log.Printf("Error handling relayer message: %v", err)
}
// ...
}This approach provides consistent logging and avoids duplication while maintaining the improved visibility into the WebSocket client's operations.
Also applies to: 118-118, 121-121, 125-125, 128-128
services/rfq/api/rest/ws.go
Outdated
| @@ -204,6 +209,7 @@ | |||
| } | |||
|
|
|||
| func (c *wsClient) handleSubscribe(ctx context.Context, content json.RawMessage) (resp model.ActiveRFQMessage) { | |||
| fmt.Printf("handleSubscribe: %s\n", content) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance logging and error handling in the handleSubscribe method.
The added logging improves visibility into the subscription process, but there are opportunities for improvement:
- Replace
fmt.Printfwith a proper logging framework for consistency with the rest of the application. - Avoid duplicating error information. The error at line 228 is both logged and returned in the error response.
- Consider adding more context to the success log at line 231, such as the relayer address.
Here's a suggested refactor for improved logging and error handling:
func (c *wsClient) handleSubscribe(ctx context.Context, content json.RawMessage) (resp model.ActiveRFQMessage) {
log.Printf("Handling subscribe request: %s", content)
// ...
err = c.pubsub.AddSubscription(c.relayerAddr, sub)
if err != nil {
log.Printf("Error adding subscription: %v", err)
return getErrorResponse(SubscribeOp, err)
}
log.Printf("Successfully added subscription for relayer %s, chain IDs: %v", c.relayerAddr, sub.Chains)
return getSuccessResponse(SubscribeOp)
}This refactor provides more consistent and informative logging while avoiding duplication of error information.
Also applies to: 228-228, 231-231
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- services/rfq/api/client/client.go (10 hunks)
- services/rfq/api/rest/rfq.go (2 hunks)
- services/rfq/api/rest/server.go (10 hunks)
- services/rfq/api/rest/ws.go (7 hunks)
- services/rfq/relayer/quoter/quoter.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- services/rfq/api/client/client.go
- services/rfq/api/rest/server.go
- services/rfq/api/rest/ws.go
- services/rfq/relayer/quoter/quoter.go
🔇 Additional comments (1)
services/rfq/api/rest/rfq.go (1)
100-100: Confirm correct error reporting withmetrics.EndSpanWithErrAfter resolving the variable shadowing issue,
metrics.EndSpanWithErr(clientSpan, err)will correctly capture any errors that occur duringReceiveQuoteResponse, ensuring accurate error reporting in your metrics.
services/rfq/api/rest/rfq.go
Outdated
| span.AddEvent("received quote response", trace.WithAttributes( | ||
| attribute.String("relayer_address", relayerAddr), | ||
| attribute.String("request_id", requestID), | ||
| attribute.String("dest_amount", resp.DestAmount), | ||
| )) |
There was a problem hiding this comment.
Use clientSpan instead of parent span within goroutine to prevent data races
Accessing the parent span from within a goroutine can lead to data races because OpenTelemetry spans are not concurrency-safe. Instead, you should use the clientSpan created within the goroutine to add events.
Apply this diff to fix the issue:
- span.AddEvent("received quote response", trace.WithAttributes(
+ clientSpan.AddEvent("received quote response", trace.WithAttributes(
attribute.String("relayer_address", relayerAddr),
attribute.String("request_id", requestID),
attribute.String("dest_amount", resp.DestAmount),
))📝 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.
| span.AddEvent("received quote response", trace.WithAttributes( | |
| attribute.String("relayer_address", relayerAddr), | |
| attribute.String("request_id", requestID), | |
| attribute.String("dest_amount", resp.DestAmount), | |
| )) | |
| clientSpan.AddEvent("received quote response", trace.WithAttributes( | |
| attribute.String("relayer_address", relayerAddr), | |
| attribute.String("request_id", requestID), | |
| attribute.String("dest_amount", resp.DestAmount), | |
| )) |
| @@ -90,13 +90,14 @@ | |||
| wg.Add(1) | |||
| go func(client WsClient) { | |||
| var respStatus db.ActiveQuoteResponseStatus | |||
| var err error | |||
There was a problem hiding this comment.
Fix variable shadowing of err to ensure correct error handling
The use of := in resp, err := client.ReceiveQuoteResponse(collectionCtx, requestID) shadows the err variable declared earlier at line 93. This means that the err used in the deferred function may remain nil, and any error returned by ReceiveQuoteResponse won't be properly logged or passed to metrics.EndSpanWithErr. To fix this, replace := with = to assign to the existing err variable.
Apply this diff to fix the issue:
- resp, err := client.ReceiveQuoteResponse(collectionCtx, requestID)
+ resp, err = client.ReceiveQuoteResponse(collectionCtx, requestID)Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
services/rfq/relayer/quoter/quoter.go (4)
262-262: Use structured logging instead offmt.PrintfstatementsUsing
fmt.Printffor logging is not recommended in production code as it writes directly to standard output without any log levels or formatting. Instead, consider using theloggerinstance for consistent and configurable logging throughout the application.Apply this diff to replace
fmt.Printfwithlogger:- fmt.Printf("%s subscribing to active RFQ\n", time.Now().String()) + logger.Info("subscribing to active RFQ", "time", time.Now().String()) - fmt.Printf("%s subscribed to active quotes\n", time.Now().String()) + logger.Info("subscribed to active quotes", "time", time.Now().String()) - fmt.Printf("%s context done for ws\n", time.Now().String()) + logger.Info("context done for websocket", "time", time.Now().String()) - fmt.Printf("%s got message: %v\n", time.Now().String(), msg) + logger.Info("received message", "time", time.Now().String(), "message", msg) - fmt.Printf("%s respChan closed\n", time.Now().String()) + logger.Info("response channel closed", "time", time.Now().String()) - fmt.Printf("%s generated response: %v\n", time.Now().String(), resp) + logger.Info("generated response", "time", time.Now().String(), "response", resp)Also applies to: 283-283, 288-288, 291-291, 293-293, 303-303
311-311: Use structured logging instead offmt.PrintfstatementsSimilarly, in the
generateActiveRFQfunction, replacefmt.Printfwithloggerfor consistent logging practices.Apply this diff:
- fmt.Printf("%s generating active RFQ\n", time.Now().String()) + logger.Info("generating active RFQ", "time", time.Now().String())
309-309: Update the function comment to match the function nameThe comment above
generateActiveRFQrefers togetActiveRFQ, which seems outdated or incorrect. Update the comment to accurately reflect the function's purpose and name.Apply this diff:
-// getActiveRFQ handles an active RFQ message. +// generateActiveRFQ processes an active RFQ message.
350-350: Avoid setting the same attribute multiple timesThe
dest_amountattribute is being set twice on the span, which is redundant. Setting it once is sufficient.Apply this diff:
span.SetAttributes(attribute.String("dest_amount", rawQuote.DestAmount)) - span.SetAttributes(attribute.String("dest_amount", rawQuote.DestAmount))Also applies to: 356-356
| if err != nil { | ||
| return fmt.Errorf("error generating active RFQ message: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors without terminating the subscription loop
Returning an error here will exit the SubscribeActiveRFQ method, terminating the subscription loop. Consider logging the error and continuing to process other messages to ensure that a single failure does not stop the entire subscription.
Apply this diff to adjust error handling:
resp, err := m.generateActiveRFQ(ctx, msg)
if err != nil {
- return fmt.Errorf("error generating active RFQ message: %w", err)
+ span.RecordError(err)
+ logger.Error("error generating active RFQ message", "error", err)
+ continue
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
services/rfq/api/model/response.go (3)
59-63: LGTM! Consider adding field descriptions.The changes to
PutUserQuoteResponselook good and align with the PR objective of supporting active quoting. The new fieldsDestAmountandRelayerAddressprovide more detailed information, while makingReasonandQuoteTypeoptional allows for more flexible responses.Consider adding brief descriptions for the new fields to improve code documentation:
type PutUserQuoteResponse struct { Success bool `json:"success"` Reason string `json:"reason,omitempty"` QuoteType string `json:"quote_type,omitempty"` - DestAmount string `json:"dest_amount,omitempty"` - RelayerAddress string `json:"relayer_address,omitempty"` + // DestAmount is the amount of tokens to be received at the destination + DestAmount string `json:"dest_amount,omitempty"` + // RelayerAddress is the address of the relayer providing the quote + RelayerAddress string `json:"relayer_address,omitempty"` }
69-69: LGTM! Consider grouping optional fields.The change to make
QuoteIDoptional inWsRFQResponselooks good. It allows for more flexible WebSocket responses, which could be beneficial for the active quoting feature.For better code organization, consider grouping optional fields together. This can improve readability and make it easier to identify which fields are required vs optional:
type WsRFQResponse struct { RequestID string `json:"request_id"` - QuoteID string `json:"quote_id,omitempty"` DestAmount string `json:"dest_amount"` UpdatedAt time.Time `json:"updated_at"` + QuoteID string `json:"quote_id,omitempty"` }
Line range hint
1-93: Consider updating API documentation.The changes in this file improve the flexibility of the API responses and support the new active quoting feature. Great job on implementing these modifications!
To ensure consistency across the codebase and to help other developers understand these changes:
- Update any existing API documentation to reflect the new optional fields and their purposes.
- If there's a changelog, make sure to include these modifications in the next release notes.
- Consider updating any client-side code that might be affected by these changes, especially regarding the handling of the new optional fields.
services/rfq/api/db/sql/base/store.go (1)
Line range hint
1-153: Summary of changes in store.goThe modifications in this file are minimal and focused on updating terminology in the
UpdateActiveQuoteRequestStatusmethod. The change from "fulfilled" to "closed" improves consistency with thedb.Closedstatus. The overall structure and functionality of theStorestruct remain unchanged, maintaining the existing methods for managing quotes and requests.Consider reviewing any dependent code or database schemas that might be affected by this terminology change to ensure consistency across the entire system.
services/rfq/api/db/api_db.go (1)
Line range hint
1-257: Overall, the changes improve theActiveQuoteRequestmodel.The modifications to
ClosedAtandClosedQuoteIDin theActiveQuoteRequeststruct enhance the model's ability to represent the state of a quote request accurately. These changes align well with the RFQ system's design as outlined in the file.However, it's crucial to ensure that these changes are reflected appropriately throughout the entire RFQ system, including:
- Database queries and migrations
- Business logic handling
ActiveQuoteRequestinstances- API responses involving these fields
Consider reviewing and updating related components to maintain consistency and prevent potential nil pointer dereferences.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- services/rfq/api/db/api_db.go (1 hunks)
- services/rfq/api/db/sql/base/store.go (1 hunks)
- services/rfq/api/model/response.go (1 hunks)
- services/rfq/api/rest/rfq.go (3 hunks)
- services/rfq/api/rest/rfq_test.go (4 hunks)
- services/rfq/api/rest/server.go (10 hunks)
- services/rfq/api/rest/ws.go (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/rfq/api/rest/rfq.go
- services/rfq/api/rest/server.go
- services/rfq/api/rest/ws.go
🔇 Additional comments (2)
services/rfq/api/db/sql/base/store.go (1)
105-106: LGTM! Terminology update from "fulfilled" to "closed".The changes in field names from
fulfilled_quote_idtoclosed_quote_idandfulfilled_attoclosed_atalign well with thedb.Closedstatus used in this method. This update improves consistency in terminology.To ensure this change doesn't introduce inconsistencies, please run the following script to check for any remaining uses of "fulfilled" in relation to quote requests:
If the script returns any results, consider updating those occurrences to maintain consistency with this change.
services/rfq/api/db/api_db.go (1)
156-157: LGTM! Consider verifying usage across the codebase.The changes to make
ClosedAtandClosedQuoteIDnullable by using pointer types are appropriate. This allows for a more accurate representation of an unclosed quote request.However, it's important to ensure that all code interacting with these fields has been updated to handle potential nil values.
To verify the impact and usage of these fields across the codebase, run the following script:
This script will help identify areas of the codebase that might need updates to handle the new nullable types.
✅ Verification successful
Verified: No usage of
ClosedAtandClosedQuoteIDfound elsewhere in the codebase.The changes to make
ClosedAtandClosedQuoteIDnullable are appropriately isolated. No additional updates are necessary as there are no existing usages that require handling ofnilvalues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of ClosedAt and ClosedQuoteID fields echo "Checking usages of ClosedAt:" rg --type go -A 5 "ClosedAt" echo "Checking usages of ClosedQuoteID:" rg --type go -A 5 "ClosedQuoteID" echo "Checking for potential nil checks:" rg --type go -A 5 -e "if.*ClosedAt.*!= nil" -e "if.*ClosedQuoteID.*!= nil"Length of output: 1358
| @@ -119,8 +118,7 @@ func (c *ServerSuite) TestActiveRFQSingleRelayer() { | |||
| // Assert the response | |||
| c.Assert().True(userQuoteResp.Success) | |||
| c.Assert().Equal("active", userQuoteResp.QuoteType) | |||
| c.Assert().Equal(destAmount, *userQuoteResp.Data.DestAmount) | |||
| c.Assert().Equal(originAmount, userQuoteResp.Data.OriginAmount) | |||
| c.Assert().Equal(destAmount, userQuoteResp.DestAmount) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repeated assertions into a helper function
The assertion c.Assert().Equal(destAmount, userQuoteResp.DestAmount) is used in multiple test cases. To improve code maintainability and reduce duplication, consider extracting this and similar assertions into a helper function.
| @@ -234,8 +231,7 @@ | |||
| // Assert the response | |||
| c.Assert().True(userQuoteResp.Success) | |||
| c.Assert().Equal("active", userQuoteResp.QuoteType) | |||
| c.Assert().Equal(destAmount, *userQuoteResp.Data.DestAmount) | |||
| c.Assert().Equal(originAmount, userQuoteResp.Data.OriginAmount) | |||
| c.Assert().Equal(destAmount, userQuoteResp.DestAmount) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication by using a helper function
This assertion duplicates logic from other tests:
c.Assert().Equal(destAmount, userQuoteResp.DestAmount)Refactoring repeated code into a helper function can enhance readability and maintainability.
| c.Assert().Equal("998000", userQuoteResp.DestAmount) // destAmount is quote destAmount minus fixed fee | ||
| c.Assert().Equal(c.relayerWallets[0].Address().Hex(), userQuoteResp.RelayerAddress) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compute expected DestAmount dynamically instead of hardcoding
In the assertions:
c.Assert().Equal("998000", userQuoteResp.DestAmount) // destAmount is quote destAmount minus fixed fee
c.Assert().Equal(c.relayerWallets[0].Address().Hex(), userQuoteResp.RelayerAddress)The expected DestAmount is hardcoded as "998000". To improve test flexibility and reduce potential errors if input values change, consider calculating the expected DestAmount using the test variables.
Apply this change:
+ expectedDestAmount := new(big.Int).Sub(passiveQuotes[0].DestAmount.BigInt(), passiveQuotes[0].FixedFee.BigInt())
// Assert the response
c.Assert().True(userQuoteResp.Success)
c.Assert().Equal("passive", userQuoteResp.QuoteType)
- c.Assert().Equal("998000", userQuoteResp.DestAmount) // destAmount is quote destAmount minus fixed fee
+ c.Assert().Equal(expectedDestAmount.String(), userQuoteResp.DestAmount)
c.Assert().Equal(c.relayerWallets[0].Address().Hex(), userQuoteResp.RelayerAddress)| c.Assert().Equal("998900", userQuoteResp.DestAmount) // destAmount is quote destAmount minus fixed fee | ||
| c.Assert().Equal(c.relayerWallets[0].Address().Hex(), userQuoteResp.RelayerAddress) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Calculate expected DestAmount instead of using hardcoded value
The assertion uses a hardcoded value:
c.Assert().Equal("998900", userQuoteResp.DestAmount) // destAmount is quote destAmount minus fixed feeTo enhance maintainability, compute the expected DestAmount dynamically based on test inputs.
Apply this change:
+ expectedDestAmount := new(big.Int).Sub(passiveQuotes[0].DestAmount.BigInt(), passiveQuotes[0].FixedFee.BigInt())
// Assert the response
c.Assert().True(userQuoteResp.Success)
c.Assert().Equal("passive", userQuoteResp.QuoteType)
- c.Assert().Equal("998900", userQuoteResp.DestAmount) // destAmount is quote destAmount minus fixed fee
+ c.Assert().Equal(expectedDestAmount.String(), userQuoteResp.DestAmount)
c.Assert().Equal(c.relayerWallets[0].Address().Hex(), userQuoteResp.RelayerAddress)
* WIP: initial websocket wiring * WIP: add ws client and handling * Fix: receive respsects context * Cleanup: split into rfq.go * Fix: build * Feat: add mocked ws client * Fix: build * Feat: add SubscribeActiveQuotes() to client * Feat: add PutUserQuoteRequest() to api client * Fix: build * WIP: rfq tests with ws auth * WIP: test fixes * Feat: working TestHandleActiveRFQ * Feat: add expired request case * WIP: functionalize test relayer resps * Feat: add runMockRelayer with multiple relayers * Feat: add MultipleRelayers case * Feat: add FallbackToPassive case * Fix: bigint ptr issue * Cleanup: bump expiration window * WIP: logs * Feat: split into separate tests * Cleanup: logs * Feat: add PassiveBestQuote case * WIP: update db interface with new models * Feat: impl new db funcs * Feat: insert models within api server * Feat: update quote request / response statuses * Fix: db error handling * Fix: api tests * Feat: add initial response validation * Feat: impl pingpong * Fix: register models * Feat: verify quote request in SingleRelayer case * Feat: verify more db requests * Cleanup: common vars * Cleanup: break down handleActiveRFQ into sub funcs * Cleanup: comments * Cleanup: remove unused mock * Fix: builds * Feat: make relayer response data optional to signify null resp * Fix: response primary key on quote id * Fix: build * Feat: update swagger docs * WIP: generic pubsub * Feat: add basic PubSubManager * Feat: implement subscription / unsubscription operations * Feat: respond to subscribe operation * Feat: add runWsListener helper * Cleanup: reduce chan buffer * Cleanup: lints * Cleanup: break down into smaller funcs * Cleanup: refactor ws client * Cleanup: more lints * Fix: build * Cleanup: lints * Feat: mark as fulfilled when updating request status * Cleanup: lint * Skip broken test for now * Cleanup: lint * Feat: add open_quote_requests endpoint with test * Feat: add new open request model * Update swagger * go mod tidy * fix error * Fix: respecting context * Replace: Fulfilled -> Closed * Cleanup: use errors.New() * Feat: ReceiveQuoteResponse specifies request id * Cleanup: remove logs * Feat: add some tracing * Feat: add IntegratorID * Feat: remove repetitive fields from relayer quote response, move requests to requests.go * Cleanup: use new routes * Cleanup: migrate req/res struct naming * Cleanup: update swagger * Cleanup: lint * [goreleaser] * Feat: run ws endpoint within existing server * [goreleaser] * Fix: build * [goreleaser] * Feat: add more tracing * [goreleaser] * feat(rfq-relayer): relayer supports active quoting (#3198) * Feat: add active rfq subscription on quoter * Feat: relayer subscribes to active quotes upon starting * [goreleaser] * Feat: specify ws url in relayer * [goreleaser] * [goreleaser] * Fix: build * [goreleaser] * Feat: relayer tracing * [goreleaser] * Feat: use supports_active_quoting instead of ws url * [goreleaser] * WIP: add logs * [goreleaser] * WIP: more logs * [goreleaser] * More logs * [goreleaser] * More logs * [goreleaser] * More logs * [goreleaser] * Close conn when encountering write err * [goreleaser] * More logs * [goreleaser] * More logs * [goreleaser] * More logs * [goreleaser] * More logs * [goreleaser] * Logs with ts * [goreleaser] * More tracing * [goreleaser] * Fix: send to reqChan * [goreleaser] * Check for zero pong time * Fix: make close_at and closed_quote_id optional * [goreleaser] * Feat: remove extra fields from responses * [goreleaser] * Fix: skip passive quote * [goreleaser] * Cleanup: remove logs * Fix: use correct span * Cleanup: remove logs * Fix: build * Cleanup: lint * Cleanup: lint * Cleanup: update swagger * Feat: client sends pings, server sends pongs * [goreleaser] * Cleanup: remove unused func * WIP: ws error handling * [goreleaser] * Feat: ws client uses errgroup * Cleanup: remove log * [goreleaser] * Replace: PutUserQuoteResponse -> PutRFQResponse * Feat: add QuoteID to PutRFQResponse * [goreleaser] * Cleanup: lint * Fix: build * Cleanup: lint * [goreleaser] * Add logs * [goreleaser] * Add logs * [goreleaser] * Cleanup: remove logs --------- Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes