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

fix(rfq-relayer): profitability check accounts for offsets #3288

Merged
merged 10 commits into from
Oct 17, 2024
Merged

fix(rfq-relayer): profitability check accounts for offsets #3288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
02e51ec
Feat: check amounts with offsets in isProfitable()
dwasse Oct 14, 2024
5cf7e81
WIP: test adj changes
dwasse Oct 14, 2024
46fb354
Fix: quoter tests
dwasse Oct 14, 2024
c4467f2
[goreleaser]
dwasse Oct 14, 2024
4c3ee56
Cleanup: lint
dwasse Oct 14, 2024
ffbc37c
Feat: move origin offset calc to getDestAmount()
dwasse Oct 14, 2024
2c43ed8
Feat: add more test cases for new offset logic
dwasse Oct 14, 2024
a306d61
[goreleaser]
dwasse Oct 14, 2024
ba2a567
Fix: offset application
dwasse Oct 15, 2024
d682f96
[goreleaser]
dwasse Oct 15, 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
4 changes: 2 additions & 2 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 @@ -18,8 +18,8 @@ func (m *Manager) GetOriginAmount(ctx context.Context, input QuoteInput) (*big.I
return m.getOriginAmount(ctx, input)
}

func (m *Manager) GetDestAmount(ctx context.Context, quoteAmount *big.Int, chainID int, tokenName string) (*big.Int, error) {
return m.getDestAmount(ctx, quoteAmount, chainID, tokenName)
func (m *Manager) GetDestAmount(ctx context.Context, quoteAmount *big.Int, tokenName string, input QuoteInput) (*big.Int, error) {
return m.getDestAmount(ctx, quoteAmount, tokenName, input)
}

func (m *Manager) SetConfig(cfg relconfig.Config) {
Expand Down
72 changes: 48 additions & 24 deletions services/rfq/relayer/quoter/quoter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,16 +237,44 @@
if err != nil {
return false, fmt.Errorf("error getting total fee: %w", err)
}

cost := new(big.Int).Add(quote.Transaction.DestAmount, fee)

span.AddEvent("fee", trace.WithAttributes(attribute.String("fee", fee.String())))
span.AddEvent("cost", trace.WithAttributes(attribute.String("cost", cost.String())))
span.AddEvent("dest_amount", trace.WithAttributes(attribute.String("dest_amount", quote.Transaction.DestAmount.String())))
span.AddEvent("origin_amount", trace.WithAttributes(attribute.String("origin_amount", quote.Transaction.OriginAmount.String())))
// adjust amounts for our internal offsets on origin / dest token values
originAmountAdj, err := m.getAmountWithOffset(ctx, quote.Transaction.OriginChainId, quote.Transaction.OriginToken, quote.Transaction.OriginAmount)
if err != nil {
return false, fmt.Errorf("error getting origin amount with offset: %w", err)
}

Check warning on line 246 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L245-L246 

Added lines #L245 - L246 were not covered by tests
Comment on lines +243 to +246
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

Ensure non-nil 'originAmountAdj' to prevent potential runtime panic

After obtaining originAmountAdj, it is important to check if it is not nil before using it in comparisons to avoid a possible nil pointer dereference.

Apply this diff to add a nil check:

originAmountAdj, err := m.getAmountWithOffset(ctx, quote.Transaction.OriginChainId, quote.Transaction.OriginToken, quote.Transaction.OriginAmount)
if err != nil {
    return false, fmt.Errorf("error getting origin amount with offset: %w", err)
}
+if originAmountAdj == nil {
+    return false, fmt.Errorf("originAmountAdj is nil")
+}
📝 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
originAmountAdj, err := m.getAmountWithOffset(ctx, quote.Transaction.OriginChainId, quote.Transaction.OriginToken, quote.Transaction.OriginAmount)
if err != nil {
return false, fmt.Errorf("error getting origin amount with offset: %w", err)
}
originAmountAdj, err := m.getAmountWithOffset(ctx, quote.Transaction.OriginChainId, quote.Transaction.OriginToken, quote.Transaction.OriginAmount)
if err != nil {
return false, fmt.Errorf("error getting origin amount with offset: %w", err)
}
if originAmountAdj == nil {
return false, fmt.Errorf("originAmountAdj is nil")
}

// assume that fee is denominated in dest token terms
costAdj, err := m.getAmountWithOffset(ctx, quote.Transaction.DestChainId, quote.Transaction.DestToken, cost)
if err != nil {
return false, fmt.Errorf("error getting cost with offset: %w", err)
}

Check warning on line 251 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L250-L251 

Added lines #L250 - L251 were not covered by tests
Comment on lines +248 to +251
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

Ensure non-nil 'costAdj' to prevent potential runtime panic

Similar to originAmountAdj, verify that costAdj is not nil after calling getAmountWithOffset to avoid a possible nil pointer dereference during comparison.

Apply this diff to add a nil check:

costAdj, err := m.getAmountWithOffset(ctx, quote.Transaction.DestChainId, quote.Transaction.DestToken, cost)
if err != nil {
    return false, fmt.Errorf("error getting cost with offset: %w", err)
}
+if costAdj == nil {
+    return false, fmt.Errorf("costAdj is nil")
+}
📝 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
costAdj, err := m.getAmountWithOffset(ctx, quote.Transaction.DestChainId, quote.Transaction.DestToken, cost)
if err != nil {
return false, fmt.Errorf("error getting cost with offset: %w", err)
}
costAdj, err := m.getAmountWithOffset(ctx, quote.Transaction.DestChainId, quote.Transaction.DestToken, cost)
if err != nil {
return false, fmt.Errorf("error getting cost with offset: %w", err)
}
if costAdj == nil {
return false, fmt.Errorf("costAdj is nil")
}


span.SetAttributes(
attribute.String("origin_amount_adj", originAmountAdj.String()),
attribute.String("cost_adj", costAdj.String()),
attribute.String("origin_amount", quote.Transaction.OriginAmount.String()),
attribute.String("dest_amount", quote.Transaction.DestAmount.String()),
attribute.String("fee", fee.String()),
attribute.String("cost", cost.String()),
)

// NOTE: this logic assumes that the origin and destination tokens have the same price.
return quote.Transaction.OriginAmount.Cmp(cost) >= 0, nil
return originAmountAdj.Cmp(costAdj) >= 0, nil
}

func (m *Manager) getAmountWithOffset(ctx context.Context, chainID uint32, tokenAddr common.Address, amount *big.Int) (*big.Int, error) {
tokenName, err := m.config.GetTokenName(chainID, tokenAddr.Hex())
if err != nil {
return nil, fmt.Errorf("error getting token name: %w", err)
}

Check warning on line 269 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L268-L269 

Added lines #L268 - L269 were not covered by tests
// apply offset directly to amount without considering origin/dest
quoteOffsetBps, err := m.config.GetQuoteOffsetBps(int(chainID), tokenName, true)
if err != nil {
return nil, fmt.Errorf("error getting quote offset bps: %w", err)
}

Check warning on line 274 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L273-L274 

Added lines #L273 - L274 were not covered by tests
amountAdj := m.applyOffset(ctx, quoteOffsetBps, amount)
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

Fix potential precision loss in 'applyOffset' due to casting 'offsetBps'

In the applyOffset function, converting offsetBps from float64 to int64 loses the fractional part, which can cause incorrect calculations. This affects the accuracy of amount adjustments.

Modify the applyOffset function to avoid casting offsetBps to int64:

func (m *Manager) applyOffset(parentCtx context.Context, offsetBps float64, target *big.Int) (result *big.Int) {
    _, span := m.metricsHandler.Tracer().Start(parentCtx, "applyOffset", trace.WithAttributes(
        attribute.Float64("offset_bps", offsetBps),
        attribute.String("target", target.String()),
    ))
    defer func() {
        metrics.EndSpan(span)
    }()

-   offsetFraction := new(big.Float).Quo(new(big.Float).SetInt64(int64(offsetBps)), new(big.Float).SetInt64(10000))
-   offsetFactor := new(big.Float).Sub(new(big.Float).SetInt64(1), offsetFraction)
+   offsetFraction := new(big.Float).Quo(big.NewFloat(offsetBps), big.NewFloat(10000))
+   offsetFactor := new(big.Float).Sub(big.NewFloat(1), offsetFraction)

    result, _ = new(big.Float).Mul(new(big.Float).SetInt(target), offsetFactor).Int(nil)
    return result
}

This change preserves the fractional part of offsetBps and ensures accurate calculations.

Committable suggestion was skipped due to low confidence.


return amountAdj, nil
}

// SubmitAllQuotes submits all quotes to the RFQ API.
Expand Down Expand Up @@ -580,7 +608,7 @@
}

// Build the quote
destAmount, err := m.getDestAmount(ctx, originAmount, input.DestChainID, destToken)
destAmount, err := m.getDestAmount(ctx, originAmount, destToken, input)
if err != nil {
logger.Error("Error getting dest amount", "error", err)
return nil, fmt.Errorf("error getting dest amount: %w", err)
Expand Down Expand Up @@ -679,17 +707,6 @@
balanceFlt := new(big.Float).SetInt(input.DestBalance)
quoteAmount, _ = new(big.Float).Mul(balanceFlt, new(big.Float).SetFloat64(quotePct/100)).Int(nil)

// Apply the quoteOffset to origin token.
tokenName, err := m.config.GetTokenName(uint32(input.DestChainID), input.DestTokenAddr.Hex())
if err != nil {
return nil, fmt.Errorf("error getting token name: %w", err)
}
quoteOffsetBps, err := m.config.GetQuoteOffsetBps(input.OriginChainID, tokenName, true)
if err != nil {
return nil, fmt.Errorf("error getting quote offset bps: %w", err)
}
quoteAmount = m.applyOffset(ctx, quoteOffsetBps, quoteAmount)

// Clip the quoteAmount by the minQuoteAmount
minQuoteAmount := m.config.GetMinQuoteAmount(input.DestChainID, input.DestTokenAddr)
if quoteAmount.Cmp(minQuoteAmount) < 0 {
Expand Down Expand Up @@ -784,28 +801,35 @@

var errMinGasExceedsQuoteAmount = errors.New("min gas token exceeds quote amount")

func (m *Manager) getDestAmount(parentCtx context.Context, originAmount *big.Int, chainID int, tokenName string) (*big.Int, error) {
func (m *Manager) getDestAmount(parentCtx context.Context, originAmount *big.Int, tokenName string, input QuoteInput) (*big.Int, error) {
ctx, span := m.metricsHandler.Tracer().Start(parentCtx, "getDestAmount", trace.WithAttributes(
attribute.String("quote_amount", originAmount.String()),
))
defer func() {
metrics.EndSpan(span)
}()

quoteOffsetBps, err := m.config.GetQuoteOffsetBps(chainID, tokenName, false)
// Apply origin, destination, and quote width offsets
originOffsetBps, err := m.config.GetQuoteOffsetBps(input.OriginChainID, tokenName, true)
if err != nil {
return nil, fmt.Errorf("error getting quote offset bps: %w", err)
}

Check warning on line 816 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L815-L816 

Added lines #L815 - L816 were not covered by tests
destOffsetBps, err := m.config.GetQuoteOffsetBps(input.DestChainID, tokenName, false)
if err != nil {
return nil, fmt.Errorf("error getting quote offset bps: %w", err)
}
quoteWidthBps, err := m.config.GetQuoteWidthBps(chainID)
quoteWidthBps, err := m.config.GetQuoteWidthBps(input.DestChainID)
if err != nil {
return nil, fmt.Errorf("error getting quote width bps: %w", err)
}
totalOffsetBps := quoteOffsetBps + quoteWidthBps
totalOffsetBps := originOffsetBps + destOffsetBps + quoteWidthBps
destAmount := m.applyOffset(ctx, totalOffsetBps, originAmount)

span.SetAttributes(
attribute.Float64("quote_offset_bps", quoteOffsetBps),
attribute.Float64("origin_offset_bps", originOffsetBps),
attribute.Float64("dest_offset_bps", destOffsetBps),
attribute.Float64("quote_width_bps", quoteWidthBps),
attribute.Float64("total_offset_bps", totalOffsetBps),
attribute.String("dest_amount", destAmount.String()),
)
return destAmount, nil
Expand Down
104 changes: 76 additions & 28 deletions services/rfq/relayer/quoter/quoter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,35 @@ func (s *QuoterSuite) TestIsProfitable() {
// Set fee to less than breakeven; i.e. destAmount < originAmount - fee.
quote.Transaction.DestAmount = balance
s.False(s.manager.IsProfitable(s.GetTestContext(), quote))

origin := int(s.origin)
dest := int(s.destination)
setQuoteOffsets := func(originOffset, destOffset float64) {
originTokenCfg := s.config.Chains[origin].Tokens["USDC"]
originTokenCfg.QuoteOffsetBps = originOffset
s.config.Chains[origin].Tokens["USDC"] = originTokenCfg
destTokenCfg := s.config.Chains[dest].Tokens["USDC"]
destTokenCfg.QuoteOffsetBps = destOffset
s.config.Chains[dest].Tokens["USDC"] = destTokenCfg
s.manager.SetConfig(s.config)
}
quote.Transaction.DestAmount = new(big.Int).Sub(balance, fee)

// Set dest offset to 20%; we send a token that is more valuable -> not profitable
setQuoteOffsets(0, 2000)
s.False(s.manager.IsProfitable(s.GetTestContext(), quote))

// Set dest offset to -20%; we send a token that is less valuable -> profitable
setQuoteOffsets(0, -2000)
s.True(s.manager.IsProfitable(s.GetTestContext(), quote))

// Set origin offset to 20%; we get a token that is more valuable -> not profitable
setQuoteOffsets(2000, 0)
s.True(s.manager.IsProfitable(s.GetTestContext(), quote))

// Set origin offset to -20%; we send a token that is less valuable -> not profitable
setQuoteOffsets(-2000, 0)
s.False(s.manager.IsProfitable(s.GetTestContext(), quote))
}

func (s *QuoterSuite) TestGetOriginAmount() {
Expand Down Expand Up @@ -229,18 +258,6 @@ func (s *QuoterSuite) TestGetOriginAmount() {
expectedAmount = big.NewInt(500_000_000)
s.Equal(expectedAmount, quoteAmount)

// Set QuotePct to 50 with QuoteOffset of -1%. Should be 1% less than 50% of balance.
setQuoteParams(quoteParams{
quotePct: 50,
quoteOffset: -100,
minQuoteAmount: "0",
maxBalance: "0",
})
quoteAmount, err = s.manager.GetOriginAmount(s.GetTestContext(), input)
s.NoError(err)
expectedAmount = big.NewInt(495_000_000)
s.Equal(expectedAmount, quoteAmount)

// Set QuotePct to 25 with MinQuoteAmount of 500; should be 50% of balance.
setQuoteParams(quoteParams{
quotePct: 25,
Expand Down Expand Up @@ -328,53 +345,84 @@ func (s *QuoterSuite) setGasSufficiency(sufficient bool) {
func (s *QuoterSuite) TestGetDestAmount() {
balance := big.NewInt(1000_000_000) // 1000 USDC

chainID := int(s.destination)
setQuoteParams := func(quoteOffsetBps, quoteWidthBps float64) {
origin := int(s.origin)
dest := int(s.destination)
input := quoter.QuoteInput{
OriginChainID: int(s.origin),
DestChainID: int(s.destination),
OriginBalance: balance,
DestBalance: balance,
}
setQuoteParams := func(originQuoteOffsetBps, destQuoteOffsetBps, quoteWidthBps float64) {
s.config.BaseChainConfig.QuoteWidthBps = quoteWidthBps
tokenCfg := s.config.Chains[chainID].Tokens["USDC"]
tokenCfg.QuoteOffsetBps = quoteOffsetBps
s.config.Chains[chainID].Tokens["USDC"] = tokenCfg
tokenCfg := s.config.Chains[origin].Tokens["USDC"]
tokenCfg.QuoteOffsetBps = originQuoteOffsetBps
s.config.Chains[origin].Tokens["USDC"] = tokenCfg
tokenCfg = s.config.Chains[dest].Tokens["USDC"]
tokenCfg.QuoteOffsetBps = destQuoteOffsetBps
s.config.Chains[dest].Tokens["USDC"] = tokenCfg
s.manager.SetConfig(s.config)
}

// Set default quote params; should return the balance.
destAmount, err := s.manager.GetDestAmount(s.GetTestContext(), balance, chainID, "USDC")
destAmount, err := s.manager.GetDestAmount(s.GetTestContext(), balance, "USDC", input)
s.NoError(err)
expectedAmount := balance
s.Equal(expectedAmount, destAmount)

// Set QuoteWidthBps to 100, should return 99% of balance.
setQuoteParams(0, 100)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, chainID, "USDC")
setQuoteParams(0, 0, 100)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, "USDC", input)
s.NoError(err)
expectedAmount = big.NewInt(990_000_000)
s.Equal(expectedAmount, destAmount)

// Set QuoteWidthBps to 500, should return 95% of balance.
setQuoteParams(0, 500)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, chainID, "USDC")
setQuoteParams(0, 0, 500)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, "USDC", input)
s.NoError(err)
expectedAmount = big.NewInt(950_000_000)
s.Equal(expectedAmount, destAmount)

// Set QuoteWidthBps to 500 and QuoteOffsetBps to 100, should return 94% of balance.
setQuoteParams(100, 500)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, chainID, "USDC")
setQuoteParams(0, 100, 500)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, "USDC", input)
s.NoError(err)
expectedAmount = big.NewInt(940_000_000)
s.Equal(expectedAmount, destAmount)

// Set QuoteWidthBps to 500 and QuoteOffsetBps to -100, should return 96% of balance.
setQuoteParams(-100, 500)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, chainID, "USDC")
setQuoteParams(0, -100, 500)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, "USDC", input)
s.NoError(err)
expectedAmount = big.NewInt(960_000_000)
s.Equal(expectedAmount, destAmount)

// Set QuoteWidthBps to -100, should default to balance.
setQuoteParams(0, -100)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, chainID, "USDC")
setQuoteParams(0, 0, -100)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, "USDC", input)
s.NoError(err)
expectedAmount = balance
s.Equal(expectedAmount, destAmount)

// Set origin offset to 100, should return 101% of balance.
setQuoteParams(100, 0, 0)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, "USDC", input)
s.NoError(err)
expectedAmount = big.NewInt(1_010_000_000)
s.Equal(expectedAmount, destAmount)

// Set origin offset to -100, should return 99% of balance.
setQuoteParams(-100, 0, 0)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, "USDC", input)
s.NoError(err)
expectedAmount = big.NewInt(990_000_000)
s.Equal(expectedAmount, destAmount)

// Set origin offset to 100, dest offset to 300, should return 98% of balance.
setQuoteParams(100, 300, 0)
destAmount, err = s.manager.GetDestAmount(s.GetTestContext(), balance, "USDC", input)
s.NoError(err)
expectedAmount = big.NewInt(980_000_000)
s.Equal(expectedAmount, destAmount)
}
Loading