feat(rfq-api): don't respond to passive quotes for requests with zap params [SLT-430, SLT-432] #3388
Conversation
WalkthroughThe changes in this pull request introduce new fields to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="[config_reader] The configuration option 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/relayer-arb-call #3388 +/- ##
===============================================================
- Coverage 26.62752% 26.60645% -0.02108%
===============================================================
Files 214 214
Lines 13625 13617 -8
Branches 82 82
===============================================================
- Hits 3628 3623 -5
+ Misses 9713 9711 -2
+ Partials 284 283 -1
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: |
81bf628
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a8b2c8cf.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-zap-params.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: 3
🧹 Outside diff range and nitpick comments (4)
services/rfq/api/rest/ws.go (2)
238-240: Consider enhancing error handling for zap-related casesWhile the error handling improvement is good, given that this PR introduces zap-related changes, consider adding specific error handling or logging for zap parameter validation failures to help with debugging issues related to passive quotes with zap parameters.
if err != nil { - logger.Errorf("error handling send quote: %v", err) + logger.Errorf("error handling send quote (zap validation failed: %t): %v", + errors.Is(err, ErrZapValidationFailed), err) }
238-240: Consider standardizing error handling patternsThe WebSocket client has inconsistent error handling patterns across different operations. While some handlers return errors in response messages (subscribe/unsubscribe), the
handleSendQuotelogs and swallows errors. Consider standardizing the error handling approach for better maintainability and debugging.Consider one of these approaches:
- Return error responses consistently across all operations
- Document why certain operations handle errors differently
services/rfq/api/rest/rfq_test.go (1)
310-319: Consider enhancing test coverage for zap parametersThe test case effectively verifies the basic rejection of requests with zap parameters. However, consider these improvements:
- Add test cases for edge cases:
- Empty but non-null zap data
- Invalid zap native amount
- Maximum allowed values
- Add a comment explaining the business logic behind rejecting zap requests
- Verify that the request was properly logged for monitoring/debugging
Here's a suggested enhancement:
// Submit a user quote request with zap data + // Zap requests should be rejected as they require special handling userQuoteReq.Data.ZapData = "abc" userQuoteReq.Data.ZapNative = "100" userQuoteResp, err = userClient.PutRFQRequest(c.GetTestContext(), userQuoteReq) c.Require().NoError(err) // Assert the response c.Assert().False(userQuoteResp.Success) c.Assert().Equal("no quotes found", userQuoteResp.Reason) + + // Test edge cases + testCases := []struct { + name string + zapData string + zapNative string + }{ + {"empty_zap_data", "", "100"}, + {"invalid_zap_native", "abc", "invalid"}, + {"max_values", strings.Repeat("a", 1000), "999999999"}, + } + + for _, tc := range testCases { + c.Run(tc.name, func() { + userQuoteReq.Data.ZapData = tc.zapData + userQuoteReq.Data.ZapNative = tc.zapNative + resp, err := userClient.PutRFQRequest(c.GetTestContext(), userQuoteReq) + c.Require().NoError(err) + c.Assert().False(resp.Success) + c.Assert().Equal("no quotes found", resp.Reason) + }) + } + + // Verify request logging + // TODO: Add verification that the request was properly loggedservices/rfq/api/rest/server.go (1)
543-543: Consider adding logging for Zap quote decisionsAdding debug logging when skipping passive quotes due to Zap parameters would help with troubleshooting.
Apply this diff to add logging:
- if !isZapQuote(&req) { + isZap := isZapQuote(&req) + if isZap { + logger.Debug("Skipping passive quote for Zap request", "request_id", requestID) + } + if !isZap {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
services/rfq/api/model/request.go(1 hunks)services/rfq/api/rest/rfq_test.go(1 hunks)services/rfq/api/rest/server.go(3 hunks)services/rfq/api/rest/ws.go(1 hunks)
🔇 Additional comments (3)
services/rfq/api/model/request.go (1)
62-64: 🛠️ Refactor suggestion
Document nullable fields and verify API compatibility
The conversion of DestAmount, RelayerAddress, and QuoteID to pointer types is a breaking change that could affect API clients. Please document when these fields might be nil.
Add documentation for the nullable fields:
+ // DestAmount is the target amount for the quote. May be nil for certain quote types.
DestAmount *string `json:"dest_amount"`
+ // RelayerAddress is the address of the relayer. May be nil for user-initiated quotes.
RelayerAddress *string `json:"relayer_address"`
+ // QuoteID is the unique identifier for the quote. May be nil for new quote requests.
QuoteID *string `json:"quote_id"`Let's verify the impact:
services/rfq/api/rest/server.go (2)
7-7: LGTM: Import addition is appropriate
The addition of the "math/big" package is necessary for handling large numbers in the new isZapQuote function.
536-550:
Potential nil pointer dereference in quote handling
The code accesses DestAmount without checking if activeQuote or passiveQuote is nil first. While there is a nil check for the quotes themselves, accessing DestAmount should also be guarded.
Apply this diff to fix the potential nil pointer dereference:
if isActiveRFQ {
activeQuote = r.handleActiveRFQ(ctx, &req, requestID)
- if activeQuote != nil && activeQuote.DestAmount != nil {
+ if activeQuote != nil {
+ if activeQuote.DestAmount != nil {
span.SetAttributes(attribute.String("active_quote_dest_amount", *activeQuote.DestAmount))
+ }
}
}
if !isZapQuote(&req) {
passiveQuote, err = r.handlePassiveRFQ(ctx, &req)
if err != nil {
logger.Error("Error handling passive RFQ", "error", err)
}
- if passiveQuote != nil && passiveQuote.DestAmount != nil {
+ if passiveQuote != nil {
+ if passiveQuote.DestAmount != nil {
span.SetAttributes(attribute.String("passive_quote_dest_amount", *passiveQuote.DestAmount))
+ }
}
}Likely invalid or redundant comment.
services/rfq/api/model/request.go
Outdated
| ZapData string `json:"zap_data"` | ||
| ZapNative string `json:"zap_native"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and consider validation for new zap fields
The new ZapData and ZapNative fields lack documentation explaining their purpose and expected format. Additionally, consider adding validation if there are specific format requirements or size constraints.
Add documentation above the fields:
+ // ZapData contains the Zap protocol-specific data for the quote request
ZapData string `json:"zap_data"`
+ // ZapNative specifies the native token amount for Zap protocol integration
ZapNative string `json:"zap_native"`📝 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.
| ZapData string `json:"zap_data"` | |
| ZapNative string `json:"zap_native"` | |
| // ZapData contains the Zap protocol-specific data for the quote request | |
| ZapData string `json:"zap_data"` | |
| // ZapNative specifies the native token amount for Zap protocol integration | |
| ZapNative string `json:"zap_native"` |
| if err != nil { | ||
| logger.Errorf("error handling send quote: %v", err) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Potential race condition in error logging after connection closure
Errors may be logged after the WebSocket connection has been closed. Consider verifying the connection state before logging errors to prevent misleading log entries.
🔗 Analysis chain
Verify error logging after connection closure
There might be a race condition where errors are logged after the connection is closed. Consider adding a check for connection state before logging errors to avoid misleading logs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for patterns where errors might be logged after connection closure
rg -A 5 "conn.*Close|close\(.*doneChan\)"
Length of output: 2740
| func isZapQuote(req *model.PutRFQRequest) bool { | ||
| if req.Data.ZapData != "" { | ||
| return true | ||
| } | ||
|
|
||
| zapNative, ok := new(big.Int).SetString(req.Data.ZapNative, 10) | ||
| if !ok { | ||
| return false | ||
| } | ||
| return zapNative.Sign() != 0 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in isZapQuote function
The function has a few potential issues:
- It silently ignores invalid ZapNative values
- No validation is performed on ZapData
- The big.Int comparison could be more explicit
Apply this diff to improve error handling and clarity:
func isZapQuote(req *model.PutRFQRequest) bool {
+ if req == nil || req.Data == nil {
+ return false
+ }
+
if req.Data.ZapData != "" {
return true
}
+ if req.Data.ZapNative == "" {
+ return false
+ }
+
zapNative, ok := new(big.Int).SetString(req.Data.ZapNative, 10)
if !ok {
return false
}
- return zapNative.Sign() != 0
+ return zapNative.Cmp(big.NewInt(0)) > 0
}📝 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 isZapQuote(req *model.PutRFQRequest) bool { | |
| if req.Data.ZapData != "" { | |
| return true | |
| } | |
| zapNative, ok := new(big.Int).SetString(req.Data.ZapNative, 10) | |
| if !ok { | |
| return false | |
| } | |
| return zapNative.Sign() != 0 | |
| } | |
| func isZapQuote(req *model.PutRFQRequest) bool { | |
| if req == nil || req.Data == nil { | |
| return false | |
| } | |
| if req.Data.ZapData != "" { | |
| return true | |
| } | |
| if req.Data.ZapNative == "" { | |
| return false | |
| } | |
| zapNative, ok := new(big.Int).SetString(req.Data.ZapNative, 10) | |
| if !ok { | |
| return false | |
| } | |
| return zapNative.Cmp(big.NewInt(0)) > 0 | |
| } |
github-actions
bot
added
size/s
and removed
size/xs
needs-go-generate-contrib/opbot
needs-go-generate-contrib/promexporter
labels
Summary by CodeRabbit
New Features
ZapDataandZapNativein quote requests to enhance data handling.Bug Fixes
Tests