feat(rfq-api): active api schema changes #3248
Conversation
WalkthroughThe changes in this pull request involve the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
|
Deploying sanguine-fe with
|
| Latest commit: |
994cd52
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://07a346d0.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-active-api-fixes.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- services/rfq/api/model/request.go (0 hunks)
- services/rfq/api/rest/server.go (1 hunks)
- services/rfq/relayer/quoter/quoter.go (1 hunks)
💤 Files with no reviewable changes (1)
- services/rfq/api/model/request.go
🧰 Additional context used
🔇 Additional comments (1)
services/rfq/api/rest/server.go (1)
560-562: Improved logic for determining quote type. Verify impact on dependent systems.The changes improve the accuracy of determining the quote type by setting it to "passive" by default and only changing to "active" when the selected quote matches the active quote. This is a more precise approach.
To ensure this change doesn't negatively impact other parts of the system, please run the following verification steps:
Please review the output of these commands to ensure that all related code and tests are updated accordingly.
✅ Verification successful
Quote type determination logic verified. No impact found on dependent systems or tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any code that might be relying on the previous quote type determination logic # Test 1: Search for usages of quoteTypeActive and quoteTypePassive echo "Searching for usages of quoteTypeActive and quoteTypePassive:" rg --type go "quoteTypeActive|quoteTypePassive" # Test 2: Search for any code that might be making assumptions about how quote types are determined echo "Searching for potential assumptions about quote type determination:" rg --type go "quote.*type|type.*quote" # Test 3: Check for any tests that might need updating due to this change echo "Checking for tests that might need updating:" rg --type go -t test "quoteType|quote.*type|type.*quote"Length of output: 1244
Script:
#!/bin/bash # Description: Check for any tests that might need updating due to this change # Test 3: Check for any tests that might need updating due to this change echo "Checking for tests that might need updating:" rg --glob "*_test.go" "quoteType|quote.*type|type.*quote"Length of output: 158
| fixedFee, ok := new(big.Int).SetString(rawQuote.FixedFee, 10) | ||
| if !ok { | ||
| return nil, fmt.Errorf("error parsing fixed fee: %s", rawQuote.FixedFee) | ||
| } | ||
| rawDestAmount, ok := new(big.Int).SetString(rawQuote.DestAmount, 10) | ||
| if !ok { | ||
| return nil, fmt.Errorf("error parsing dest amount: %s", rawQuote.DestAmount) | ||
| } | ||
| destAmountNet := new(big.Int).Sub(rawDestAmount, fixedFee) | ||
| span.SetAttributes( | ||
| attribute.String("dest_amount", rawQuote.DestAmount), | ||
| attribute.String("fixed_fee", rawQuote.FixedFee), | ||
| attribute.String("dest_amount_net", destAmountNet.String()), | ||
| ) | ||
|
|
||
| rfqResp := model.WsRFQResponse{ | ||
| RequestID: rfqRequest.RequestID, | ||
| DestAmount: rawQuote.DestAmount, | ||
| DestAmount: destAmountNet.String(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and consider using decimal arithmetic for financial calculations.
The changes introduce logic to calculate the net destination amount after deducting a fixed fee. While the implementation is generally correct, there are a few areas that could be improved:
-
Error handling: The code uses
if !okchecks for parsing strings to big.Int. Consider using more descriptive error messages or custom errors for better debugging. -
Precision concerns: Financial calculations often require high precision. Consider using a decimal arithmetic library like
shopspring/decimalfor more accurate calculations, especially when dealing with fees and amounts. -
Negative amount check: There's no check to ensure that the
destAmountNetdoesn't become negative after subtracting the fixed fee. This could lead to unexpected behavior.
Here's a suggested refactoring using the shopspring/decimal library:
import (
"github.com/shopspring/decimal"
// ... other imports
)
// ... inside generateActiveRFQ method
fixedFee, err := decimal.NewFromString(rawQuote.FixedFee)
if err != nil {
return nil, fmt.Errorf("error parsing fixed fee: %w", err)
}
rawDestAmount, err := decimal.NewFromString(rawQuote.DestAmount)
if err != nil {
return nil, fmt.Errorf("error parsing dest amount: %w", err)
}
destAmountNet := rawDestAmount.Sub(fixedFee)
if destAmountNet.IsNegative() {
return nil, fmt.Errorf("net destination amount is negative: %s", destAmountNet)
}
span.SetAttributes(
attribute.String("dest_amount", rawQuote.DestAmount),
attribute.String("fixed_fee", rawQuote.FixedFee),
attribute.String("dest_amount_net", destAmountNet.String()),
)
rfqResp := model.WsRFQResponse{
RequestID: rfqRequest.RequestID,
DestAmount: destAmountNet.String(),
}This refactoring improves precision, error handling, and adds a check for negative net amounts.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3248 +/- ##
============================================
Coverage ? 25.79828%
============================================
Files ? 97
Lines ? 3946
Branches ? 82
============================================
Hits ? 1018
Misses ? 2920
Partials ? 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Summary by CodeRabbit
QuoteRequeststruct to simplify the model without affecting existing structures.