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-relayer): apply zap fee to dest amount for active quotes [SLT-465] #3395

Merged
merged 5 commits into from
Nov 14, 2024
Merged

feat(rfq-relayer): apply zap fee to dest amount for active quotes [SLT-465] #3395

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
765a5f8
Feat: set zap params in supply to fee pricer
dwasse Nov 14, 2024
61b1b9a
Feat: adjust dest amount by fee for active quote
dwasse Nov 14, 2024
868c213
Feat: add TestGenerateActiveRFQ
dwasse Nov 14, 2024
1caa62c
Feat: add nonzero case
dwasse Nov 14, 2024
40eb027
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
1 change: 0 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,7 +55,6 @@ type QuoteData struct {
DestChainID int `json:"dest_chain_id"`
OriginTokenAddr string `json:"origin_token_addr"`
DestTokenAddr string `json:"dest_token_addr"`
OriginAmount string `json:"origin_amount"`
ExpirationWindow int64 `json:"expiration_window"`
ZapData string `json:"zap_data"`
ZapNative string `json:"zap_native"`
Expand Down
4 changes: 4 additions & 0 deletions services/rfq/relayer/quoter/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ func (m *Manager) GetDestAmount(ctx context.Context, quoteAmount *big.Int, token
return m.getDestAmount(ctx, quoteAmount, tokenName, input)
}

func (m *Manager) GenerateActiveRFQ(ctx context.Context, msg *model.ActiveRFQMessage) (resp *model.ActiveRFQMessage, err error) {
return m.generateActiveRFQ(ctx, msg)
}
Comment on lines +25 to +27
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

Missing test coverage for zap fee calculations

The test coverage for the addZapFees functionality is incomplete. While there are tests for general fee calculations in fee_pricer_test.go, there are no specific test cases for:

  • Zap data gas estimation (getZapGasEstimate)
  • Zap native token value conversion
  • Combined zap fees with both data and native token components

Required test additions:

  • Test cases in fee_pricer_test.go for addZapFees with various zap data scenarios
  • Test cases for zap native token value calculations
  • Edge cases with zero values and invalid inputs
🔗 Analysis chain

Verify test coverage for the new functionality.

Since this is a test export, ensure that comprehensive tests are implemented for the underlying generateActiveRFQ method, particularly around the new zap fee calculations mentioned in the PR title.

Let me gather more information about the test coverage by checking the actual test implementations.

Let me check for specific test coverage of the zap fee calculations.

Let me check for tests in the pricer package since that's where the zap fee implementation is located.

Let me check the actual content of the fee pricer test files to verify the test coverage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for test coverage of the generateActiveRFQ functionality
# Look for test files that might contain tests for the new functionality

# Search for test files containing ActiveRFQ tests
echo "Searching for relevant test files..."
rg -l "TestGenerate.*ActiveRFQ|TestManager.*ActiveRFQ" --type go

# Look for actual test implementations of the functionality
echo "Searching for test implementations..."
rg -A 10 "func Test.*ActiveRFQ" --type go

# Check for zap fee related test assertions
echo "Checking for zap fee related tests..."
rg "zap.*fee|zapFee|ZapFee" --type go

Length of output: 950

Script:

#!/bin/bash
# Check the actual test implementations in the identified test file
echo "Checking test implementations in quoter_test.go..."
cat services/rfq/relayer/quoter/quoter_test.go

# Also check the implementation of generateActiveRFQ to understand what should be tested
echo -e "\nChecking the actual implementation..."
rg -A 20 "func.*generateActiveRFQ" services/rfq/relayer/quoter/

Length of output: 29647

Script:

#!/bin/bash
# Look for test files that might contain tests for zap fee calculations
echo "Checking test coverage for zap fee calculations..."
rg -A 10 "TestAdd.*ZapFee|Test.*ZapFee" --type go

# Look for test assertions related to zap fees
echo -e "\nChecking for zap fee related test assertions..."
rg "assert.*[Zz]ap.*[Ff]ee|expect.*[Zz]ap.*[Ff]ee" --type go

# Check the implementation of addZapFees to understand what should be tested
echo -e "\nChecking addZapFees implementation..."
rg -A 20 "func.*addZapFees" services/rfq/relayer/pricer/

Length of output: 2280

Script:

#!/bin/bash
# Check for test files in the pricer package
echo "Checking for test files in pricer package..."
fd ".*_test.go" services/rfq/relayer/pricer/

# Look for test implementations related to fee calculations
echo -e "\nChecking fee calculation test implementations..."
rg -A 10 "func Test.*Fee" services/rfq/relayer/pricer/

# Check for test setup and mocks related to fee calculations
echo -e "\nChecking test setup and mocks..."
rg "type.*Suite.*struct|func.*Suite.*Setup" services/rfq/relayer/pricer/

Length of output: 785

Script:

#!/bin/bash
# Check the content of fee pricer test files
echo "Checking fee_pricer_test.go content..."
cat services/rfq/relayer/pricer/fee_pricer_test.go

echo -e "\nChecking suite_test.go content..."
cat services/rfq/relayer/pricer/suite_test.go

Length of output: 20603


func (m *Manager) SetConfig(cfg relconfig.Config) {
m.config = cfg
}
Expand Down
28 changes: 28 additions & 0 deletions services/rfq/relayer/quoter/quoter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

"github.com/ipfs/go-log"
"github.com/synapsecns/sanguine/core/metrics"
"github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgev2"
"github.com/synapsecns/sanguine/services/rfq/relayer/pricer"
"github.com/synapsecns/sanguine/services/rfq/relayer/relconfig"
"github.com/synapsecns/sanguine/services/rfq/relayer/reldb"
Expand Down Expand Up @@ -338,7 +339,7 @@
// getActiveRFQ handles an active RFQ message.
//
//nolint:nilnil
func (m *Manager) generateActiveRFQ(ctx context.Context, msg *model.ActiveRFQMessage) (resp *model.ActiveRFQMessage, err error) {

Check failure on line 342 in services/rfq/relayer/quoter/quoter.go

View workflow job for this annotation

GitHub Actions / Lint (services/rfq) 

calculated cyclomatic complexity for function generateActiveRFQ is 13, max is 10 (cyclop)
ctx, span := m.metricsHandler.Tracer().Start(ctx, "generateActiveRFQ", trace.WithAttributes(
attribute.String("op", msg.Op),
attribute.String("content", string(msg.Content)),
Expand Down Expand Up @@ -379,11 +380,38 @@
DestBalance: inv[rfqRequest.Data.DestChainID][common.HexToAddress(rfqRequest.Data.DestTokenAddr)],
OriginAmountExact: originAmountExact,
}
if rfqRequest.Data.ZapNative != "" || rfqRequest.Data.ZapData != "" {
zapNative, ok := new(big.Int).SetString(rfqRequest.Data.ZapNative, 10)
if !ok {
return nil, fmt.Errorf("invalid zap native amount: %s", rfqRequest.Data.ZapNative)
}
quoteInput.QuoteRequest = &reldb.QuoteRequest{
Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{
ZapNative: zapNative,
ZapData: []byte(rfqRequest.Data.ZapData),
},
}
}
Comment on lines +383 to +394
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect conversion of ZapData to byte slice

The code converts rfqRequest.Data.ZapData directly to a byte slice using []byte(rfqRequest.Data.ZapData). If ZapData is a hex-encoded string representing binary data, this conversion will not produce the intended byte slice. Instead, it will convert the string's characters to their byte values, which may lead to incorrect data being used in the transaction. To handle ZapData correctly, you should decode the hex string into a byte slice.

Apply this diff to fix the issue:

+import (
+    "encoding/hex"
+    "strings"
+)

...

    if rfqRequest.Data.ZapNative != "" || rfqRequest.Data.ZapData != "" {
        zapNative, ok := new(big.Int).SetString(rfqRequest.Data.ZapNative, 10)
        if !ok {
            return nil, fmt.Errorf("invalid zap native amount: %s", rfqRequest.Data.ZapNative)
        }
+       zapDataBytes, err := hex.DecodeString(strings.TrimPrefix(rfqRequest.Data.ZapData, "0x"))
+       if err != nil {
+           return nil, fmt.Errorf("invalid zap data: %s", rfqRequest.Data.ZapData)
+       }
        quoteInput.QuoteRequest = &reldb.QuoteRequest{
            Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{
                ZapNative: zapNative,
-               ZapData:   []byte(rfqRequest.Data.ZapData),
+               ZapData:   zapDataBytes,
            },
        }
    }
📝 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
if rfqRequest.Data.ZapNative != "" || rfqRequest.Data.ZapData != "" {
zapNative, ok := new(big.Int).SetString(rfqRequest.Data.ZapNative, 10)
if !ok {
return nil, fmt.Errorf("invalid zap native amount: %s", rfqRequest.Data.ZapNative)
}
quoteInput.QuoteRequest = &reldb.QuoteRequest{
Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{
ZapNative: zapNative,
ZapData: []byte(rfqRequest.Data.ZapData),
},
}
}
if rfqRequest.Data.ZapNative != "" || rfqRequest.Data.ZapData != "" {
zapNative, ok := new(big.Int).SetString(rfqRequest.Data.ZapNative, 10)
if !ok {
return nil, fmt.Errorf("invalid zap native amount: %s", rfqRequest.Data.ZapNative)
}
zapDataBytes, err := hex.DecodeString(strings.TrimPrefix(rfqRequest.Data.ZapData, "0x"))
if err != nil {
return nil, fmt.Errorf("invalid zap data: %s", rfqRequest.Data.ZapData)
}
quoteInput.QuoteRequest = &reldb.QuoteRequest{
Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{
ZapNative: zapNative,
ZapData: zapDataBytes,
},
}
}


rawQuote, err := m.generateQuote(ctx, quoteInput)
if err != nil {
return nil, fmt.Errorf("error generating quote: %w", err)
}

// adjust dest amount by fixed fee
destAmountBigInt, ok := new(big.Int).SetString(rawQuote.DestAmount, 10)
if !ok {
return nil, fmt.Errorf("invalid dest amount: %s", rawQuote.DestAmount)
}
fixedFeeBigInt, ok := new(big.Int).SetString(rawQuote.FixedFee, 10)
if !ok {
return nil, fmt.Errorf("invalid fixed fee: %s", rawQuote.FixedFee)
}
destAmountAdj := new(big.Int).Sub(destAmountBigInt, fixedFeeBigInt)
if destAmountAdj.Sign() < 0 {
destAmountAdj = big.NewInt(0)
}
rawQuote.DestAmount = destAmountAdj.String()
span.SetAttributes(attribute.String("dest_amount", rawQuote.DestAmount))

rfqResp := model.WsRFQResponse{
Expand Down
89 changes: 89 additions & 0 deletions services/rfq/relayer/quoter/quoter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
package quoter_test

import (
"encoding/json"
"fmt"
"math/big"

"github.com/ethereum/go-ethereum/common"
"github.com/stretchr/testify/mock"
"github.com/synapsecns/sanguine/core/metrics"
"github.com/synapsecns/sanguine/core/testsuite"
clientMocks "github.com/synapsecns/sanguine/ethergo/client/mocks"
fetcherMocks "github.com/synapsecns/sanguine/ethergo/submitter/mocks"
"github.com/synapsecns/sanguine/services/rfq/api/model"
"github.com/synapsecns/sanguine/services/rfq/api/rest"
"github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgev2"
inventoryMocks "github.com/synapsecns/sanguine/services/rfq/relayer/inventory/mocks"
"github.com/synapsecns/sanguine/services/rfq/relayer/pricer"
Expand Down Expand Up @@ -444,6 +447,92 @@ func (s *QuoterSuite) TestGetOriginAmountActiveQuotes() {
s.Equal(expectedAmount, quoteAmount)
}

func (s *QuoterSuite) TestGenerateActiveRFQ() {
origin := int(s.origin)
dest := int(s.destination)
originAddr := common.HexToAddress("0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48")
destAddr := common.HexToAddress("0x0b2c639c533813f4aa9d7837caf62653d097ff85")
balance := big.NewInt(1_000_000_000_000)
balances := map[int]map[common.Address]*big.Int{
origin: {
originAddr: balance,
},
dest: {
destAddr: balance,
},
}

currentHeader := big.NewInt(100_000_000_000) // 100 gwei
clientFetcher := new(fetcherMocks.ClientFetcher)
clientMock := new(clientMocks.EVM)
clientMock.On(testsuite.GetFunctionName(clientMock.SuggestGasPrice), mock.Anything).Return(currentHeader, nil)
clientFetcher.On(testsuite.GetFunctionName(clientMock.EstimateGas), mock.Anything, mock.Anything).Return(100_000, nil)
clientFetcher.On(testsuite.GetFunctionName(clientFetcher.GetClient), mock.Anything, mock.Anything).Return(clientMock, nil)
priceFetcher := new(priceMocks.CoingeckoPriceFetcher)
priceFetcher.On(testsuite.GetFunctionName(priceFetcher.GetPrice), mock.Anything, mock.Anything).Return(0., fmt.Errorf("not using mocked price"))
feePricer := pricer.NewFeePricer(s.config, clientFetcher, priceFetcher, metrics.NewNullHandler(), common.HexToAddress("0x123"))
inventoryManager := new(inventoryMocks.Manager)
inventoryManager.On(testsuite.GetFunctionName(inventoryManager.HasSufficientGas), mock.Anything, mock.Anything, mock.Anything).Return(true, nil)
inventoryManager.On(testsuite.GetFunctionName(inventoryManager.GetCommittableBalances), mock.Anything, mock.Anything, mock.Anything).Return(balances, nil)
mgr, err := quoter.NewQuoterManager(s.config, metrics.NewNullHandler(), inventoryManager, nil, feePricer, nil)
s.NoError(err)

var ok bool
s.manager, ok = mgr.(*quoter.Manager)
s.True(ok)

req := model.PutRFQRequest{
UserAddress: "0x123",
IntegratorID: "123",
QuoteTypes: []string{"active"},
Data: model.QuoteData{
OriginChainID: origin,
DestChainID: dest,
OriginAmountExact: "100000",
OriginTokenAddr: originAddr.String(),
DestTokenAddr: destAddr.String(),
},
}
reqBytes, err := json.Marshal(req)
s.NoError(err)
msg := model.ActiveRFQMessage{
Op: rest.RequestQuoteOp,
Content: json.RawMessage(reqBytes),
}

respMsg, err := s.manager.GenerateActiveRFQ(s.GetTestContext(), &msg)
s.NoError(err)
var resp model.WsRFQResponse
err = json.Unmarshal(respMsg.Content, &resp)
s.NoError(err)
s.Equal("0", resp.DestAmount)

req = model.PutRFQRequest{
UserAddress: "0x123",
IntegratorID: "123",
QuoteTypes: []string{"active"},
Data: model.QuoteData{
OriginChainID: origin,
DestChainID: dest,
OriginAmountExact: "500000000000",
OriginTokenAddr: originAddr.String(),
DestTokenAddr: destAddr.String(),
},
}
reqBytes, err = json.Marshal(req)
s.NoError(err)
msg = model.ActiveRFQMessage{
Op: rest.RequestQuoteOp,
Content: json.RawMessage(reqBytes),
}

respMsg, err = s.manager.GenerateActiveRFQ(s.GetTestContext(), &msg)
s.NoError(err)
err = json.Unmarshal(respMsg.Content, &resp)
s.NoError(err)
s.Equal("499899950000", resp.DestAmount)
}

func (s *QuoterSuite) TestGetOriginAmount() {
origin := int(s.origin)
dest := int(s.destination)
Expand Down
Loading