feat(rfq-relayer): apply zap fee to dest amount for active quotes [SLT-465] #3395
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="[config_reader] The configuration option 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 @@
## feat/arb-call-fee-pricer #3395 +/- ##
==================================================================
+ Coverage 30.58729% 30.87471% +0.28742%
==================================================================
Files 457 457
Lines 30019 30193 +174
Branches 82 82
==================================================================
+ Hits 9182 9322 +140
- Misses 19986 19993 +7
- Partials 851 878 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
40eb027
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2952e095.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-zap-quote-pricing.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: 2
🧹 Outside diff range and nitpick comments (3)
services/rfq/relayer/quoter/quoter_test.go (2)
450-534: Enhance test readability and coverageThe new test method could be improved in several ways:
- Add comments explaining the test scenarios and expected outcomes
- Document the significance of magic numbers (e.g., "500000000000", "499899950000")
- Consider using table-driven tests to cover more scenarios
- Add test cases for error conditions
Here's a suggested refactor:
func (s *QuoterSuite) TestGenerateActiveRFQ() { + // Test cases for different RFQ scenarios + testCases := []struct { + name string + originAmount string + expectedDest string + description string + }{ + { + name: "small_amount", + originAmount: "100000", + expectedDest: "0", + description: "Small amount below minimum threshold returns zero", + }, + { + name: "large_amount", + originAmount: "500000000000", + expectedDest: "499899950000", + description: "Large amount gets processed with fee deduction", + }, + } + origin := int(s.origin) dest := int(s.destination) originAddr := common.HexToAddress("0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48") // ... setup code ... - req := model.PutRFQRequest{ - UserAddress: "0x123", - // ... first test case ... - } - // ... test execution ... - - req = model.PutRFQRequest{ - UserAddress: "0x123", - // ... second test case ... - } - // ... test execution ... + for _, tc := range testCases { + s.Run(tc.name, func() { + // Test description for clarity + s.T().Logf("Testing %s: %s", tc.name, tc.description) + + req := model.PutRFQRequest{ + UserAddress: "0x123", + IntegratorID: "123", + QuoteTypes: []string{"active"}, + Data: model.QuoteData{ + OriginChainID: origin, + DestChainID: dest, + OriginAmountExact: tc.originAmount, + OriginTokenAddr: originAddr.String(), + DestTokenAddr: destAddr.String(), + }, + } + // ... test execution ... + s.Equal(tc.expectedDest, resp.DestAmount) + }) + } }
465-470: Document gas price assumptionsThe test uses hardcoded gas price values without explaining their significance. Consider adding comments to explain these values and their impact on the test outcomes.
- currentHeader := big.NewInt(100_000_000_000) // 100 gwei + // Set gas price to 100 gwei to simulate moderate network congestion + currentHeader := big.NewInt(100_000_000_000) + + // Simulate standard gas consumption for RFQ operations + expectedGasUsage := uint64(100_000)services/rfq/relayer/quoter/quoter.go (1)
401-414: Refactor repetitive big integer parsing codeThe code for parsing
rawQuote.DestAmountandrawQuote.FixedFeeinto*big.Intand handling errors is repetitive. Consider extracting this logic into a helper function to improve maintainability and reduce duplication.Apply this diff to refactor the code:
+func parseBigInt(value string, fieldName string) (*big.Int, error) { + bigIntValue, ok := new(big.Int).SetString(value, 10) + if !ok { + return nil, fmt.Errorf("invalid %s: %s", fieldName, value) + } + return bigIntValue, nil +} ... // 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) - } + destAmountBigInt, err := parseBigInt(rawQuote.DestAmount, "dest amount") + if err != nil { + return nil, err + } - fixedFeeBigInt, ok := new(big.Int).SetString(rawQuote.FixedFee, 10) - if !ok { - return nil, fmt.Errorf("invalid fixed fee: %s", rawQuote.FixedFee) - } + fixedFeeBigInt, err := parseBigInt(rawQuote.FixedFee, "fixed fee") + if err != nil { + return nil, err + } 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))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
services/rfq/api/model/request.go(0 hunks)services/rfq/relayer/quoter/export_test.go(1 hunks)services/rfq/relayer/quoter/quoter.go(2 hunks)services/rfq/relayer/quoter/quoter_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- services/rfq/api/model/request.go
🔇 Additional comments (2)
services/rfq/relayer/quoter/export_test.go (1)
25-27: LGTM! The test export follows established patterns.
The new GenerateActiveRFQ method follows the consistent pattern used throughout the file for exposing internal methods for testing purposes. The implementation is clean and straightforward.
services/rfq/relayer/quoter/quoter_test.go (1)
471-473: Add error case testing for price fetcher
The price fetcher mock is set up to always return an error, but the test doesn't verify how the system handles this error condition.
| func (m *Manager) GenerateActiveRFQ(ctx context.Context, msg *model.ActiveRFQMessage) (resp *model.ActiveRFQMessage, err error) { | ||
| return m.generateActiveRFQ(ctx, msg) | ||
| } |
There was a problem hiding this comment.
💡 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.goforaddZapFeeswith 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
| 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), | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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, | |
| }, | |
| } | |
| } |
…SLT-320] (#3299) * WIP: incorporate call params into fee * WIP: decompose getFee() * Feat: account for CallValue in getDestinationFee() * Fix: build * Feat: test call value and call param calculation in fee pricer * Feat: add context on request body in rpc fwd errs * Fix: zap estimate gas * Cleanup: move gas estimate into own func * Fix: quoter tests * Cleanup: lint * Cleanup: lint * Fix: tests * Cleanup: decompose func * Cleanup: lint * Fix: tests * Cleanup: lint * Feat: always use quote fee multiplier * WIP: abi encode pack relay() * Feat: pass full RawRequest for gas estimation * Cleanup: lint * Fix: pricer tests * Feat: ignore static l2 fee when incorporating call params * Fix: tests * Clarifying comment * Feat: add extra check for call param len * Attempt to fix flake * Cleanup: lint * Fix: build * feat(rfq-relayer): apply zap fee to dest amount for active quotes [SLT-465] (#3395) * Feat: set zap params in supply to fee pricer * Feat: adjust dest amount by fee for active quote * Feat: add TestGenerateActiveRFQ * Feat: add nonzero case * Cleanup: lint * fix(rfq-relayer): gas estimation for zaps (#3413) * WIP: use QuoteData instead of QuoteRequest * WIP: impl conversion * Feat: use QuoteRequest instead of QuoteData for encoding * WIP: another encoding impl * WIP: add bridgetransactionv2 * WIP: regenerate with running test * Working zap e2e * Cleanup: encoding * Cleanup: move to new encoding file * ABIgen from harness instead of lib * Cleanup: lint * Fix: CI build * Fix: deploy harness * Fix: rfq test * Feat: use gofakeit to mock * Cleanup: test structure * Replace: EncodeQuoteRequest -> EncodeBridgeTx * Feat: add Decode func and add to parity test * Fix: eth address mocks * Cleanup: lint * Cleanup: lint --------- Co-authored-by: parodime <[email protected]>
Summary by CodeRabbit
New Features
ZapNativeandZapData).Bug Fixes
Tests