fix(rfq-relayer): profitability check accounts for offsets #3288
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🔇 Additional comments (6)
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3288 +/- ##
===================================================
- Coverage 25.89782% 25.00000% -0.89782%
===================================================
Files 97 198 +101
Lines 3954 13132 +9178
Branches 82 82
===================================================
+ Hits 1024 3283 +2259
- Misses 2922 9563 +6641
- Partials 8 286 +278
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
services/rfq/relayer/quoter/quoter.go (1)
266-266: Remove redundant type conversion of 'chainID'The variable
chainIDis already of typeuint32. Casting it again touint32is unnecessary.Apply this diff to simplify the code:
-tokenName, err := m.config.GetTokenName(uint32(chainID), tokenAddr.Hex()) +tokenName, err := m.config.GetTokenName(chainID, tokenAddr.Hex())
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- services/rfq/relayer/quoter/quoter.go (1 hunks)
- services/rfq/relayer/quoter/quoter_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
services/rfq/relayer/quoter/quoter_test.go (1)
175-185: Good use of helper functionsetQuoteOffsetsfor configuration adjustments.Encapsulating the quote offset configuration into the
setQuoteOffsetsfunction improves code readability and reduces code duplication.
| // 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)) |
There was a problem hiding this comment.
🛠️ 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.
| // 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)) |
| 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) | ||
| } |
There was a problem hiding this comment.
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.
| 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") | |
| } |
| 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) |
There was a problem hiding this comment.
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.
| 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) | ||
| } |
There was a problem hiding this comment.
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.
| 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") | |
| } |
| if err != nil { | ||
| return nil, fmt.Errorf("error getting quote offset bps: %w", err) | ||
| } | ||
| amountAdj := m.applyOffset(ctx, quoteOffsetBps, amount) |
There was a problem hiding this comment.
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.
Deploying sanguine-fe with
|
| Latest commit: |
d682f96
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1b7e448e.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-profitability-check.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/rfq/relayer/quoter/quoter.go (1)
Line range hint
837-852: Potential precision loss in offset calculationWhile the
applyOffsetfunction remains largely unchanged, there's a potential issue with precision loss due to castingoffsetBpsfromfloat64toint64. This can affect the accuracy of the offset calculations.To improve precision, consider modifying the function as follows:
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
offsetBpsand ensures more accurate calculations.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 245-246: services/rfq/relayer/quoter/quoter.go#L245-L246
Added lines #L245 - L246 were not covered by tests
[warning] 250-251: services/rfq/relayer/quoter/quoter.go#L250-L251
Added lines #L250 - L251 were not covered by tests
[warning] 268-269: services/rfq/relayer/quoter/quoter.go#L268-L269
Added lines #L268 - L269 were not covered by tests
[warning] 273-274: services/rfq/relayer/quoter/quoter.go#L273-L274
Added lines #L273 - L274 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- services/rfq/relayer/quoter/export_test.go (1 hunks)
- services/rfq/relayer/quoter/quoter.go (3 hunks)
- services/rfq/relayer/quoter/quoter_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/quoter/quoter_test.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/relayer/quoter/quoter.go
[warning] 245-246: services/rfq/relayer/quoter/quoter.go#L245-L246
Added lines #L245 - L246 were not covered by tests
[warning] 250-251: services/rfq/relayer/quoter/quoter.go#L250-L251
Added lines #L250 - L251 were not covered by tests
[warning] 268-269: services/rfq/relayer/quoter/quoter.go#L268-L269
Added lines #L268 - L269 were not covered by tests
[warning] 273-274: services/rfq/relayer/quoter/quoter.go#L273-L274
Added lines #L273 - L274 were not covered by tests
🔇 Additional comments (4)
services/rfq/relayer/quoter/export_test.go (1)
21-22: LGTM. Verify usage across the codebase.The updated function signature looks good. It removes the
chainIDparameter and introduces a more comprehensiveinput QuoteInputparameter, which likely provides more flexibility and information for quote generation.To ensure this change doesn't break existing code, please run the following script to verify all calls to
GetDestAmounthave been updated:✅ Verification successful
To ensure that all calls to
GetDestAmounthave been updated to match the new signature, please run the following script:
Verification Successful.
All calls to
GetDestAmounthave been updated to match the new signature with theinput QuoteInputparameter. No instances of the old signature were found that could cause issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to GetDestAmount match the new signature # Test: Search for function calls to GetDestAmount. # Expect: Only occurrences of the new signature (4 parameters, with QuoteInput as the last one). rg --type go -e 'GetDestAmount\s*\([^)]*\)' -g '!*_test.go'Length of output: 445
Script:
#!/bin/bash # Description: Find all calls to GetDestAmount with fewer than 4 parameters rg --type go -e 'GetDestAmount\s*\(([^,]*,){0,2}[^,]*\)' -g '!*_test.go'Length of output: 458
services/rfq/relayer/quoter/quoter.go (3)
Line range hint
611-624: Updated quote generation logicThe changes to the
generateQuotefunction correctly incorporate the newgetDestAmountmethod, which now takes additional parameters (destTokenandinput). This update aligns well with the enhanced quote calculation logic that considers offsets.
804-835: Enhanced destination amount calculation with multiple offsetsThe
getDestAmountfunction has been significantly improved:
- It now considers origin, destination, and quote width offsets, providing a more accurate calculation of the destination amount.
- The use of the
QuoteInputstruct enhances code readability and maintainability.- Error handling is consistent and appropriate throughout the function.
- Span attributes have been updated to include the new offset values, improving observability.
These changes result in a more robust and precise quote calculation process.
242-263:⚠️ Potential issueImproved profitability calculation with offset adjustments.
The changes to the
IsProfitablefunction enhance the accuracy of profitability calculations by considering internal offsets for both origin and destination token values. This is a good improvement to the logic.However, there's a potential issue with nil pointer dereferences. Please add nil checks for
originAmountAdjandcostAdjbefore using them in the comparison.Apply this diff to add nil checks:
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) } +if costAdj == nil { + return false, fmt.Errorf("costAdj is nil") +}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 245-246: services/rfq/relayer/quoter/quoter.go#L245-L246
Added lines #L245 - L246 were not covered by tests
[warning] 250-251: services/rfq/relayer/quoter/quoter.go#L250-L251
Added lines #L250 - L251 were not covered by tests
| 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) | ||
| } | ||
| // apply offset directly to amount without considering origin/dest | ||
| quoteOffsetBps, err := m.config.GetQuoteOffsetBps(int(chainID), tokenName, false) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting quote offset bps: %w", err) | ||
| } | ||
| amountAdj := m.applyOffset(ctx, quoteOffsetBps, amount) | ||
|
|
||
| return amountAdj, nil |
There was a problem hiding this comment.
New function for offset-adjusted amounts
The getAmountWithOffset function is a good addition that calculates adjusted amounts based on offsets. It improves the accuracy of quote calculations.
However, there are two issues to address:
- Unsafe type conversion:
The GetQuoteOffsetBps call involves an unsafe conversion from uint32 to int for chainID. This can lead to data loss or overflow if chainID exceeds the maximum value of int. 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.
- Missing nil check:
Add a nil check after calling applyOffset to handle potential nil returns. Apply this diff:
amountAdj := m.applyOffset(ctx, quoteOffsetBps, amount)
+if amountAdj == nil {
+ return nil, fmt.Errorf("adjusted amount is nil")
+}
return amountAdj, nilCommittable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 268-269: services/rfq/relayer/quoter/quoter.go#L268-L269
Added lines #L268 - L269 were not covered by tests
[warning] 273-274: services/rfq/relayer/quoter/quoter.go#L273-L274
Added lines #L273 - L274 were not covered by tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests