Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rfq-api): don't respond to passive quotes for requests with zap params [SLT-430, SLT-432] #3388

Merged
merged 5 commits into from
Nov 14, 2024
Merged

feat(rfq-api): don't respond to passive quotes for requests with zap params [SLT-430, SLT-432] #3388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f8d361d
Feat: add zap params to api model
dwasse Nov 12, 2024
54da64d
Feat: don't consider quotes with zap data as passive quotes
dwasse Nov 12, 2024
98a0bad
Feat: add test cases for zap quotes
dwasse Nov 12, 2024
839ab03
Merge branch 'feat/relayer-arb-call' into feat/zap-params
dwasse Nov 14, 2024
81bf628
Cleanup: lint
dwasse Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion services/rfq/api/model/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,11 @@ type QuoteData struct {
DestChainID int `json:"dest_chain_id"`
OriginTokenAddr string `json:"origin_token_addr"`
DestTokenAddr string `json:"dest_token_addr"`
OriginAmountExact string `json:"origin_amount_exact"`
OriginAmount string `json:"origin_amount"`
ExpirationWindow int64 `json:"expiration_window"`
ZapData string `json:"zap_data"`
ZapNative string `json:"zap_native"`
OriginAmountExact string `json:"origin_amount_exact"`
DestAmount *string `json:"dest_amount"`
RelayerAddress *string `json:"relayer_address"`
QuoteID *string `json:"quote_id"`
Expand Down
10 changes: 10 additions & 0 deletions services/rfq/api/rest/rfq_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,16 @@ func (c *ServerSuite) TestActiveRFQFallbackToPassive() {
c.Assert().Equal("passive", userQuoteResp.QuoteType)
c.Assert().Equal("998000", userQuoteResp.DestAmount) // destAmount is quote destAmount minus fixed fee
c.Assert().Equal(c.relayerWallets[0].Address().Hex(), userQuoteResp.RelayerAddress)

// Submit a user quote request with zap data
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)
}

func (c *ServerSuite) TestActiveRFQPassiveBestQuote() {
Expand Down
29 changes: 22 additions & 7 deletions services/rfq/api/rest/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package rest
import (
"context"
"encoding/json"
"math/big"

"fmt"
"net/http"
Expand Down Expand Up @@ -532,19 +533,21 @@ func (r *QuoterAPIServer) PutRFQRequest(c *gin.Context) {
span.SetAttributes(attribute.Bool("is_active_rfq", isActiveRFQ))

// if specified, fetch the active quote. always consider passive quotes
var activeQuote *model.QuoteData
var activeQuote, passiveQuote *model.QuoteData
if isActiveRFQ {
activeQuote = r.handleActiveRFQ(ctx, &req, requestID)
if activeQuote != nil && activeQuote.DestAmount != nil {
span.SetAttributes(attribute.String("active_quote_dest_amount", *activeQuote.DestAmount))
}
}
passiveQuote, err := r.handlePassiveRFQ(ctx, &req)
if err != nil {
logger.Error("Error handling passive RFQ", "error", err)
}
if passiveQuote != nil && passiveQuote.DestAmount != nil {
span.SetAttributes(attribute.String("passive_quote_dest_amount", *passiveQuote.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 {
span.SetAttributes(attribute.String("passive_quote_dest_amount", *passiveQuote.DestAmount))
}
}
quote := getBestQuote(activeQuote, passiveQuote)

Expand Down Expand Up @@ -576,6 +579,18 @@ func (r *QuoterAPIServer) PutRFQRequest(c *gin.Context) {
c.JSON(http.StatusOK, resp)
}

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
}
Comment on lines +582 to +592
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling in isZapQuote function

The function has a few potential issues:

  1. It silently ignores invalid ZapNative values
  2. No validation is performed on ZapData
  3. 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.

Suggested change
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
}


func (r *QuoterAPIServer) recordLatestQuoteAge(ctx context.Context, observer metric.Observer) (err error) {
if r.handler == nil || r.latestQuoteAgeGauge == nil {
return nil
Expand Down
6 changes: 5 additions & 1 deletion services/rfq/api/rest/ws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ func (c *wsClient) sendRelayerRequest(ctx context.Context, req *model.WsRFQReque

// handleRelayerMessage handles messages from the relayer.
// An error returned will result in the websocket connection being closed.
//
//nolint:cyclop
func (c *wsClient) handleRelayerMessage(ctx context.Context, msg []byte) (err error) {
_, span := c.handler.Tracer().Start(ctx, "handleRelayerMessage", trace.WithAttributes(
attribute.String("relayer_address", c.relayerAddr),
Expand Down Expand Up @@ -235,7 +237,9 @@ func (c *wsClient) handleRelayerMessage(ctx context.Context, msg []byte) (err er
}
case SendQuoteOp:
err = c.handleSendQuote(ctx, rfqMsg.Content)
logger.Errorf("error handling send quote: %v", err)
if err != nil {
logger.Errorf("error handling send quote: %v", err)
}
Comment on lines +240 to +242
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

default:
logger.Errorf("received unexpected operation from relayer: %s", rfqMsg.Op)
return nil
Expand Down
Loading