feat(rfq-relayer): fee pricer considers v2 CallValue and CallParams [SLT-320] #3299
Conversation
WalkthroughThe changes primarily involve enhancements to the Changes
Possibly related PRs
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/relayer-arb-call #3299 +/- ##
================================================================
- Coverage 32.93100% 21.86949% -11.06151%
================================================================
Files 107 580 +473
Lines 4391 47660 +43269
Branches 82 82
================================================================
+ Hits 1446 10423 +8977
- Misses 2939 36194 +33255
- Partials 6 1043 +1037
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: |
d9db854
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://852da776.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-arb-call-fee-pricer.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: 10
🧹 Outside diff range and nitpick comments (8)
services/rfq/relayer/pricer/fee_pricer.go (4)
Line range hint
275-313: Correct the Fee Conversion Logic ingetDenomFeeIn the
getDenomFeefunction, there is a logical error in converting the fee from USD to the denominated token. The calculation multipliesfeeUSDbydenomTokenPrice, which is incorrect:feeUSDC := new(big.Float).Mul(feeUSD, new(big.Float).SetFloat64(denomTokenPrice))Since
denomTokenPricerepresents the price of the denominated token in USD, multiplyingfeeUSDbydenomTokenPriceresults in the fee being in USD squared per token, which is unintended.To correctly convert the fee from USD to the denominated token amount, you should divide
feeUSDbydenomTokenPrice:- feeUSDC := new(big.Float).Mul(feeUSD, new(big.Float).SetFloat64(denomTokenPrice)) + feeDenom := new(big.Float).Quo(feeUSD, new(big.Float).SetFloat64(denomTokenPrice))Ensure that you update variable names accordingly and adjust any subsequent calculations that use
feeDenom.
Line range hint
55-71: Update Constructor Documentation and Default ParametersThe
NewFeePricerconstructor now includes a new parameterrelayerAddress common.Address:func NewFeePricer(config relconfig.Config, clientFetcher submitter.ClientFetcher, priceFetcher CoingeckoPriceFetcher, handler metrics.Handler, relayerAddress common.Address) FeePricer { // ... relayerAddress: relayerAddress, }Consider updating the function documentation to reflect the new parameter. Additionally, if there are default or common values for
relayerAddress, consider providing guidance or default handling within the constructor.
Line range hint
125-197: Optimize Gas Estimation and Error HandlingIn the
GetDestinationFeemethod, when estimating gas for the call:gasEstimate, err := client.EstimateGas(ctx, callMsg) if err != nil { return nil, fmt.Errorf("could not estimate gas: %w", err) }Consider implementing retries or fallback mechanisms for gas estimation failures, as network issues or transient errors could cause the estimation to fail. Additionally, logging detailed error information could aid in debugging.
Line range hint
323-346: Uniform Error Messages and LoggingIn the
getFeeWithMultipliermethod, error messages vary in specificity:return nil, fmt.Errorf("could not get quote fixed fee multiplier: %w", err) // vs. return nil, fmt.Errorf("could not get relay fixed fee multiplier: %w", err)Standardize error messages for consistency. This practice improves log readability and aids in debugging.
services/rfq/relayer/pricer/fee_pricer_test.go (3)
20-20: Use of Hardcoded Relayer AddressThe
relayerAddressis hardcoded with the value0x1234567890123456789012345678901234567890. While this is acceptable for testing purposes, consider defining this address as a constant or adding a comment to clarify its intended use in the test context.
244-244: Provide Meaningful Data forCallParams
CallParamsis initialized as an empty byte slice. To better simulate real-world scenarios, consider providing sample data that represents typical call parameters.
339-339: Ensure Comprehensive Testing of Additional ParameterConsider testing
GetTotalFeewith non-niltransaction data when the multiplier is less than one to ensure consistency across different configurations.services/rfq/relayer/quoter/quoter.go (1)
340-340: Address the TODO: Incorporate user call dataThe TODO comment indicates that user call data should be incorporated into the quote request, and the
Transactionfield should be set accordingly. This is important for ensuring that transaction-specific data is utilized in fee calculations.Would you like assistance in implementing this functionality or opening a GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- services/rfq/relayer/pricer/fee_pricer.go (11 hunks)
- services/rfq/relayer/pricer/fee_pricer_test.go (12 hunks)
- services/rfq/relayer/quoter/quoter.go (5 hunks)
- services/rfq/relayer/service/relayer.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (19)
services/rfq/relayer/pricer/fee_pricer_test.go (18)
44-44: Consistent Update toFeePricerInitializationThe
FeePriceris now initialized with therelayerAddressparameter. Ensure that this change is consistently applied across all test cases and that therelayerAddressis correctly utilized within theFeePricerimplementation.
88-88: Consistent Update toFeePricerInitializationThe inclusion of
relayerAddressin theFeePricerinitialization is appropriate. Confirm that any dependent functionality withinFeePricercorrectly handles therelayerAddress.
129-129: Consistent Update toFeePricerInitializationUpdating the
FeePricerconstructor withrelayerAddressmaintains consistency across test cases.
152-152: Caching Mechanism VerificationGood job verifying that the fee is cached by simulating a failure in fetching the gas price and ensuring the cached fee is returned.
173-173: Consistent Update toFeePricerInitializationThe
FeePricerinitialization includes therelayerAddress, maintaining consistency.
177-177: Testing with OverridesThe test correctly calculates the destination fee with overridden gas estimates. Ensure that these overrides reflect realistic scenarios.
201-201: Caching Mechanism VerificationVerifying the caching mechanism by simulating errors strengthens test reliability.
213-216: Mocking Clients for Origin and Destination ChainsProperly mocking
clientOriginandclientDestinationensures isolated testing of gas prices for different chains.
218-218: Consistent Update toFeePricerInitializationIncluding
relayerAddressin the initialization maintains consistency.
229-253: Addition of Test Cases CoveringCallValueandCallParamsThe new test cases in
TestGetTotalFeeeffectively cover scenarios withCallValueandCallParams, enhancing test coverage for fee calculations involving transaction-specific data.
246-246: MockingEstimateGasfor Call withCallParamsMocking
EstimateGasis appropriate here. Ensure that error scenarios are also tested to validate error handling in fee calculation when gas estimation fails.
266-266: Consistent Update toFeePricerInitializationThe inclusion of
relayerAddressmaintains consistency inFeePricerinitialization across tests.
308-308: Consistent Update toFeePricerInitializationThe
FeePriceris correctly initialized with the new parameter.
312-312: Testing Fee Multiplier Greater Than OneThe test accurately checks fee calculations when the fixed fee multiplier is greater than one, ensuring the multiplier logic functions as expected.
320-320: Testing Fee Multiplier for Relay FeesTesting with
isQuote=falsevalidates the relay fee multiplier, ensuring both quote and relay calculations are correct.
335-335: Testing Fee Multiplier Less Than OneBy setting the multiplier to less than one, the test ensures the default behavior correctly handles invalid multipliers.
354-354: Consistent Update toFeePricerInitializationRe-initializing
FeePricerwith the updated configuration continues to maintain test consistency.
358-358: Testing Fee Multiplier Set to ZeroThe test confirms that setting the multiplier to zero defaults it to one, ensuring robustness in handling edge cases.
services/rfq/relayer/quoter/quoter.go (1)
24-24: Import statement is appropriateThe addition of the
fastbridgev2import is necessary for accessing theIFastBridgeV2BridgeTransactionV2interface.
| feeScaled, _ = new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil) | ||
|
|
There was a problem hiding this comment.
Specify Rounding Behavior When Applying Fee Multiplier
In the getFeeWithMultiplier method, the conversion from big.Float to big.Int truncates towards zero:
feeScaled, _ = new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil)This truncation may lead to underestimating the fee, potentially resulting in a smaller fee than intended.
Consider specifying the rounding mode to ensure that the fee is rounded up, preventing undercharging:
feeFloat := new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier))
- feeScaled, _ = feeFloat.Int(nil)
+ feeScaled = new(big.Int)
+ feeFloat.SetMode(big.AwayFromZero)
+ feeFloat.Int(feeScaled)Alternatively, use the Ceil method to always round up:
feeFloat := new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier))
+ feeFloat = feeFloat.SetMode(big.AwayFromZero)
+ feeScaled, _ = feeFloat.Int(nil)Ensure that the rounding behavior complies with your fee policy to accurately reflect the intended fees.
| if tx != nil { | ||
| if tx.CallParams != nil { | ||
| client, err := f.clientFetcher.GetClient(ctx, big.NewInt(int64(destination))) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get client: %w", err) | ||
| } | ||
| callMsg := ethereum.CallMsg{ | ||
| From: f.relayerAddress, | ||
| To: &tx.DestRecipient, | ||
| Value: tx.CallValue, | ||
| Data: tx.CallParams, | ||
| } | ||
| gasEstimate, err := client.EstimateGas(ctx, callMsg) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not estimate gas: %w", err) | ||
| } | ||
| callFee, err := f.getFee(ctx, destination, destination, int(gasEstimate), denomToken, isQuote) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fee = new(big.Int).Add(fee, callFee) | ||
| span.SetAttributes(attribute.String("call_fee", callFee.String())) | ||
| } | ||
|
|
||
| if tx.CallValue != nil { | ||
| callValueFloat := new(big.Float).SetInt(tx.CallValue) | ||
| valueDenom, err := f.getDenomFee(ctx, destination, destination, denomToken, callValueFloat) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| valueScaled, err := f.getFeeWithMultiplier(ctx, destination, isQuote, valueDenom) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fee = new(big.Int).Add(fee, valueScaled) | ||
| span.SetAttributes(attribute.String("value_scaled", valueScaled.String())) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle Potential Nil Pointers and Errors in Transaction Processing
In the GetDestinationFee method, when processing the transaction tx, there are nested conditionals:
if tx != nil {
if tx.CallParams != nil {
// ...
}
if tx.CallValue != nil {
// ...
}
}To prevent potential nil pointer dereferences and improve code clarity, consider:
- Adding comments or logs to handle cases where
txor its fields arenil. - Ensuring that error messages provide sufficient context.
- Validating
txand its fields before use, or handlingnilcases appropriately.
| GetDestinationFee(ctx context.Context, origin, destination uint32, denomToken string, isQuote bool, tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2) (*big.Int, error) | ||
| // GetTotalFee returns the total fee for a given origin and destination chainID, denominated in a given token. | ||
| GetTotalFee(ctx context.Context, origin, destination uint32, denomToken string, isQuote bool) (*big.Int, error) | ||
| GetTotalFee(ctx context.Context, origin, destination uint32, denomToken string, isQuote bool, tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2) (*big.Int, error) |
There was a problem hiding this comment.
💡 Codebase verification
Potential Breaking Changes Confirmed
The FeePricer interface in services/rfq/relayer/pricer/fee_pricer.go has not been updated to include the new tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2 parameter in the GetDestinationFee and GetTotalFee methods. However, several implementations and callers are already passing this new parameter, which will lead to compilation errors.
Action Items:
- Update the
FeePricerinterface to include thetxparameter in bothGetDestinationFeeandGetTotalFeemethods. - Ensure all implementations of the
FeePricerinterface are updated accordingly. - Verify all callers of these methods pass the required
txargument to prevent breaking changes.
Category:
🔗 Analysis chain
Potential Breaking Changes Due to Interface Modification
The method signatures for GetDestinationFee and GetTotalFee in the FeePricer interface have been updated to include a new parameter tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2. This change may cause breaking changes in any implementations of the FeePricer interface and any code that calls these methods.
Please ensure that all implementations of the FeePricer interface are updated to accommodate the new parameter. Additionally, verify that all callers of these methods pass the required tx argument.
You can run the following script to identify all implementations and usages of the updated methods:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of the FeePricer interface and usages of the updated methods.
# Find all Go files in the repository
fd --type f --extension go
# Find implementations of the FeePricer interface
rg -A 5 'type .*FeePricer interface \{'
# Find structs that implement FeePricer
ast-grep --lang go --pattern 'func ($_ *$StructType) GetDestinationFee($_) ($_)' --no-filename
# Find all calls to GetDestinationFee and GetTotalFee
rg '\.(GetDestinationFee|GetTotalFee)\(' -A 2
Length of output: 76698
Script:
#!/bin/bash
# Description: Corrected ast-grep command to find implementations of FeePricer interface methods with the new tx parameter.
ast-grep --lang go --pattern 'func ($_ *$StructType) GetDestinationFee($_, $_) ($_)' -- --no-filename
ast-grep --lang go --pattern 'func ($_ *$StructType) GetTotalFee($_, $_) ($_)' -- --no-filename
Length of output: 323
Script:
#!/bin/bash
# Description: Find all implementations of the FeePricer interface to verify if they have been updated with the new tx parameter.
rg -A 5 'type .*FeePricer interface \{'
ast-grep --lang go --pattern 'func ($_ *$StructType) GetDestinationFee($_, $_) ($_)' -- --no-filename
ast-grep --lang go --pattern 'func ($_ *$StructType) GetTotalFee($_, $_) ($_)' -- --no-filename
Length of output: 1032
Script:
#!/bin/bash
# Description: Find implementations of the FeePricer interface methods with the new tx parameter.
# Search for structs implementing GetDestinationFee with the new tx parameter
ast-grep --lang go --pattern 'func ($_ *$StructType) GetDestinationFee($_, $_, $_) ($_)'
# Search for structs implementing GetTotalFee with the new tx parameter
ast-grep --lang go --pattern 'func ($_ *$StructType) GetTotalFee($_, $_, $_) ($_)'
Length of output: 175
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
services/rfq/relayer/pricer/fee_pricer.go (2)
Line range hint
285-307: Correct the fee conversion from USD todenomTokenIn the
getDenomFeefunction, when convertingfeeUSDtofeeDenom, the code incorrectly multipliesfeeUSDbydenomTokenPrice. To obtain the fee indenomTokenunits, you should dividefeeUSDbydenomTokenPriceand then scale bydenomDecimalsFactor. Multiplying instead of dividing leads to an incorrect fee calculation.Apply this diff to correct the conversion:
- feeUSDC := new(big.Float).Mul(feeUSD, new(big.Float).SetFloat64(denomTokenPrice)) - feeDenom = new(big.Float).Mul(feeUSDC, new(big.Float).SetInt(denomDecimalsFactor)) + feeDenomTokens := new(big.Float).Quo(feeUSD, new(big.Float).SetFloat64(denomTokenPrice)) + feeDenom = new(big.Float).Mul(feeDenomTokens, new(big.Float).SetInt(denomDecimalsFactor))Consider renaming variables for clarity:
- Rename
feeUSDCtofeeDenomTokensto reflect that it represents the amount indenomTokenunits.
Line range hint
323-346: Handle rounding to avoid underestimating fees when converting frombig.Floattobig.IntIn
getFeeWithMultiplier, convertingfeeDenom(a*big.Float) tofeeScaled(a*big.Int) truncates any fractional part without proper rounding. This may underestimate the fee, potentially leading to insufficient fee calculations.Consider using
feeDenom.Text('f', 0)to round the value appropriately before converting it tobig.Int, or use theRoundmethod if available. Alternatively, you can add a small epsilon value to ensure correct rounding.For example:
-feeScaled, _ = new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil) +feeFloat := new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)) +feeFloatRounded := feeFloat.SetMode(big.AwayFromZero) +feeScaled, _ = feeFloatRounded.Int(nil)This ensures that any fractional amounts are properly considered in the final fee calculation.
services/rfq/relayer/pricer/fee_pricer_test.go (1)
7-13: Organize imports according to Go conventionsThe added imports are necessary for the new functionality. However, consider grouping standard library imports, third-party imports, and local package imports separately for better readability and maintainability.
services/rfq/relayer/quoter/quoter.go (1)
340-340: Offer assistance to implement the TODOThe TODO comment indicates a pending task to incorporate user call data into the quote request and set the
Transactionfield. I can help implement this functionality to ensureTransactionis properly initialized.Would you like assistance in implementing this feature or creating a new GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- services/rfq/relayer/pricer/fee_pricer.go (11 hunks)
- services/rfq/relayer/pricer/fee_pricer_test.go (12 hunks)
- services/rfq/relayer/quoter/quoter.go (5 hunks)
- services/rfq/relayer/service/relayer.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
services/rfq/relayer/pricer/fee_pricer.go (1)
159-196:⚠️ Potential issueEnsure
DestRecipientis notnilto prevent potential nil pointer dereferenceIn
GetDestinationFee, when constructing thecallMsg, theTofield is set to&tx.DestRecipientwithout a nil check ontx.DestRecipient. IfDestRecipientisnil, this could lead to a nil pointer dereference.Verify that
tx.DestRecipientcannot benilat this point. If there is a possibility, add a nil check to handle this scenario gracefully:if tx.DestRecipient == (common.Address{}) { return nil, fmt.Errorf("DestRecipient is nil") }services/rfq/relayer/pricer/fee_pricer_test.go (7)
229-253: Well-implemented test cases for transactions withCallValueandCallParamsThe added test cases in
TestGetTotalFeeeffectively cover scenarios involving transactions withCallValueandCallParams. This enhances test coverage and ensures that theGetTotalFeemethod handles these new parameters correctly.
246-247: Properly mockEstimateGasfor accurate testingMocking the
EstimateGasmethod inclientDestinationprovides a realistic simulation of gas estimation in the tests, ensuring that the fee calculations are based on controlled and predictable inputs.
235-239: Double-check expected fee calculations in testsEnsure that the expected fees in the test cases accurately reflect the new fee calculation logic, especially with the inclusion of
CallValueandCallParams. This verification prevents false positives and ensures the tests are validating the correct behavior.Also applies to: 252-253
213-216: Consistency in mocking client fetcher and clientsThe clients and client fetcher are correctly mocked to simulate different behaviors for
originanddestination. Ensure that this pattern is consistently applied in all tests requiring mocked clients.
133-133: Ensure all calls toGetDestinationFeeinclude the newparamsparameterThe method
GetDestinationFeenow accepts an additionalparamsparameter. Verify that all calls to this method throughout the codebase have been updated to include this parameter to maintain consistency and prevent potential issues.Run the following script to identify any calls to
GetDestinationFeethat may be missing the newparamsparameter:#!/bin/bash # Description: Find all calls to GetDestinationFee without the new params parameter. # Search for method calls to GetDestinationFee with five arguments ast-grep --lang go --pattern 'GetDestinationFee($_, $_, $_, $_, $_)' --files-with-matches | xargs grep -L 'params'Also applies to: 152-152, 177-177, 201-201
222-222: Ensure all calls toGetTotalFeeinclude the newparamsparameterThe method
GetTotalFeehas been updated to include an additionalparamsparameter. Verify that all calls to this method have been updated accordingly to prevent inconsistencies and potential runtime errors.Run the following script to identify any calls to
GetTotalFeethat may be missing the newparamsparameter:#!/bin/bash # Description: Find all calls to GetTotalFee without the new params parameter. # Search for method calls to GetTotalFee with five arguments ast-grep --lang go --pattern 'GetTotalFee($_, $_, $_, $_, $_)' --files-with-matches | xargs grep -L 'params'Also applies to: 233-234, 248-249, 312-312, 320-320, 339-339, 358-358
44-44: Verify allNewFeePricerinstantiations includerelayerAddressThe constructor
NewFeePricernow requires therelayerAddressparameter. Verify that all instances ofNewFeePriceracross the codebase have been updated to include this new parameter to prevent runtime errors due to missing arguments.Run the following script to identify any instantiations of
NewFeePricerthat may be missing therelayerAddressparameter:Also applies to: 88-88, 129-129, 173-173, 266-266, 308-308, 335-335, 354-354
✅ Verification successful
All
NewFeePricerinstantiations includerelayerAddress.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all instantiations of NewFeePricer without the relayerAddress parameter. # Search for function calls to NewFeePricer with four arguments ast-grep --lang go --pattern 'NewFeePricer($_, $_, $_, $_)'Length of output: 395
Script:
#!/bin/bash # Description: Find all instantiations of NewFeePricer with four arguments to ensure relayerAddress is included. # Search for function calls to NewFeePricer with four arguments ast-grep --lang go --pattern 'NewFeePricer($_, $_, $_, $_)'Length of output: 61
services/rfq/relayer/quoter/quoter.go (1)
24-24: Import statement added for fastbridgev2The addition of the import for
fastbridgev2is appropriate for accessing the required interfaces and types.
trajan0x
changed the title
trajan0x
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/omnirpc/proxy/forward.go (1)
60-60: Improved error message with body contextThe enhancement to include a shortened version of the response body in the error message is excellent. It provides more context for debugging while using
ellipsis.Shorten()to keep the message concise.Consider adding a constant for the maximum length (currently 200) to make it easily adjustable:
const maxErrorBodyLength = 200 // Use it in the error message fmt.Errorf("could not standardize response from body %s: %w", ellipsis.Shorten(string(body), maxErrorBodyLength), err)This minor change would make the code more maintainable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- services/omnirpc/proxy/forward.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
services/omnirpc/proxy/forward.go (2)
8-10: LGTM: Import statement improvementsThe changes to the import statements are beneficial:
- Importing
net/httpasgoHTTPhelps avoid potential naming conflicts with the localhttppackage.- Separating the
stringsimport improves code readability.These modifications contribute to better code organization.
Line range hint
89-89: Enhanced batch processing error reportingThe modification to include the index of the failed response in the error message is a valuable improvement. It provides more specific information for debugging batch processing issues, which is crucial for maintaining and troubleshooting the system.
This change is consistent with the improvement made in the
newRawResponsefunction and contributes to a more informative error reporting system throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/rfq/relayer/pricer/fee_pricer.go (1)
57-57: Update function comment to reflect the new parameterrelayerAddress.The
NewFeePricerfunction now includes therelayerAddressparameter, but the function comment does not mention this addition. Updating the comment will improve code readability and maintain consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- services/rfq/relayer/pricer/fee_pricer.go (11 hunks)
🧰 Additional context used
🔇 Additional comments (2)
services/rfq/relayer/pricer/fee_pricer.go (2)
Line range hint
127-189: Proper handling of optional transaction parameters to prevent nil pointer dereferences.The
GetDestinationFeemethod correctly checks fornilvalues oftx,tx.CallParams, andtx.CallValuebefore using them. This ensures that the code handles optional parameters safely, preventing potential runtime panics due to nil pointer dereferences.
373-374:⚠️ Potential issueSpecify rounding mode when converting
feeFloattofeeScaled.In the
getFeeWithMultipliermethod, convertingfeeFloattofeeScaledusingInt(nil)truncates towards zero, which may lead to underestimating the fee. This could result in charging less than the intended amount.To prevent undercharging, specify the rounding behavior by setting the mode to round away from zero:
feeFloat := new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)) + feeFloat.SetMode(big.AwayFromZero) feeScaled, _ = feeFloat.Int(nil)Ensure that the rounding behavior aligns with the fee policy to accurately reflect the intended fees.
Likely invalid or redundant comment.
| // cache so that we don't have to parse the ABI every time | ||
| var recipientABI *abi.ABI | ||
|
|
||
| const methodName = "fastBridgeTransferReceived" | ||
|
|
||
| func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2) (gasEstimate uint64, err error) { | ||
| client, err := f.clientFetcher.GetClient(ctx, big.NewInt(int64(destination))) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("could not get client: %w", err) | ||
| } | ||
|
|
||
| if recipientABI == nil { | ||
| parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeRecipientMetaData.ABI)) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("could not parse ABI: %w", err) | ||
| } | ||
| recipientABI = &parsedABI | ||
| } |
There was a problem hiding this comment.
Prevent potential data race on recipientABI with concurrent access.
The global variable recipientABI is accessed and modified without synchronization in getZapGasEstimate. In a concurrent environment, this can lead to a data race if multiple goroutines initialize recipientABI simultaneously.
Consider initializing recipientABI in a thread-safe manner using sync.Once to ensure that it's only parsed once, even when accessed concurrently. Here's how you can modify the code:
import (
"sync"
// other imports...
)
var (
recipientABI *abi.ABI
recipientABIInit sync.Once
)
func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, tx *fastbridgev2.IFastBridgeV2BridgeTransactionV2) (gasEstimate uint64, err error) {
client, err := f.clientFetcher.GetClient(ctx, big.NewInt(int64(destination)))
if err != nil {
return 0, fmt.Errorf("could not get client: %w", err)
}
var parseErr error
recipientABIInit.Do(func() {
parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeRecipientMetaData.ABI))
if err != nil {
parseErr = fmt.Errorf("could not parse ABI: %w", err)
return
}
recipientABI = &parsedABI
})
if parseErr != nil {
return 0, parseErr
}
encodedData, err := recipientABI.Pack(methodName, tx.DestToken, tx.DestAmount, tx.CallParams)
if err != nil {
return 0, fmt.Errorf("could not encode function call: %w", err)
}
// rest of the function...
}This ensures that recipientABI is safely initialized exactly once.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
services/rfq/relayer/quoter/quoter.go (2)
367-367: Track TODO comment as a GitHub issueThe TODO comment indicates a need to incorporate user call data into the quote request and set the Transaction field. This should be tracked for future implementation.
Would you like me to create a GitHub issue to track this enhancement? The issue would include:
- Title: "Incorporate user call data into quote request"
- Description: Details about setting the Transaction field and its impact on quote generation
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 367-367: services/rfq/relayer/quoter/quoter.go#L367
Added line #L367 was not covered by tests
589-589: Add test coverage for QuoteRequest fieldThe new
QuoteRequestfield inQuoteInputstruct lacks test coverage according to the code coverage report.Would you like me to help generate test cases for this new field? The tests would cover:
- Initialization with nil and non-nil values
- Integration with fee calculation logic
- Error handling scenarios
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 589-589: services/rfq/relayer/quoter/quoter.go#L589
Added line #L589 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
services/rfq/relayer/quoter/quoter.go(4 hunks)services/rfq/relayer/quoter/quoter_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/relayer/quoter/quoter.go
[warning] 367-367: services/rfq/relayer/quoter/quoter.go#L367
Added line #L367 was not covered by tests
[warning] 589-589: services/rfq/relayer/quoter/quoter.go#L589
Added line #L589 was not covered by tests
🔇 Additional comments (2)
services/rfq/relayer/quoter/quoter.go (1)
610-610: LGTM: Proper usage of QuoteRequest in fee calculation
The GetTotalFee call correctly uses input.QuoteRequest for fee calculation.
services/rfq/relayer/quoter/quoter_test.go (1)
576-576: Approve the update to feePricer instantiation
The updated call to pricer.NewFeePricer with the additional common.HexToAddress("0x123") parameter correctly reflects the updated function signature and adapts the test setup accordingly.
github-actions
bot
removed
needs-go-generate-services/explorer
needs-go-generate-agents
labels
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ethergo/backends/anvil/anvil.go (1)
390-398: Consider making retry parameters configurableThe retry mechanism uses hardcoded values which might not be suitable for all environments.
Consider making these parameters configurable:
+// DefaultAccountRetryTimeout is the default timeout for account retrieval +const DefaultAccountRetryTimeout = 10 * time.Second +// BackendConfig contains configuration for the anvil backend +type BackendConfig struct { + AccountRetryTimeout time.Duration +} func (f *Backend) GetTxContext(ctx context.Context, address *common.Address) (res backends.AuthType) { + timeout := DefaultAccountRetryTimeout + if f.config != nil && f.config.AccountRetryTimeout > 0 { + timeout = f.config.AccountRetryTimeout + } // ... rest of the code ... - }, retry.WithMaxTotalTime(10*time.Second)) + }, retry.WithMaxTotalTime(timeout))🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 390-395: ethergo/backends/anvil/anvil.go#L390-L395
Added lines #L390 - L395 were not covered by tests
[warning] 397-398: ethergo/backends/anvil/anvil.go#L397-L398
Added lines #L397 - L398 were not covered by tests🪛 GitHub Check: Lint (ethergo)
[failure] 390-390:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
ethergo/backends/anvil/anvil.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
ethergo/backends/anvil/anvil.go
[warning] 390-395: ethergo/backends/anvil/anvil.go#L390-L395
Added lines #L390 - L395 were not covered by tests
[warning] 397-398: ethergo/backends/anvil/anvil.go#L397-L398
Added lines #L397 - L398 were not covered by tests
🪛 GitHub Check: Lint (ethergo)
ethergo/backends/anvil/anvil.go
[failure] 390-390:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
🔇 Additional comments (1)
ethergo/backends/anvil/anvil.go (1)
33-33: LGTM: Import addition is appropriate
The addition of the retry package import is necessary for implementing the new retry mechanism.
ethergo/backends/anvil/anvil.go
Outdated
| err := retry.WithBackoff(ctx, func(ctx context.Context) error { | ||
| acct = f.GetAccount(*address) | ||
| if acct == nil { | ||
| return fmt.Errorf("could not get account %s", address.String()) | ||
| } | ||
| return nil | ||
| }, retry.WithMaxTotalTime(10*time.Second)) | ||
| if err != nil { | ||
| f.T().Errorf("could not get account %s: %v", address.String(), err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve retry mechanism implementation
While adding retry logic for account retrieval is a good practice, there are several areas for improvement:
- The retry function doesn't use its context parameter
- The timeout duration is hardcoded
- The error handling could be more informative
- The changes lack test coverage
Consider applying these improvements:
- err := retry.WithBackoff(ctx, func(ctx context.Context) error {
+ err := retry.WithBackoff(ctx, func(retryCtx context.Context) error {
+ select {
+ case <-retryCtx.Done():
+ return retryCtx.Err()
+ default:
acct = f.GetAccount(*address)
if acct == nil {
- return fmt.Errorf("could not get account %s", address.String())
+ return fmt.Errorf("account %s not found in backend store", address.String())
}
return nil
+ }
- }, retry.WithMaxTotalTime(10*time.Second))
+ }, retry.WithMaxTotalTime(10*time.Second), retry.WithDescription("get account"))
if err != nil {
- f.T().Errorf("could not get account %s: %v", address.String(), err)
+ f.T().Errorf("failed to get account %s after retries: %v", address.String(), err)
return res
}Also, consider adding the following test:
func TestGetTxContextWithRetry(t *testing.T) {
ctx := context.Background()
backend := setupTestBackend(t)
// Test retry mechanism
address := common.HexToAddress("0x123")
result := backend.GetTxContext(ctx, &address)
// Verify result
assert.Equal(t, backends.AuthType{}, result, "Should return empty AuthType for non-existent account")
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 390-395: ethergo/backends/anvil/anvil.go#L390-L395
Added lines #L390 - L395 were not covered by tests
[warning] 397-398: ethergo/backends/anvil/anvil.go#L397-L398
Added lines #L397 - L398 were not covered by tests
🪛 GitHub Check: Lint (ethergo)
[failure] 390-390:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
services/rfq/relayer/pricer/fee_pricer.go (2)
53-54: Add documentation for the relayerAddress fieldThe new
relayerAddressfield should be documented to explain its purpose and importance in fee calculations.type feePricer struct { + // relayerAddress is the address of the relayer used for gas estimation and fee calculations relayerAddress common.Address }Also applies to: 58-58, 74-74
179-214: Add validation for negative valuesThe method should validate that
quoteRequest.Transaction.ZapNativeis not negative before processing.+ if quoteRequest.Transaction.ZapNative != nil && quoteRequest.Transaction.ZapNative.Sign() < 0 { + return nil, fmt.Errorf("negative ZapNative value not allowed") + } if quoteRequest.Transaction.ZapNative != nil && quoteRequest.Transaction.ZapNative.Sign() > 0 {services/rfq/relayer/pricer/fee_pricer_test.go (2)
21-21: Consider making relayerAddress configurableThe hardcoded address reduces test flexibility. Consider moving it to a test configuration or using a helper function to generate test addresses.
-var relayerAddress = common.HexToAddress("0x1234567890123456789012345678901234567890") +var defaultRelayerAddress = common.HexToAddress("0x1234567890123456789012345678901234567890") + +func getTestRelayerAddress() common.Address { + if addr := os.Getenv("TEST_RELAYER_ADDRESS"); addr != "" { + return common.HexToAddress(addr) + } + return defaultRelayerAddress +}
254-254: Improve gas estimation test coverageThe gas estimation is currently mocked with a single hardcoded value. Consider testing different scenarios including errors and varying gas costs.
+func (s *PricerSuite) TestGetTotalFeeGasEstimation() { + // ... setup code ... + + testCases := []struct { + name string + gasEstimate uint64 + gasError error + expectError bool + }{ + { + name: "normal gas estimation", + gasEstimate: 1_000_000, + }, + { + name: "high gas estimation", + gasEstimate: 5_000_000, + }, + { + name: "gas estimation error", + gasError: errors.New("estimation failed"), + expectError: true, + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + clientDestination.On("EstimateGas", mock.Anything, mock.Anything). + Return(tc.gasEstimate, tc.gasError).Once() + // ... test implementation + }) + } +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
ethergo/backends/anvil/anvil.go(2 hunks)services/rfq/e2e/rfq_test.go(1 hunks)services/rfq/relayer/pricer/fee_pricer.go(11 hunks)services/rfq/relayer/pricer/fee_pricer_test.go(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/e2e/rfq_test.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
ethergo/backends/anvil/anvil.go
[warning] 390-395: ethergo/backends/anvil/anvil.go#L390-L395
Added lines #L390 - L395 were not covered by tests
[warning] 397-398: ethergo/backends/anvil/anvil.go#L397-L398
Added lines #L397 - L398 were not covered by tests
🔇 Additional comments (3)
services/rfq/relayer/pricer/fee_pricer.go (3)
310-328:
Use integer arithmetic for fee calculations
The current implementation uses floating-point arithmetic which can lead to precision loss. Consider using integer arithmetic throughout.
This issue was previously identified in past reviews. The suggestion to use big.Int instead of big.Float for calculations remains valid.
401-402:
Implement conservative rounding for fee calculations
The current implementation rounds towards zero when applying fee multipliers, which could lead to fee underestimation.
This issue was previously identified in past reviews. The suggestion to use SetMode(big.AwayFromZero) remains valid.
245-254: 🛠️ Refactor suggestion
Improve error handling for gas estimation
The current implementation might not handle all possible revert scenarios gracefully. Consider adding fallback logic for cases where the transaction simulation fails.
+ // Add fallback gas estimation logic
+ if err != nil {
+ if strings.Contains(err.Error(), "execution reverted") {
+ // Use configured fallback gas limit
+ gasEstimate, err = f.config.GetDestGasEstimate(int(destination))
+ if err != nil {
+ return 0, fmt.Errorf("fallback gas estimation failed: %w", err)
+ }
+ return gasEstimate, nil
+ }
return 0, fmt.Errorf("could not estimate gas: %w", err)
}| err := retry.WithBackoff(ctx, func(_ context.Context) error { | ||
| acct = f.GetAccount(*address) | ||
| if acct == nil { | ||
| return fmt.Errorf("could not get account %s", address.String()) | ||
| } | ||
| return nil | ||
| }, retry.WithMaxTotalTime(10*time.Second)) | ||
| if err != nil { | ||
| f.T().Errorf("could not get account %s: %v", address.String(), err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve retry mechanism implementation
The retry mechanism needs several improvements:
- The context parameter is unused in the retry function
- The error handling could be more descriptive
- The changes lack test coverage
Apply this diff to improve the implementation:
- err := retry.WithBackoff(ctx, func(_ context.Context) error {
+ err := retry.WithBackoff(ctx, func(retryCtx context.Context) error {
+ select {
+ case <-retryCtx.Done():
+ return retryCtx.Err()
+ default:
acct = f.GetAccount(*address)
if acct == nil {
- return fmt.Errorf("could not get account %s", address.String())
+ return fmt.Errorf("account %s not found in backend store", address.String())
}
return nil
+ }
- }, retry.WithMaxTotalTime(10*time.Second))
+ }, retry.WithMaxTotalTime(10*time.Second), retry.WithDescription("get account"))
if err != nil {
- f.T().Errorf("could not get account %s: %v", address.String(), err)
+ f.T().Errorf("failed to get account %s after retries: %v", address.String(), err)
return res
}Additionally, add the following test to cover the retry mechanism:
func TestGetTxContextWithRetry(t *testing.T) {
ctx := context.Background()
backend := setupTestBackend(t)
// Test retry mechanism
address := common.HexToAddress("0x123")
result := backend.GetTxContext(ctx, &address)
// Verify result
assert.Equal(t, backends.AuthType{}, result, "Should return empty AuthType for non-existent account")
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 390-395: ethergo/backends/anvil/anvil.go#L390-L395
Added lines #L390 - L395 were not covered by tests
[warning] 397-398: ethergo/backends/anvil/anvil.go#L397-L398
Added lines #L397 - L398 were not covered by tests
| if quoteRequest == nil || quoteRequest.Transaction.ZapData == nil || len(quoteRequest.Transaction.ZapData) == 0 { | ||
| gasEstimate, err := f.config.GetDestGasEstimate(int(destination)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get dest gas estimate: %w", err) | ||
| } | ||
| fee, err = f.getFee(ctx, destination, destination, gasEstimate, denomToken, isQuote) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Improve nil checks for Transaction field
The current check doesn't validate if quoteRequest.Transaction is nil before accessing its fields. This could lead to a panic.
-if quoteRequest == nil || quoteRequest.Transaction.ZapData == nil || len(quoteRequest.Transaction.ZapData) == 0 {
+if quoteRequest == nil || quoteRequest.Transaction == nil || quoteRequest.Transaction.ZapData == nil || len(quoteRequest.Transaction.ZapData) == 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.
| if quoteRequest == nil || quoteRequest.Transaction.ZapData == nil || len(quoteRequest.Transaction.ZapData) == 0 { | |
| gasEstimate, err := f.config.GetDestGasEstimate(int(destination)) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get dest gas estimate: %w", err) | |
| } | |
| fee, err = f.getFee(ctx, destination, destination, gasEstimate, denomToken, isQuote) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if quoteRequest == nil || quoteRequest.Transaction == nil || quoteRequest.Transaction.ZapData == nil || len(quoteRequest.Transaction.ZapData) == 0 { | |
| gasEstimate, err := f.config.GetDestGasEstimate(int(destination)) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get dest gas estimate: %w", err) | |
| } | |
| fee, err = f.getFee(ctx, destination, destination, gasEstimate, denomToken, isQuote) | |
| if err != nil { | |
| return nil, err | |
| } |
| // cache so that we don't have to parse the ABI every time. | ||
| var fastBridgeV2ABI *abi.ABI | ||
|
|
||
| const methodName = "relay0" | ||
|
|
||
| func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (gasEstimate uint64, err error) { | ||
| client, err := f.clientFetcher.GetClient(ctx, big.NewInt(int64(destination))) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("could not get client: %w", err) | ||
| } | ||
|
|
||
| if fastBridgeV2ABI == nil { | ||
| parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI)) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("could not parse ABI: %w", err) | ||
| } | ||
| fastBridgeV2ABI = &parsedABI | ||
| } |
There was a problem hiding this comment.
Add thread safety to ABI cache
The global fastBridgeV2ABI variable is accessed without synchronization, which could lead to race conditions in concurrent scenarios.
+var (
+ fastBridgeV2ABI *abi.ABI
+ fastBridgeV2ABIOnce sync.Once
+)
func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (gasEstimate uint64, err error) {
// ...
- if fastBridgeV2ABI == nil {
- parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI))
- if err != nil {
- return 0, fmt.Errorf("could not parse ABI: %w", err)
- }
- fastBridgeV2ABI = &parsedABI
- }
+ var parseErr error
+ fastBridgeV2ABIOnce.Do(func() {
+ parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI))
+ if err != nil {
+ parseErr = fmt.Errorf("could not parse ABI: %w", err)
+ return
+ }
+ fastBridgeV2ABI = &parsedABI
+ })
+ if parseErr != nil {
+ return 0, parseErr
+ }📝 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.
| // cache so that we don't have to parse the ABI every time. | |
| var fastBridgeV2ABI *abi.ABI | |
| const methodName = "relay0" | |
| func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (gasEstimate uint64, err error) { | |
| client, err := f.clientFetcher.GetClient(ctx, big.NewInt(int64(destination))) | |
| if err != nil { | |
| return 0, fmt.Errorf("could not get client: %w", err) | |
| } | |
| if fastBridgeV2ABI == nil { | |
| parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI)) | |
| if err != nil { | |
| return 0, fmt.Errorf("could not parse ABI: %w", err) | |
| } | |
| fastBridgeV2ABI = &parsedABI | |
| } | |
| // cache so that we don't have to parse the ABI every time. | |
| var ( | |
| fastBridgeV2ABI *abi.ABI | |
| fastBridgeV2ABIOnce sync.Once | |
| ) | |
| const methodName = "relay0" | |
| func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (gasEstimate uint64, err error) { | |
| client, err := f.clientFetcher.GetClient(ctx, big.NewInt(int64(destination))) | |
| if err != nil { | |
| return 0, fmt.Errorf("could not get client: %w", err) | |
| } | |
| var parseErr error | |
| fastBridgeV2ABIOnce.Do(func() { | |
| parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI)) | |
| if err != nil { | |
| parseErr = fmt.Errorf("could not parse ABI: %w", err) | |
| return | |
| } | |
| fastBridgeV2ABI = &parsedABI | |
| }) | |
| if parseErr != nil { | |
| return 0, parseErr | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
services/rfq/relayer/pricer/fee_pricer.go (1)
144-144: Consider using consistent condition checks for zap functionality.The condition
quoteRequest.Transaction.ZapData == nil || len(quoteRequest.Transaction.ZapData) == 0should be consistent with the check inaddZapFees. Currently,addZapFeesonly checks length while this checks both nil and length.-if quoteRequest == nil || quoteRequest.Transaction.ZapData == nil || len(quoteRequest.Transaction.ZapData) == 0 { +if quoteRequest == nil || len(quoteRequest.Transaction.ZapData) == 0 {services/rfq/relayer/pricer/fee_pricer_test.go (3)
21-21: Consider using test constants for addressesThe hardcoded
relayerAddressshould be defined as a test constant with a meaningful name for better maintainability and readability.-var relayerAddress = common.HexToAddress("0x1234567890123456789012345678901234567890") +const ( + testRelayerAddress = "0x1234567890123456789012345678901234567890" +) +var relayerAddress = common.HexToAddress(testRelayerAddress)
230-262: Good addition of v2 call value test casesThe test cases effectively verify the fee calculation with v2 call value and call params. However, consider adding negative test cases:
- Invalid call value
- Zero call value
- Maximum call value boundary
+ // Test with invalid call value + quoteRequest = &reldb.QuoteRequest{ + RawRequest: []byte{}, + Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{ + ZapNative: big.NewInt(-1), + }, + } + fee, err = feePricer.GetTotalFee(s.GetTestContext(), s.origin, s.destination, "USDC", true, quoteRequest) + s.Error(err) + + // Test with maximum call value + maxValue := new(big.Int).Lsh(big.NewInt(1), 256) + maxValue.Sub(maxValue, big.NewInt(1)) + quoteRequest = &reldb.QuoteRequest{ + RawRequest: []byte{}, + Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{ + ZapNative: maxValue, + }, + } + fee, err = feePricer.GetTotalFee(s.GetTestContext(), s.origin, s.destination, "USDC", true, quoteRequest) + s.Error(err)
254-254: Enhance gas estimation test coverageThe current test only covers a successful gas estimation case. Consider adding test cases for:
- Gas estimation failure
- Different gas values
- Rate limiting scenarios
+ // Test gas estimation failure + clientDestination.On(testsuite.GetFunctionName(clientDestination.EstimateGas), mock.Anything, mock.Anything).Once().Return(uint64(0), fmt.Errorf("estimation failed")) + fee, err = feePricer.GetTotalFee(s.GetTestContext(), s.origin, s.destination, "USDC", true, quoteRequest) + s.Error(err) + + // Test with high gas estimation + clientDestination.On(testsuite.GetFunctionName(clientDestination.EstimateGas), mock.Anything, mock.Anything).Once().Return(uint64(10_000_000), nil) + fee, err = feePricer.GetTotalFee(s.GetTestContext(), s.origin, s.destination, "USDC", true, quoteRequest) + s.NoError(err) + s.Greater(fee.Int64(), expectedFee.Int64())services/rfq/e2e/rfq_test.go (2)
405-405: Document the reason for suppressing security checksThe
//nolint:gosecdirective suppresses all security checks for this test function without explanation. Please add a comment explaining why it's safe to ignore these security checks.
Line range hint
779-799: Add timeout protection to the time manipulation goroutineThe infinite loop for time manipulation could potentially cause the test to hang if the test context isn't cancelled properly. Consider adding a timeout mechanism or using
time.NewTickerfor better control.go func() { + ticker := time.NewTicker(4 * time.Second) + defer ticker.Stop() for { select { case <-i.GetTestContext().Done(): return - case <-time.After(time.Second * 4): + case <-ticker.C: // increase time by 30 mintutes every second, should be enough to get us a fastish e2e test // we don't need to worry about deadline since we're only doing this on origin err = anvilClient.IncreaseTime(i.GetTestContext(), 60*30)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
ethergo/backends/anvil/anvil.go(2 hunks)services/rfq/e2e/rfq_test.go(1 hunks)services/rfq/relayer/pricer/fee_pricer.go(11 hunks)services/rfq/relayer/pricer/fee_pricer_test.go(12 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
ethergo/backends/anvil/anvil.go
[warning] 390-395: ethergo/backends/anvil/anvil.go#L390-L395
Added lines #L390 - L395 were not covered by tests
[warning] 397-398: ethergo/backends/anvil/anvil.go#L397-L398
Added lines #L397 - L398 were not covered by tests
🔇 Additional comments (5)
ethergo/backends/anvil/anvil.go (1)
390-398: 🛠️ Refactor suggestion
Improve retry mechanism implementation
The retry mechanism for account retrieval needs several improvements:
- The context parameter is unused in the retry function
- The timeout duration is hardcoded
- The error handling could be more descriptive
- The changes lack test coverage
Apply this diff to improve the implementation:
- err := retry.WithBackoff(ctx, func(_ context.Context) error {
+ err := retry.WithBackoff(ctx, func(retryCtx context.Context) error {
+ select {
+ case <-retryCtx.Done():
+ return retryCtx.Err()
+ default:
acct = f.GetAccount(*address)
if acct == nil {
- return fmt.Errorf("could not get account %s", address.String())
+ return fmt.Errorf("account %s not found in backend store", address.String())
}
return nil
+ }
- }, retry.WithMaxTotalTime(10*time.Second))
+ }, retry.WithMaxTotalTime(10*time.Second), retry.WithDescription("get account"))
if err != nil {
- f.T().Errorf("could not get account %s: %v", address.String(), err)
+ f.T().Errorf("failed to get account %s after retries: %v", address.String(), err)
return res
}Let's verify the test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 390-395: ethergo/backends/anvil/anvil.go#L390-L395
Added lines #L390 - L395 were not covered by tests
[warning] 397-398: ethergo/backends/anvil/anvil.go#L397-L398
Added lines #L397 - L398 were not covered by tests
services/rfq/relayer/pricer/fee_pricer.go (3)
Line range hint 31-54: LGTM: Interface and struct changes are well-designed.
The addition of the quoteRequest parameter to interface methods and the relayerAddress field to the struct are well-thought-out changes that support the new zap fee functionality.
Line range hint 58-74: LGTM: Constructor properly initializes new field.
The NewFeePricer constructor correctly initializes the new relayerAddress field while maintaining the existing initialization logic.
Line range hint 259-291: LGTM: GetTotalFee properly handles quote request propagation.
The method correctly propagates the quoteRequest parameter to GetDestinationFee while maintaining existing functionality.
services/rfq/e2e/rfq_test.go (1)
406-407: Remove debug print statement
This debug print statement should be removed to keep the test output clean.
| // addZapFees incorporates the cost of the call and the call value into the fee. | ||
| // Note that to be conservative, we always use the QuoteFixedFeeMultiplier over the RelayFixedFeeMultiplier. | ||
| // | ||
| //nolint:gosec | ||
| func (f *feePricer) addZapFees(ctx context.Context, destination uint32, denomToken string, quoteRequest *reldb.QuoteRequest, fee *big.Int) (*big.Int, error) { | ||
| span := trace.SpanFromContext(ctx) | ||
|
|
||
| if len(quoteRequest.Transaction.ZapData) > 0 { | ||
| gasEstimate, err := f.getZapGasEstimate(ctx, destination, quoteRequest) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| callFee, err := f.getFee(ctx, destination, destination, int(gasEstimate), denomToken, true) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fee = new(big.Int).Add(fee, callFee) | ||
| span.SetAttributes(attribute.String("call_fee", callFee.String())) | ||
| } | ||
|
|
||
| if quoteRequest.Transaction.ZapNative != nil && quoteRequest.Transaction.ZapNative.Sign() > 0 { | ||
| callValueFloat := new(big.Float).SetInt(quoteRequest.Transaction.ZapNative) | ||
| valueDenom, err := f.getDenomFee(ctx, destination, destination, denomToken, callValueFloat) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| valueScaled, err := f.getFeeWithMultiplier(ctx, destination, true, valueDenom) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fee = new(big.Int).Add(fee, valueScaled) | ||
| span.SetAttributes(attribute.String("value_scaled", valueScaled.String())) | ||
| } | ||
|
|
||
| return fee, nil | ||
| } |
There was a problem hiding this comment.
Address potential griefing vector with fee multipliers.
As noted in the previous review, using relay_fixed_fee_multiplier (which is < 1.0) could create a griefing vector. For example, a user could request a CallValue worth $1 but only supply $0.8 in fees.
Consider either:
- Always using
quote_fixed_fee_multiplierfor determining the requestedCallValueworth - Introducing a separate multiplier parameter specifically for this purpose
…T-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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/rfq/relayer/quoter/quoter_test.go (1)
450-534: Consider adding more test cases for comprehensive coverage.The test function covers basic functionality but could benefit from additional test cases:
- Error scenarios (invalid addresses, chain IDs)
- Boundary conditions (zero amounts, maximum amounts)
- Invalid message formats
- Network-related failures
📜 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(7 hunks)services/rfq/relayer/quoter/quoter_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- services/rfq/api/model/request.go
🔇 Additional comments (6)
services/rfq/relayer/quoter/export_test.go (1)
25-27: LGTM! Clean test export implementation.
The new GenerateActiveRFQ method follows the established pattern of test exports in this file, providing a clean wrapper around the internal generateActiveRFQ method for testing purposes.
services/rfq/relayer/quoter/quoter_test.go (1)
665-665: LGTM!
The addition of the signer address parameter to NewFeePricer is properly implemented in the test helper function.
services/rfq/relayer/quoter/quoter.go (4)
617-617: Past review comment is still applicable.
The concern about using a pointer to interface type remains valid.
383-394: LGTM! Proper handling of ZapNative and ZapData.
The implementation includes proper error handling and type conversion checks.
401-414: LGTM! Comprehensive fee adjustment implementation.
The code properly handles:
- Input validation
- Edge cases (negative amounts)
- Type conversions with error checking
638-638: LGTM! Correct parameter passing to GetTotalFee.
The implementation correctly passes the QuoteRequest field to the fee calculation function.
| fastBridgeV2ABI = &parsedABI | ||
| } | ||
|
|
||
| encodedData, err := fastBridgeV2ABI.Pack(methodName, quoteRequest.RawRequest, f.relayerAddress) |
There was a problem hiding this comment.
Seeing an issue here. I don't think quoteRequest.RawRequest is getting set at all when the active quote is generated:
sanguine/services/rfq/relayer/quoter/quoter.go
Lines 374 to 394 in f93b3b3
Therefore we don't correctly estimate the gas here. Constructing the full request is somewhat tricky here (e.g. we need to set the destination amount in advance), happy to discuss the details on a call.
There was a problem hiding this comment.
So we actually do supply the RawRequest when doing the EstimateGas() call here:
The QuoteInput.QuoteRequest is really only used to communicate the extra data around the quoter functions. I considered introducing a new struct for this, maybe simpler to just include these two fields individually within the QuoteInput struct?
There was a problem hiding this comment.
We supply it, but we never actually set the value for it, so it's always an empty bytes array. Let's figure out the best way to generate it, as the current contracts don't expose the encoding function.
* 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
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
services/rfq/relayer/chain/encoding.go (2)
39-47: Handle potential nil or negative values inpadUint256The
padUint256function does not account for cases where the input*big.Intisnilor negative. Consider adding checks to handlenilinputs and ensure that negative values are either properly handled or explicitly disallowed to prevent unexpected behavior during encoding.
17-35: Enhance constant documentation for better maintainabilityWhile the constants for field sizes and offsets are defined, adding more descriptive comments for each can improve readability. This will aid future developers in understanding the purpose of each constant and how they contribute to the encoding/decoding process.
services/rfq/contracts/bridgetransactionv2/helper.go (1)
17-18: Correct typo and clarify method comment inAddressThere's a typo in the comment for the
Addressmethod: "ocntract" should be "contract". Also, for clarity, consider rephrasing the comment to "Address returns the contract address."Apply this diff to correct the comment:
-// Address gets the ocntract address. +// Address returns the contract address.services/rfq/e2e/rfq_test.go (1)
781-883: Consider adding fixed test cases alongside random onesWhile random test data is good for catching edge cases, it can make tests flaky and harder to debug. Consider adding a set of fixed test cases alongside the random ones to ensure consistent coverage of known scenarios.
services/rfq/relayer/quoter/quoter.go (1)
396-409: Consider extracting amount adjustment logic to a helper functionThe code performs multiple string to big.Int conversions and amount adjustments. This logic could be encapsulated in a helper function for better reusability and maintainability.
Example helper function:
func adjustDestAmount(destAmount, fixedFee string) (string, error) { destAmountBigInt, ok := new(big.Int).SetString(destAmount, 10) if !ok { return "", fmt.Errorf("invalid dest amount: %s", destAmount) } fixedFeeBigInt, ok := new(big.Int).SetString(fixedFee, 10) if !ok { return "", fmt.Errorf("invalid fixed fee: %s", fixedFee) } destAmountAdj := new(big.Int).Sub(destAmountBigInt, fixedFeeBigInt) if destAmountAdj.Sign() < 0 { destAmountAdj = big.NewInt(0) } return destAmountAdj.String(), nil }services/rfq/testutil/contracttypeimpl_string.go (1)
Line range hint
1-1: Reminder: This is generated codeThis file is generated by the
stringertool. Any changes should be made to the source enum definitions rather than this file directly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
packages/contracts-rfq/package.json(1 hunks)services/rfq/api/model/request.go(1 hunks)services/rfq/contracts/bridgetransactionv2/bridgetransactionv2.metadata.go(1 hunks)services/rfq/contracts/bridgetransactionv2/generate.go(1 hunks)services/rfq/contracts/bridgetransactionv2/helper.go(1 hunks)services/rfq/e2e/rfq_test.go(3 hunks)services/rfq/relayer/chain/encoding.go(1 hunks)services/rfq/relayer/pricer/fee_pricer.go(11 hunks)services/rfq/relayer/quoter/quoter.go(8 hunks)services/rfq/testutil/contracttype.go(3 hunks)services/rfq/testutil/contracttypeimpl_string.go(1 hunks)services/rfq/testutil/deployers.go(3 hunks)services/rfq/testutil/typecast.go(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- services/rfq/contracts/bridgetransactionv2/generate.go
- services/rfq/contracts/bridgetransactionv2/bridgetransactionv2.metadata.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/api/model/request.go
🔇 Additional comments (14)
packages/contracts-rfq/package.json (1)
29-29: Include test harness contracts in build process
Updating the build:go script to include test/harnesses/*.sol ensures that all necessary Solidity test harnesses are processed during the build. This inclusion is appropriate for comprehensive testing.
services/rfq/testutil/typecast.go (1)
48-53: Integration of GetBridgeTransactionV2 method looks good
The addition of the GetBridgeTransactionV2 method to DeployManager follows the established pattern for accessing deployed contracts. This enhances the test utilities by providing access to the new bridge transaction contract.
services/rfq/testutil/contracttype.go (1)
62-63: LGTM! Contract type and mapping added correctly.
The new BridgeTransactionV2Type is properly integrated into the contract type system with correct sequencing and mapping to its contract info.
Also applies to: 106-107
services/rfq/testutil/deployers.go (2)
32-32: LGTM! Deployer properly integrated into manager.
The BridgeTransactionV2Deployer is correctly added to the list of deployers in NewDeployManager.
165-185: LGTM! Deployer implementation follows established patterns.
The BridgeTransactionV2Deployer implementation follows the same pattern as other deployers in the file, with proper error handling and contract deployment logic.
services/rfq/relayer/pricer/fee_pricer.go (4)
217-234:
Add thread safety to ABI cache.
The global fastBridgeV2ABI variable is accessed without synchronization, which could lead to race conditions in concurrent scenarios.
Apply this diff to make the ABI cache thread-safe:
var (
fastBridgeV2ABI *abi.ABI
+ fastBridgeV2ABIOnce sync.Once
)
func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (gasEstimate uint64, err error) {
// ...
- if fastBridgeV2ABI == nil {
- parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI))
- if err != nil {
- return 0, fmt.Errorf("could not parse ABI: %w", err)
- }
- fastBridgeV2ABI = &parsedABI
- }
+ var parseErr error
+ fastBridgeV2ABIOnce.Do(func() {
+ parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI))
+ if err != nil {
+ parseErr = fmt.Errorf("could not parse ABI: %w", err)
+ return
+ }
+ fastBridgeV2ABI = &parsedABI
+ })
+ if parseErr != nil {
+ return 0, parseErr
+ }316-317:
Fix precision loss in fee calculations.
The current implementation has two issues:
- Converting
gasEstimatetofloat64may lead to precision loss - Rounding towards zero when applying fee multiplier could lead to undercharging
Apply these changes:
-feeWei := new(big.Float).Mul(new(big.Float).SetInt(gasPrice), new(big.Float).SetFloat64(float64(gasEstimate)))
+feeWei := new(big.Int).Mul(gasPrice, big.NewInt(int64(gasEstimate)))
-feeScaled, _ = new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil)
+feeFloat := new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier))
+feeFloat.SetMode(big.AwayFromZero)
+feeScaled, _ = feeFloat.Int(nil)Also applies to: 407-407
251-257:
Improve gas estimation for ERC20 tokens.
The current implementation estimates gas for just the recipient call, which will fail for ERC20 tokens as they are transferred separately by the FastBridge contract.
Consider estimating gas for the whole relay() call instead of just the recipient call to accurately account for token transfers.
180-184:
Address potential griefing vector with fee multipliers.
Using relay_fixed_fee_multiplier (which is < 1.0) could create a griefing vector where users request a higher CallValue but only supply a fraction in fees.
Consider either:
- Always using
quote_fixed_fee_multiplierfor determining the requestedCallValueworth - Introducing a separate multiplier parameter for this purpose
services/rfq/e2e/rfq_test.go (2)
410-411: Remove unnecessary debug print statement
The debug print statement should be removed to keep the test output clean.
780-782: Evaluate the necessity of the //nolint:gosec directive
The //nolint:gosec comment suppresses security linter warnings without justification. Unless there is a specific reason to disable these security checks, consider removing this directive.
services/rfq/relayer/quoter/quoter.go (1)
237-237: Fix pointer handling in GetTotalFee call
The code passes "e to GetTotalFee, which creates a pointer to a pointer. This is likely not the intended behavior.
services/rfq/testutil/contracttypeimpl_string.go (2)
22-22: Verify string representation consistency
The name constant and index array have been updated to include BridgeTransactionV2. Please ensure:
- The string lengths in
_contractTypeImpl_namematch the index differences in_contractTypeImpl_index - The
stringercommand was run after updating the enum constants
Also applies to: 24-24
✅ Verification successful
Let me gather more information to verify the string representation consistency.
String representation is correctly generated and consistent
The verification confirms that:
- The string lengths in
_contractTypeImpl_namematch the index differences in_contractTypeImpl_index. For example, "BridgeTransactionV2" spans from index 46 to 65 (19 characters). - The
stringercommand is properly configured withgo:generate go run golang.org/x/tools/cmd/stringer -type=contractTypeImpl -linecommentand has been run, as evidenced by the generated file's content and header comment. - All enum constants are properly represented in both the index array and name string.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the stringer command invocation to verify it's being run correctly
# Look for go:generate directives or build scripts that run stringer
rg -l 'go:generate.*stringer.*contractTypeImpl'
Length of output: 395
Script:
#!/bin/bash
# Check the content of the contracttype.go and its generated string file
cat services/rfq/testutil/contracttype.go
cat services/rfq/testutil/contracttypeimpl_string.go
Length of output: 5629
15-19: Verify enum constant declarations match these indices
The indices have been updated to accommodate the new BridgeTransactionV2Type at index 5. Please ensure these indices match the actual enum constant declarations in the source file.
✅ Verification successful
Let me gather more specific information about the RFQ service's contract types since that's the one we're verifying.
Indices in the generated stringer file match the enum declarations
The indices in contracttypeimpl_string.go correctly match the enum declarations in services/rfq/testutil/contracttype.go. The sequence is:
BridgeTransactionV2Typeat index 5WETH9Typeat index 6USDTTypeat index 7USDCTypeat index 8DAITypeat index 9
This matches the iota-based declarations and their order in the source file, confirming the generated code is consistent with the type definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the enum constant declarations match these indices
# Look for the contractTypeImpl type declaration and its constants
# Search for the enum type declaration and its constants
ast-grep --pattern 'type contractTypeImpl int' -A 15
Length of output: 9181
Script:
#!/bin/bash
# Let's get more specific information about the RFQ service contract types
rg -A 10 "USDTType|USDCType|DAIType" services/rfq/testutil/
Length of output: 9929
| if len(data) < offsetZapData { | ||
| return fastbridgev2.IFastBridgeV2BridgeTransactionV2{}, fmt.Errorf("data too short: got %d bytes, need at least %d", len(data), offsetZapData) | ||
| } | ||
|
|
||
| tx := fastbridgev2.IFastBridgeV2BridgeTransactionV2{ | ||
| OriginChainId: binary.BigEndian.Uint32(data[offsetOriginChainID:offsetDestChainID]), | ||
| DestChainId: binary.BigEndian.Uint32(data[offsetDestChainID:offsetOriginSender]), | ||
| OriginSender: common.BytesToAddress(data[offsetOriginSender:offsetDestRecipient]), | ||
| DestRecipient: common.BytesToAddress(data[offsetDestRecipient:offsetOriginToken]), | ||
| OriginToken: common.BytesToAddress(data[offsetOriginToken:offsetDestToken]), | ||
| DestToken: common.BytesToAddress(data[offsetDestToken:offsetOriginAmount]), | ||
| OriginAmount: new(big.Int).SetBytes(data[offsetOriginAmount:offsetDestAmount]), | ||
| DestAmount: new(big.Int).SetBytes(data[offsetDestAmount:offsetOriginFeeAmount]), | ||
| OriginFeeAmount: new(big.Int).SetBytes(data[offsetOriginFeeAmount:offsetDeadline]), | ||
| Deadline: new(big.Int).SetBytes(data[offsetDeadline:offsetNonce]), | ||
| Nonce: new(big.Int).SetBytes(data[offsetNonce:offsetExclusivityRelayer]), | ||
| ExclusivityRelayer: common.BytesToAddress(data[offsetExclusivityRelayer:offsetExclusivityEndTime]), | ||
| ExclusivityEndTime: new(big.Int).SetBytes(data[offsetExclusivityEndTime:offsetZapNative]), | ||
| ZapNative: new(big.Int).SetBytes(data[offsetZapNative:offsetZapData]), | ||
| ZapData: data[offsetZapData:], | ||
| } |
There was a problem hiding this comment.
Add comprehensive validation in DecodeBridgeTx to prevent panics
While DecodeBridgeTx checks if len(data) < offsetZapData, it does not verify that the subsequent slice operations are within the bounds of the data slice. If data is shorter than expected, this could lead to runtime panics due to out-of-bounds access. Consider adding bounds checks before each slice operation or ensuring that the length of data is sufficient for all fields being decoded.
| tx := fastbridgev2.IFastBridgeV2BridgeTransactionV2{ | ||
| OriginChainId: binary.BigEndian.Uint32(data[offsetOriginChainID:offsetDestChainID]), | ||
| DestChainId: binary.BigEndian.Uint32(data[offsetDestChainID:offsetOriginSender]), | ||
| OriginSender: common.BytesToAddress(data[offsetOriginSender:offsetDestRecipient]), | ||
| DestRecipient: common.BytesToAddress(data[offsetDestRecipient:offsetOriginToken]), | ||
| OriginToken: common.BytesToAddress(data[offsetOriginToken:offsetDestToken]), | ||
| DestToken: common.BytesToAddress(data[offsetDestToken:offsetOriginAmount]), | ||
| OriginAmount: new(big.Int).SetBytes(data[offsetOriginAmount:offsetDestAmount]), | ||
| DestAmount: new(big.Int).SetBytes(data[offsetDestAmount:offsetOriginFeeAmount]), | ||
| OriginFeeAmount: new(big.Int).SetBytes(data[offsetOriginFeeAmount:offsetDeadline]), | ||
| Deadline: new(big.Int).SetBytes(data[offsetDeadline:offsetNonce]), | ||
| Nonce: new(big.Int).SetBytes(data[offsetNonce:offsetExclusivityRelayer]), | ||
| ExclusivityRelayer: common.BytesToAddress(data[offsetExclusivityRelayer:offsetExclusivityEndTime]), | ||
| ExclusivityEndTime: new(big.Int).SetBytes(data[offsetExclusivityEndTime:offsetZapNative]), | ||
| ZapNative: new(big.Int).SetBytes(data[offsetZapNative:offsetZapData]), | ||
| ZapData: data[offsetZapData:], | ||
| } | ||
|
|
||
| return tx, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement unit tests for encoding and decoding functions
To ensure the reliability of EncodeBridgeTx and DecodeBridgeTx, consider adding unit tests that verify the correctness of these functions. Testing with various transaction data, including edge cases, will help catch potential issues with offsets and data handling.
Would you like assistance in creating unit tests for these functions?
| for i := range zapDataSize { | ||
| zapData[i] = gofakeit.Uint8() | ||
| } |
There was a problem hiding this comment.
Fix incorrect range loop syntax
Using range on an integer (zapDataSize) is incorrect. This will cause a compilation error.
Apply this diff to fix the loop:
-for i := range zapDataSize {
+for i := 0; i < zapDataSize; i++ {
zapData[i] = gofakeit.Uint8()
}📝 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.
| for i := range zapDataSize { | |
| zapData[i] = gofakeit.Uint8() | |
| } | |
| for i := 0; i < zapDataSize; i++ { | |
| zapData[i] = gofakeit.Uint8() | |
| } |
| // cache so that we don't have to parse the ABI every time. | ||
| var fastBridgeV2ABI *abi.ABI | ||
|
|
||
| const methodName = "relay0" |
There was a problem hiding this comment.
Needs to be updated to "relayV2" to update to #3410
|
|
||
| // Calculate the static L2 fee if it won't be incorporated by directly estimating the relay() call | ||
| // in addZapFees(). | ||
| if quoteRequest == nil || quoteRequest.Transaction.ZapNative == nil || quoteRequest.Transaction.ZapData == nil { |
There was a problem hiding this comment.
This only needs to check if ZapData is a non-zero length byte slice (we don't do getZapGasEstimate if ZapNative > 0 and ZapData is empty).
| func (f *feePricer) addZapFees(ctx context.Context, destination uint32, denomToken string, quoteRequest *reldb.QuoteRequest, fee *big.Int) (*big.Int, error) { | ||
| span := trace.SpanFromContext(ctx) | ||
|
|
||
| if quoteRequest.Transaction.ZapData != nil { |
There was a problem hiding this comment.
I think this also needs to check the ZapData length. The zero length ZapData could also be passed and signals that no additonal Zap action is required.
There was a problem hiding this comment.
Also it's likely that this check and one in GetDestinationFee will be identical (with boolean negation that is), so deduping this logic would be helpful.
| @@ -378,11 +380,33 @@ func (m *Manager) generateActiveRFQ(ctx context.Context, msg *model.ActiveRFQMes | |||
| DestBalance: inv[rfqRequest.Data.DestChainID][common.HexToAddress(rfqRequest.Data.DestTokenAddr)], | |||
| OriginAmountExact: originAmountExact, | |||
| } | |||
| if rfqRequest.Data.ZapNative != "" || rfqRequest.Data.ZapData != "" { | |||
There was a problem hiding this comment.
We should check for non-zero ZapNative here rather than for a non-empty string.
I also think we might need to sanitise ZapNative / ZapData on the API level in terms of the default values ("0" and "" respectively), and also in terms of removing the 0x potential prefix in ZapData
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
services/rfq/e2e/rfq_test.go (3)
878-879: Justify the gosec lint suppressionThe
//nolint:gosecdirective suppresses security-related linting checks. Please add a comment explaining why this is safe to ignore.
892-902: Consider defining constants for random value rangesThe magic numbers used in random value generation (e.g.,
1, 1000000for chain IDs) should be defined as named constants to improve maintainability and document the valid ranges.+const ( + minChainID = 1 + maxChainID = 1000000 +) - originChainId := uint32(gofakeit.Number(1, 1000000)) - destChainId := uint32(gofakeit.Number(1, 1000000)) + originChainId := uint32(gofakeit.Number(minChainID, maxChainID)) + destChainId := uint32(gofakeit.Number(minChainID, maxChainID))
879-980: Consider adding multiple test iterationsThe test currently runs once with a single set of random values. Consider wrapping the test in a loop to run multiple iterations with different random values for more thorough testing.
func (i *IntegrationSuite) TestEncodeBridgeTransactionParity() { + const iterations = 5 + for iter := 0; iter < iterations; iter++ { + i.Run(fmt.Sprintf("iteration_%d", iter), func() { // existing test code + }) + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
packages/contracts-rfq/package.json(1 hunks)services/rfq/e2e/rfq_test.go(3 hunks)services/rfq/testutil/contracttype.go(3 hunks)services/rfq/testutil/contracttypeimpl_string.go(1 hunks)services/rfq/testutil/deployers.go(3 hunks)services/rfq/testutil/typecast.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/contracts-rfq/package.json
- services/rfq/testutil/typecast.go
- services/rfq/testutil/contracttype.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
services/rfq/e2e/rfq_test.go
32-32: could not import github.com/synapsecns/sanguine/services/rfq/testutil (-: # github.com/synapsecns/sanguine/services/rfq/testutil
testutil/typecast.go:21:145: undefined: fastbridge
testutil/contracttypeimpl_string.go:12:8: invalid argument: index 1 out of bounds [0:1]
testutil/contracttypeimpl_string.go:13:8: invalid argument: index 1 out of bounds [0:1]
testutil/contracttypeimpl_string.go:14:8: invalid argument: index 1 out of bounds [0:1]
testutil/contracttypeimpl_string.go:15:8: invalid argument: index 1 out of bounds [0:1]
testutil/contracttypeimpl_string.go:16:8: invalid argument: index 1 out of bounds [0:1]
testutil/contracttypeimpl_string.go:17:8: invalid argument: index 1 out of bounds [0:1]
testutil/contracttypeimpl_string.go:18:8: invalid argument: index 1 out of bounds [0:1]
testutil/contracttypeimpl_string.go:19:8: invalid argument: index 1 out of bounds [0:1]
testutil/deployers.go:118:10: undefined: fastbridge
testutil/deployers.go:118:10: too many errors)
(typecheck)
🪛 GitHub Check: Build (1.22.x)
services/rfq/testutil/contracttypeimpl_string.go
[failure] 12-12:
invalid argument: index 1 out of bounds [0:1]
[failure] 13-13:
invalid argument: index 1 out of bounds [0:1]
[failure] 14-14:
invalid argument: index 1 out of bounds [0:1]
[failure] 15-15:
invalid argument: index 1 out of bounds [0:1]
[failure] 16-16:
invalid argument: index 1 out of bounds [0:1]
[failure] 17-17:
invalid argument: index 1 out of bounds [0:1]
[failure] 18-18:
invalid argument: index 1 out of bounds [0:1]
[failure] 19-19:
invalid argument: index 1 out of bounds [0:1]
🔇 Additional comments (7)
services/rfq/e2e/rfq_test.go (2)
410-411: Remove debug print statement
The debug print statement should be removed to keep the test output clean.
910-912: Fix incorrect range loop syntax
Using range on an integer (zapDataSize) is incorrect and will cause a compilation error.
services/rfq/testutil/contracttypeimpl_string.go (2)
12-15: LGTM: Contract type indices are properly ordered
The reordering of contract types and addition of BridgeTransactionV2Type at index 5 is consistent with the changes described in the summary.
🧰 Tools
🪛 GitHub Check: Build (1.22.x)
[failure] 12-12:
invalid argument: index 1 out of bounds [0:1]
[failure] 13-13:
invalid argument: index 1 out of bounds [0:1]
[failure] 14-14:
invalid argument: index 1 out of bounds [0:1]
[failure] 15-15:
invalid argument: index 1 out of bounds [0:1]
22-22: Verify string representation matches contract types
The name string and index array have been updated to include BridgeTransactionV2, maintaining proper alignment with the contract type indices.
Also applies to: 24-24
✅ Verification successful
Let me gather more information about the contract types defined in the RFQ service to verify the string representation.
String representation correctly matches contract types
The string representation in _contractTypeImpl_name perfectly aligns with the contract types defined in contracttype.go. The order matches the iota-based constants:
- FastBridgeV1 (FastBridgeType)
- FastBridgeV2 (FastBridgeV2Type)
- MockERC20 (MockERC20Type)
- FastBridgeMock (FastBridgeMockType)
- RecipientMock (RecipientMockType)
- BridgeTransactionV2 (BridgeTransactionV2Type)
- WETH9 (WETH9Type)
- USDT (USDTType)
- USDC (USDCType)
- DAI (DAIType)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the stringer tool generated this file correctly
# Look for the go:generate directive that would have created this file
rg --type go "//go:generate.*stringer.*contractTypeImpl"
Length of output: 986
Script:
#!/bin/bash
# Check the contract type definitions in the RFQ service
cat services/rfq/testutil/contracttype.go
Length of output: 4841
services/rfq/testutil/deployers.go (3)
17-17: LGTM: Required import added for bridgetransactionv2
The import is correctly added for the new bridge transaction contract.
32-32: LGTM: BridgeTransactionV2Deployer properly integrated
The deployer manager is updated to include the new BridgeTransactionV2Deployer while maintaining the existing order of deployers.
187-206: Verify the use of Harness contract in deployment
The implementation follows the established pattern for contract deployers, but it uses a Harness version of the contract instead of the main contract.
✅ Verification successful
Let me gather more information about the Harness contracts to understand their purpose.
Let me try a different approach to understand the context of Harness contracts in the test utilities.
Let me check the content of the Harness contract to understand its purpose.
Using Harness contract is appropriate for testing purposes
The BridgeTransactionV2Harness contract is a test utility that exposes internal library functions from BridgeTransactionV2Lib for testing purposes. It provides test-only methods to encode, decode, and access individual fields of bridge transactions, which is essential for thorough testing of the RFQ service. This follows the established pattern in the codebase where Harness contracts are used extensively in test environments to facilitate testing of internal contract functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if there's a specific reason for using the Harness version
# Look for other test contracts using Harness
rg --type go "DeployBridgeTransactionV2Harness"
rg --type go "Harness.*Deploy"
Length of output: 15643
Script:
#!/bin/bash
# Look for the BridgeTransactionV2Harness contract definition
ast-grep --pattern 'contract BridgeTransactionV2Harness $$$'
# Look for test files using BridgeTransactionV2Harness
rg -g '**/*_test.go' "BridgeTransactionV2Harness"
Length of output: 112
Script:
#!/bin/bash
# Look for files containing BridgeTransactionV2Harness
fd BridgeTransactionV2Harness
# Look for test files in the rfq service directory
rg -g '**/*_test.go' --type go "BridgeTransaction.*Test" services/rfq/
Length of output: 286
Script:
#!/bin/bash
# Look at the Harness contract implementation
cat packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol
# Look for other test files using this contract
rg -g '**/*_test.go' "BridgeTransactionV2" services/rfq/
Length of output: 4483
Summary by CodeRabbit
New Features
Bug Fixes
Documentation