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

Jump to bottom

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 4 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
42 changes: 35 additions & 7 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)
}
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)
}
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()),
)

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(uint32(chainID), tokenAddr.Hex())

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

View workflow job for this annotation

GitHub Actions / Lint (services/rfq) 

unnecessary conversion (unconvert)
if err != nil {
return nil, fmt.Errorf("error getting token name: %w", err)
}
// apply offset directly to amount without considering origin/dest
quoteOffsetBps, err := m.config.GetQuoteOffsetBps(int(chainID), tokenName, false)
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

Avoid unsafe conversion from 'uint32' to 'int' for 'chainID'

Converting chainID from uint32 to int can lead to data loss or overflow if chainID exceeds the maximum value of int. It's safer to use consistent types.

Apply this diff to fix the issue:

-quoteOffsetBps, err := m.config.GetQuoteOffsetBps(int(chainID), tokenName, false)
+quoteOffsetBps, err := m.config.GetQuoteOffsetBps(chainID, tokenName, false)

Ensure the GetQuoteOffsetBps method accepts uint32 for chainID.

Committable suggestion was skipped due to low confidence.

if err != nil {
return nil, fmt.Errorf("error getting quote offset bps: %w", err)
}
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.


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

// SubmitAllQuotes submits all quotes to the RFQ API.
Expand Down
29 changes: 29 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
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 get a token that is more valuable -> still profitable
setQuoteOffsets(0, 2000)
s.True(s.manager.IsProfitable(s.GetTestContext(), quote))

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

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

// Set origin offset to -20%; we send a token that is less valuable -> still profitable
setQuoteOffsets(-2000, 0)
s.True(s.manager.IsProfitable(s.GetTestContext(), quote))
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

Consider defining offset values as constants for clarity and maintainability.

Defining the offset values (2000, -2000) as named constants will enhance readability and make it easier to adjust the values in the future.

Here's how you could define the constants:

+	const (
+		positiveOffsetBps = 2000  // +20% offset
+		negativeOffsetBps = -2000 // -20% offset
+	)

And update the function calls:

-	setQuoteOffsets(0, 2000)
+	setQuoteOffsets(0, positiveOffsetBps)

-	setQuoteOffsets(0, -2000)
+	setQuoteOffsets(0, negativeOffsetBps)

-	setQuoteOffsets(2000, 0)
+	setQuoteOffsets(positiveOffsetBps, 0)

-	setQuoteOffsets(-2000, 0)
+	setQuoteOffsets(negativeOffsetBps, 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
// Set dest offset to 20%; we get a token that is more valuable -> still profitable
setQuoteOffsets(0, 2000)
s.True(s.manager.IsProfitable(s.GetTestContext(), quote))
// Set dest offset to -20%; we get a token that is less valuable -> no longer profitable
setQuoteOffsets(0, -2000)
s.False(s.manager.IsProfitable(s.GetTestContext(), quote))
// Set origin offset to 20%; we send a token that is more valuable -> no longer profitable
setQuoteOffsets(2000, 0)
s.False(s.manager.IsProfitable(s.GetTestContext(), quote))
// Set origin offset to -20%; we send a token that is less valuable -> still profitable
setQuoteOffsets(-2000, 0)
s.True(s.manager.IsProfitable(s.GetTestContext(), quote))
const (
positiveOffsetBps = 2000 // +20% offset
negativeOffsetBps = -2000 // -20% offset
)
// Set dest offset to 20%; we get a token that is more valuable -> still profitable
setQuoteOffsets(0, positiveOffsetBps)
s.True(s.manager.IsProfitable(s.GetTestContext(), quote))
// Set dest offset to -20%; we get a token that is less valuable -> no longer profitable
setQuoteOffsets(0, negativeOffsetBps)
s.False(s.manager.IsProfitable(s.GetTestContext(), quote))
// Set origin offset to 20%; we send a token that is more valuable -> no longer profitable
setQuoteOffsets(positiveOffsetBps, 0)
s.False(s.manager.IsProfitable(s.GetTestContext(), quote))
// Set origin offset to -20%; we send a token that is less valuable -> still profitable
setQuoteOffsets(negativeOffsetBps, 0)
s.True(s.manager.IsProfitable(s.GetTestContext(), quote))

}

func (s *QuoterSuite) TestGetOriginAmount() {
Expand Down
Loading