feat(rfq-relayer): support FastBridgeV2 with arbitrary calls [SLT-320] #3258
Conversation
![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 (1)
services/rfq/api/client/client.go (1)
Line range hint
278-291: Consider enhancing websocket message monitoringThe addition of debug logging suggests a need for better visibility into message processing. Consider these improvements:
- Add structured logging with correlation IDs for message tracking
- Implement metrics for websocket message processing (success/failure rates, latency)
- Add proper error context when message handling fails
Would you like assistance in implementing these monitoring enhancements?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/rfq/api/client/client.go(2 hunks)services/rfq/api/rest/rfq.go(9 hunks)services/rfq/relayer/quoter/quoter.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/api/rest/rfq.go
🔇 Additional comments (8)
services/rfq/relayer/quoter/quoter.go (8)
24-24: LGTM!
The import of the FastBridgeV2 package is correctly added to support the new bridge functionality.
237-240: LGTM!
The update to pass the quote to GetTotalFee is correct and necessary for accurate fee calculation with FastBridgeV2.
345-346: Remove debug print statements.
Debug print statements should be removed before merging to production.
Also applies to: 373-374
390-396: LGTM!
The FastBridgeV2 quote handling is correctly implemented with proper error handling.
403-415: Add validation for minimum viable destination amount.
Setting negative amounts to zero could lead to unexpected behavior. Consider adding validation to ensure the amount remains above a minimum viable threshold.
437-484: Review default values and assumptions in quote conversion.
Several potential issues in the implementation:
- Using max uint256 for deadline might be too permissive
- Empty exclusivity relayer address might cause contract validation issues
- Assuming dest amount equals origin amount might not account for token decimals or exchange rates
668-668: LGTM!
The addition of the QuoteRequest field to QuoteInput struct is well-designed and properly supports the FastBridgeV2 functionality.
Line range hint 689-693: LGTM!
The update to pass QuoteRequest to GetTotalFee is correct and maintains proper error handling.
services/rfq/api/client/client.go
Outdated
| @@ -283,6 +284,7 @@ func (c *clientImpl) processWebsocket(ctx context.Context, conn *websocket.Conn, | |||
| return fmt.Errorf("error sending message to websocket: %w", err) | |||
| } | |||
| case msg, ok := <-readChan: | |||
| fmt.Printf("recved message from readChan: %+v\n", msg) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fmt.Printf with structured logging
Similar to the previous comment, use the logger instance for consistent logging practices.
-fmt.Printf("recved message from readChan: %+v\n", msg)
+logger.Debugf("received message from readChan: %+v", msg)Committable suggestion skipped: line range outside the PR's diff.
services/rfq/api/client/client.go
Outdated
| @@ -275,6 +275,7 @@ | |||
| case <-ctx.Done(): | |||
| return nil | |||
| case msg, ok := <-reqChan: | |||
| fmt.Printf("recved message from reqChan: %+v\n", msg) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fmt.Printf with structured logging
The debug logging should use the existing logger instance with appropriate log levels instead of fmt.Printf. This ensures consistent logging across the codebase and better control over log levels in production.
-fmt.Printf("recved message from reqChan: %+v\n", msg)
+logger.Debugf("received message from reqChan: %+v", msg)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
services/rfq/relayer/quoter/quoter.go (2)
297-298: Document the rationale for channel buffer size.The buffer size of 1000 seems arbitrary. Please document why this specific size was chosen and consider if it should be configurable based on system load and memory constraints.
Line range hint
379-400: Add validation for zap data length.While the changes handle V2 quote requests correctly, consider adding validation for the zap data length to prevent potential issues with malformed input.
if (rfqRequest.Data.ZapNative != "0" && rfqRequest.Data.ZapNative != "") || rfqRequest.Data.ZapData != "" { + // Validate zap data length + if len(rfqRequest.Data.ZapData) > 0 && len(rfqRequest.Data.ZapData) < 2 { + return nil, fmt.Errorf("invalid zap data length: %d", len(rfqRequest.Data.ZapData)) + } quoteRequest, err := quoteDataToQuoteRequestV2(&rfqRequest.Data)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/rfq/api/client/client.go(7 hunks)services/rfq/api/rest/ws.go(8 hunks)services/rfq/relayer/quoter/quoter.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/rfq/api/rest/ws.go
- services/rfq/api/client/client.go
🔇 Additional comments (5)
services/rfq/relayer/quoter/quoter.go (5)
313-321: LGTM! Improved websocket subscription handling.
The changes enhance the robustness of the websocket subscription handling with proper error management and channel setup.
333-333: Remove debug print statements before merging.
Multiple debug print statements have been added throughout the code. These should be removed before merging to production.
Also applies to: 338-338, 349-350, 377-378, 437-437
407-419: Consider minimum viable destination amount validation.
The current implementation sets the destination amount to 0 when it would be negative after fee deduction. This could lead to undesirable behavior.
442-489: Review default values in quote conversion.
The implementation uses potentially problematic default values:
- Maximum uint256 for deadline
- Empty exclusivity relayer address
238-238: LGTM! Enhanced fee calculation with V2 support.
The fee calculation changes correctly integrate the quote request parameter to support FastBridgeV2 functionality.
Also applies to: 694-694
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
services/rfq/relayer/pricer/fee_pricer.go (2)
236-264: Enhance error handling in getZapGasEstimateThe error handling could be improved by:
- Adding context to errors for better debugging
- Handling potential nil client response
func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (gasEstimate uint64, err error) { + if quoteRequest == nil || quoteRequest.Transaction == nil { + return 0, fmt.Errorf("invalid quote request: %v", quoteRequest) + } client, err := f.clientFetcher.GetClient(ctx, big.NewInt(int64(destination))) if err != nil { return 0, fmt.Errorf("could not get client: %w", err) } + if client == nil { + return 0, fmt.Errorf("received nil client for destination %d", destination) + }
Line range hint
338-384: Improve error handling in getDenomFeeThe method should handle edge cases for token prices and decimal conversions.
func (f *feePricer) getDenomFee(ctx context.Context, gasChain, denomChain uint32, denomToken string, feeWei *big.Float) (*big.Float, error) { + if feeWei == nil { + return nil, fmt.Errorf("feeWei cannot be nil") + } + if feeWei.Sign() < 0 { + return nil, fmt.Errorf("negative fee not allowed: %v", feeWei) + } span := trace.SpanFromContext(ctx)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
services/rfq/api/client/client.go(3 hunks)services/rfq/api/model/request.go(2 hunks)services/rfq/api/rest/rfq.go(2 hunks)services/rfq/api/rest/ws.go(5 hunks)services/rfq/relayer/pricer/fee_pricer.go(11 hunks)services/rfq/relayer/quoter/quoter.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- services/rfq/api/rest/rfq.go
- services/rfq/api/rest/ws.go
- services/rfq/api/model/request.go
- services/rfq/api/client/client.go
🔇 Additional comments (13)
services/rfq/relayer/quoter/quoter.go (7)
24-24: LGTM!
The fastbridgev2 import is correctly added to support the new FastBridgeV2 functionality.
296-297: LGTM!
The channel buffer size of 1000 is appropriate for the websocket subscription to prevent blocking while processing messages.
312-320: LGTM!
The changes properly implement:
- Buffered channel creation
- Error handling with proper context
- Correct span management for tracing
398-410: Consider adding validation for minimum viable destination amount.
Setting destAmountAdj to 0 when it becomes negative could lead to undesirable behavior. A minimum viable amount check would be more appropriate.
432-479: Review default values and assumptions in quote conversion.
The implementation has several potential issues with default values and assumptions.
663-663: LGTM!
The QuoteRequest field is properly added to support FastBridgeV2 functionality.
684-684: LGTM!
The fee calculation is correctly updated to consider FastBridgeV2 quote request data.
services/rfq/relayer/pricer/fee_pricer.go (6)
Line range hint 59-77: Add validation for relayerAddress parameter
The relayerAddress parameter should be validated to ensure it's not a zero address, as this could cause issues with gas estimation and fee calculations.
217-234: Potential Data Race on Global Variable fastBridgeV2ABI
The variable fastBridgeV2ABI is a package-level global that's set without synchronization.
408-409: Handle potential precision loss in fee calculations
The current implementation discards the accuracy flag from Int() conversion.
251-256: Add gas limit to CallMsg for more accurate estimation
The CallMsg struct should include a gas limit to prevent potential out-of-gas scenarios during estimation.
180-215: 🛠️ Refactor suggestion
Add validation for edge cases in addZapFees
The method should handle additional edge cases:
- Negative fee values
- Overflow during fee addition
func (f *feePricer) addZapFees(ctx context.Context, destination uint32, denomToken string, quoteRequest *reldb.QuoteRequest, fee *big.Int) (*big.Int, error) {
+ if fee == nil {
+ return nil, fmt.Errorf("fee cannot be nil")
+ }
+ if fee.Sign() < 0 {
+ return nil, fmt.Errorf("negative fee not allowed: %v", fee)
+ }
span := trace.SpanFromContext(ctx)Likely invalid or redundant comment.
32-34: Verify backward compatibility of interface changes
The addition of the quoteRequest parameter to both GetDestinationFee and GetTotalFee methods might break existing implementations of the FeePricer interface.
✅ Verification successful
Interface changes are safe and already implemented correctly
The search results show that:
- There is only one implementation of the
FeePricerinterface infee_pricer.go - All callers (
quoter.go) are already using the updated interface with thequoteRequestparameter - All tests have been updated to use the new parameter, passing
nilfor backward compatibility - The changes maintain backward compatibility as the implementation handles
nilquoteRequest gracefully
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing implementations of FeePricer interface
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) GetDestinationFee($$$) {
$$$
}'
Length of output: 94
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, let's find all files that might contain FeePricer implementations
rg -l "FeePricer" --type go
# Then, let's look for the actual interface definition and its implementations
rg -A 5 "type FeePricer interface" --type go
# Also search for GetDestinationFee implementations
rg -B 2 -A 5 "func .* GetDestinationFee" --type go
Length of output: 1979
Script:
#!/bin/bash
# Let's check for any other implementations or usages of the interface
# to ensure we have a complete picture of the impact
# Check for mock implementations
rg -l "mock.*FeePricer" --type go
# Look for interface usage in tests
rg -B 2 -A 5 "NewFeePricer|mock.FeePricer" --type go
# Check for any direct calls to these methods
rg -B 2 -A 5 "GetDestinationFee\(|GetTotalFee\(" --type go
Length of output: 26885
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/rfq/testutil/deployers.go (1)
193-193: Fix incorrect commentThe comment incorrectly states "deploys a mock recipient contract" when it should refer to a bridge transaction contract.
-// NewBridgeTransactionV2Deployer deploys a mock recipient contract. +// NewBridgeTransactionV2Deployer deploys a bridge transaction contract.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/rfq/testutil/contracttypeimpl_string.go(1 hunks)services/rfq/testutil/deployers.go(4 hunks)services/rfq/testutil/typecast.go(3 hunks)
🔇 Additional comments (12)
services/rfq/testutil/contracttypeimpl_string.go (3)
12-20: LGTM: Standard Go pattern for validating iota constants
The array index validation is correctly implemented to catch changes in constant values at compile time.
Line range hint 27-34: LGTM: Standard stringer implementation
The String() method correctly implements the stringer interface with proper bounds checking and error handling.
23-25: Verify string indices alignment
The string constants and index array need to be precisely aligned. Let's verify the alignment:
✅ Verification successful
String indices are correctly aligned with contract type names
The verification confirms that the string indices in _contractTypeImpl_index array perfectly align with the contract type names in _contractTypeImpl_name. Each substring extracted using the indices matches a valid and complete contract type name:
- FastBridgeV1 (0-12)
- FastBridgeV2 (12-24)
- MockERC20 (24-33)
- FastBridgeMock (33-47)
- RecipientMock (47-60)
- BridgeTransactionV2 (60-79)
- WETH9 (79-84)
- USDT (84-88)
- USDC (88-92)
- DAI (92-95)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify string indices alignment with contract type names
# Expected: Each substring between indices should match a valid contract type name
# Extract and verify each contract type name
echo "FastBridgeV1FastBridgeV2MockERC20FastBridgeMockRecipientMockBridgeTransactionV2WETH9USDTUSDCDAI" | \
awk 'BEGIN{split("0,12,24,33,47,60,79,84,88,92,95",i,",")}
{for(j=1;j<length(i);j++) print substr($0,i[j]+1,i[j+1]-i[j])}'
Length of output: 338
services/rfq/testutil/deployers.go (4)
17-21: LGTM: Comprehensive initialization of deploy manager
The imports and deploy manager initialization correctly include all necessary contracts and deployers.
Also applies to: 33-34
125-144: LGTM: FastBridgeV2Deployer follows established pattern
The implementation correctly mirrors the existing FastBridgeDeployer pattern while using V2 contract references.
161-164: LGTM: MockFastBridgeDeployer updated for V2
The deployment implementation correctly uses V2 mock contract references.
167-186: LGTM: RecipientMockDeployer follows established pattern
The implementation correctly follows the established deployer pattern.
services/rfq/testutil/typecast.go (5)
28-33: Function addition appears consistent with existing patterns.
The new GetFastBridgeV2 method follows the established pattern of retrieving a deployed contract with manager.GetContract. Ensure this method’s usage is covered by tests to confirm correct instantiation and functionality.
49-54: Recipient mock retrieval method.
The GetRecipientMock name and comment conform to Go conventions, and the method accurately retrieves the recipient mock contract. Looks good as is.
9-9: Confirm that older imports are no longer needed.
The newly introduced imports for bridgetransactionv2, fastbridgev2, fastbridgemockv2, and recipientmock appear correct and consistent with the PR goals. If the old fastbridge references are now obsolete, consider removing that import to avoid confusion or unintentional usage.
Also applies to: 11-11, 13-13, 15-15
56-60: BridgeTransactionV2 retrieval method.
The GetBridgeTransactionV2 function is consistent with the existing pattern for deployed contracts. Ensure thorough test coverage for new BridgeTransactionV2 functionalities to prevent regressions.
✅ Verification successful
Let me gather information about the test coverage and implementation of BridgeTransactionV2.
Let me search specifically for test implementations of BridgeTransactionV2.
BridgeTransactionV2 implementation is well-tested and follows the established pattern.
The verification shows that BridgeTransactionV2 has:
- Integration test coverage in
services/rfq/e2e/rfq_test.gowithTestEncodeBridgeTransactionParity - Proper contract bindings and implementations in both
bridgetransactionv2andfastbridgev2packages - Consistent usage pattern in the relayer service handlers
- Follows the same contract retrieval pattern as other contracts in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for test files related to BridgeTransactionV2
fd -e go -e test.go | grep -i "bridgetransaction"
# Look for BridgeTransactionV2 test implementations
rg -l "BridgeTransactionV2" --type go
# Search for test functions related to BridgeTransactionV2
ast-grep --pattern 'func Test$_($_) {
$$$
}'
Length of output: 1278
Script:
#!/bin/bash
# Look for test cases specifically involving BridgeTransactionV2
rg -A 5 "Test.*BridgeTransaction.*" --type go
# Check the implementation of GetBridgeTransactionV2 and related tests
rg -A 10 "GetBridgeTransactionV2" --type go
# Look for any test setup involving BridgeTransactionV2
rg -A 5 "func \(.*\) (Setup|Before|Test).*BridgeTransaction.*" --type go
Length of output: 21720
43-47: Mock FastBridge updated to V2.
Switching the mock reference to fastbridgemockv2 looks aligned with the new contract version. Verify that any references to the old mock have been removed if they are no longer needed.
✅ Verification successful
Let me generate shell scripts to verify if there are any remaining references to the old FastBridge mock.
Let me search more specifically for the old FastBridge mock version and its references.
Based on the search results, I can now generate the final response:
FastBridge mock V2 update is properly implemented.
The old FastBridge mock package (fastbridgemock) appears to be kept only for reference and metadata, while all active code is using the new V2 version (fastbridgemockv2). The codebase shows consistent usage of V2 in imports and type references, with no active dependencies on the old version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for any references to the old FastBridge mock
rg -l "fastbridgemock\." --type go
# Search for any imports of the old FastBridge mock
rg -l 'fastbridgemock"' --type go
# Search for any type declarations or variable assignments using the old mock
ast-grep --pattern 'fastbridgemock.$_'
Length of output: 1652
Script:
#!/bin/bash
# Search for files containing FastBridgeMock (case-insensitive) to catch all variations
rg -i "fastbridgemock" -A 3 --type go
# Look for any contract deployments or type definitions related to FastBridge
ast-grep --pattern 'FastBridgeMock'
# Check for any test files that might be using the old mock
fd -e go -e test.go -x rg -l "FastBridgeMock" {}
Length of output: 75417
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
services/rfq/api/rest/server_test.go (1)
542-555: 🛠️ Refactor suggestionAdd test coverage for FastBridgeV2.
The test only uses FastBridgeV1 contracts. To ensure compatibility with the new FastBridgeV2 feature, we need test coverage for both V1 and V2 contracts.
Apply this diff to support both contract versions:
func (c *ServerSuite) sendPutQuoteRequest(header string) (*http.Response, error) { + // Test both V1 and V2 contracts + contracts := []struct { + name string + originBridge string + destBridge string + }{ + { + name: "V1", + originBridge: c.cfg.FastBridgeContractsV1[uint32(c.originChainID)], + destBridge: c.cfg.FastBridgeContractsV1[uint32(c.destChainID)], + }, + { + name: "V2", + originBridge: c.cfg.FastBridgeContractsV2[uint32(c.originChainID)], + destBridge: c.cfg.FastBridgeContractsV2[uint32(c.destChainID)], + }, + } + + var responses []*http.Response + for _, contract := range contracts { putData := model.PutRelayerQuoteRequest{ OriginChainID: c.originChainID, DestChainID: c.destChainID, DestTokenAddr: "0xDestTokenAddr", DestAmount: "100.0", MaxOriginAmount: "200.0", FixedFee: "10.0", - OriginFastBridgeAddress: c.cfg.FastBridgeContractsV1[uint32(c.originChainID)], - DestFastBridgeAddress: c.cfg.FastBridgeContractsV1[uint32(c.destChainID)], + OriginFastBridgeAddress: contract.originBridge, + DestFastBridgeAddress: contract.destBridge, } // ... rest of the function + resp, err := sendRequest(putData) + if err != nil { + return nil, fmt.Errorf("failed to send %s request: %w", contract.name, err) + } + responses = append(responses, resp) + } + // Return the last response for backward compatibility + return responses[len(responses)-1], nil }services/rfq/api/rest/server.go (1)
103-142: 🛠️ Refactor suggestionConsider extracting duplicate initialization logic
The initialization code for V1 and V2 contracts is nearly identical. This violates the DRY principle.
Extract the common initialization logic into a helper function:
+func initializeBridgeContracts[T roleContract]( + ctx context.Context, + contracts map[string]string, + omniRPCClient omniClient.RPCClient, + newContract func(common.Address, bind.ContractBackend) (T, error), +) (map[uint32]T, map[uint32]*ttlcache.Cache[string, bool], error) { + bridgeContracts := make(map[uint32]T) + roles := make(map[uint32]*ttlcache.Cache[string, bool]) + + for chainID, contract := range contracts { + chainClient, err := omniRPCClient.GetChainClient(ctx, int(chainID)) + if err != nil { + return nil, nil, fmt.Errorf("could not create omnirpc client: %w", err) + } + + bridgeContracts[chainID], err = newContract(common.HexToAddress(contract), chainClient) + if err != nil { + return nil, nil, fmt.Errorf("could not create bridge contract: %w", err) + } + + roles[chainID] = ttlcache.New[string, bool]( + ttlcache.WithTTL[string, bool](cacheInterval), + ) + roleCache := roles[chainID] + go roleCache.Start() + go func() { + <-ctx.Done() + roleCache.Stop() + }() + } + return bridgeContracts, roles, nil +}
🧹 Nitpick comments (4)
services/rfq/api/rest/server_test.go (1)
615-616: Enhance contract validation in TestContracts.While the test verifies the length of both V1 and V2 contracts, it should also validate the contract addresses to ensure they are properly configured.
Add these assertions:
c.Require().Len(contracts.ContractsV1, 2) c.Require().Len(contracts.ContractsV2, 2) + // Validate contract addresses + for chainID, addr := range contracts.ContractsV1 { + c.Require().NotEmpty(addr, "FastBridgeV1 contract address for chain %d should not be empty", chainID) + c.Require().True(common.IsHexAddress(addr), "FastBridgeV1 contract address %s for chain %d should be a valid hex address", addr, chainID) + } + for chainID, addr := range contracts.ContractsV2 { + c.Require().NotEmpty(addr, "FastBridgeV2 contract address for chain %d should not be empty", chainID) + c.Require().True(common.IsHexAddress(addr), "FastBridgeV2 contract address %s for chain %d should be a valid hex address", addr, chainID) + }services/rfq/relayer/quoter/quoter.go (2)
388-394: Add validation for ZapNative and ZapData.Consider adding validation to ensure:
- ZapNative is not negative
- ZapData length is within acceptable limits
Apply this diff to add validation:
if (rfqRequest.Data.ZapNative != "0" && rfqRequest.Data.ZapNative != "") || rfqRequest.Data.ZapData != "" { + // Validate ZapNative is not negative + if zapNative, ok := new(big.Int).SetString(rfqRequest.Data.ZapNative, 10); !ok || zapNative.Sign() < 0 { + return nil, errors.New("invalid or negative zap native value") + } + // Validate ZapData length + if len(rfqRequest.Data.ZapData) > 4096 { // adjust limit as needed + return nil, errors.New("zap data exceeds maximum length") + } quoteRequest, err := quoteDataToQuoteRequestV2(&rfqRequest.Data) if err != nil { return nil, fmt.Errorf("error converting quote data to quote request: %w", err) } quoteInput.QuoteRequest = quoteRequest }
328-328: Wrap context error for consistent error handling.The error returned from
ctx.Err()should be wrapped to maintain consistent error handling throughout the codebase.Apply this diff:
-return ctx.Err() +return fmt.Errorf("context cancelled: %w", ctx.Err())🧰 Tools
🪛 golangci-lint (1.62.2)
328-328: error returned from interface method should be wrapped: sig: func (context.Context).Err() error
(wrapcheck)
🪛 GitHub Check: Lint (services/rfq)
[failure] 328-328:
error returned from interface method should be wrapped: sig: func (context.Context).Err() error (wrapcheck)services/rfq/api/rest/server.go (1)
Line range hint
391-432: Consider adding timeout to role checkingThe
checkRolefunction makes external calls without a timeout, which could lead to hanging requests.Add a timeout to the context:
-func (r *QuoterAPIServer) checkRole(c *gin.Context, destChainID uint32, useV1 bool, role [32]byte) (addressRecovered common.Address, err error) { +func (r *QuoterAPIServer) checkRole(c *gin.Context, destChainID uint32, useV1 bool, role [32]byte) (addressRecovered common.Address, err error) { + ctx, cancel := context.WithTimeout(c.Request.Context(), 5*time.Second) + defer cancel() + var bridge roleContract // ... rest of the function ... - ops := &bind.CallOpts{Context: c} + ops := &bind.CallOpts{Context: ctx}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
services/rfq/api/rest/rfq.go(2 hunks)services/rfq/api/rest/rfq_test.go(1 hunks)services/rfq/api/rest/server.go(10 hunks)services/rfq/api/rest/server_test.go(2 hunks)services/rfq/relayer/quoter/quoter.go(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/rfq/api/rest/rfq_test.go
- services/rfq/api/rest/rfq.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
services/rfq/relayer/quoter/quoter.go
328-328: error returned from interface method should be wrapped: sig: func (context.Context).Err() error
(wrapcheck)
🪛 GitHub Check: Lint (services/rfq)
services/rfq/relayer/quoter/quoter.go
[failure] 328-328:
error returned from interface method should be wrapped: sig: func (context.Context).Err() error (wrapcheck)
🔇 Additional comments (8)
services/rfq/relayer/quoter/quoter.go (3)
24-24: LGTM: Import and struct changes for FastBridgeV2 support.
The addition of the fastbridgev2 package import and the QuoteRequest field to QuoteInput struct are well-structured changes that support the new functionality.
Also applies to: 666-666
401-413: Add validation for minimum viable destination amount.
The destination amount adjustment could result in a zero amount, which might not be desirable for all scenarios.
435-482: Review default values and assumptions in quote conversion.
Several potential issues in the implementation:
- Using max uint256 for deadline might be too permissive
- Empty exclusivity relayer address might cause contract validation issues
- Assuming dest amount equals origin amount might not account for token decimals or exchange rates
services/rfq/api/rest/server.go (5)
23-23: LGTM: Required imports added
The addition of errgroup for parallel execution and fastbridgev2 for the new bridge version is appropriate.
Also applies to: 39-39
56-66: LGTM: Clean struct extension
The QuoterAPIServer struct has been cleanly extended to support both V1 and V2 versions of the FastBridge contracts and role caches.
354-389: LGTM: Well-implemented parallel role checking
The checkRoleParallel function efficiently checks roles for both V1 and V2 contracts concurrently using errgroup. The error handling and fallback logic are robust.
615-629: LGTM: Improved quote handling logic
The quote handling logic has been enhanced to support both active and passive quotes, with proper error handling and telemetry.
677-687:
Add input validation for ZapNative
The isZapQuote function could panic if req or req.Data is nil.
Add proper nil checks:
func isZapQuote(req *model.PutRFQRequest) bool {
+ if req == nil || req.Data == nil {
+ return false
+ }
+
if req.Data.ZapData != "" {
return true
}
+ if req.Data.ZapNative == "" {
+ return false
+ }
+
zapNative, ok := new(big.Int).SetString(req.Data.ZapNative, 10)
if !ok {
return false
}
return zapNative.Sign() != 0
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
services/rfq/guard/service/guard.go (1)
Line range hint
319-366: Consider unifying chain indexer implementationsThe V1 and V2 indexer implementations share significant code. Consider creating a generic indexer implementation:
+ func (g Guard) runChainIndexer[T any]( + ctx context.Context, + chainID int, + chainListener listener.ContractListener, + newParser func(common.Address) (T, error), + handleEvent func(context.Context, T, int) error, + ) error { + parser, err := newParser(chainListener.Address()) + if err != nil { + return fmt.Errorf("could not parse: %w", err) + } + + return chainListener.Listen(ctx, func(parentCtx context.Context, log types.Log) error { + // ... common logging and event handling logic + }) + }
♻️ Duplicate comments (1)
services/rfq/guard/service/guard.go (1)
262-262:⚠️ Potential issueFix potential integer overflow in block number conversion
Converting
log.BlockNumberfromuint64toint64may cause integer overflow. Consider using string representation for block numbers in logs.- attribute.Int64("block_number", int64(log.BlockNumber)), + attribute.String("block_number", fmt.Sprintf("%d", log.BlockNumber)),
🧹 Nitpick comments (4)
services/rfq/e2e/rfq_test.go (1)
481-486: Consider parameterizing test valuesThe hardcoded values for
QuoteExclusivitySeconds,ZapData, andZapNativeshould be extracted into constants for better maintainability.services/rfq/e2e/setup_test.go (2)
230-245: Consider enhancing error handling in setupRecipientMock.The error handling could be improved by returning the error instead of using
i.NoError(err), allowing better error propagation and testing flexibility.-func (i *IntegrationSuite) setupRecipientMock() { +func (i *IntegrationSuite) setupRecipientMock() error { testBackends := core.ToSlice(i.originBackend, i.destBackend) for _, b := range testBackends { backend := b err := retry.WithBackoff(i.GetTestContext(), func(_ context.Context) (err error) { handle := i.manager.Get(i.GetTestContext(), backend, testutil.RecipientMockType) err = i.waitForContractDeployment(i.GetTestContext(), backend, handle.Address()) if err != nil { return fmt.Errorf("failed to wait for contract deployment: %w", err) } return nil }, retry.WithMaxTotalTime(30*time.Second)) - i.NoError(err) + if err != nil { + return fmt.Errorf("failed to setup recipient mock for backend %d: %w", backend.GetChainID(), err) + } } + return nil }
536-554: Consider consolidating role granting logic.The role granting logic for V1 and V2 bridges is duplicated. Consider extracting this into a helper function to improve maintainability.
+func (i *IntegrationSuite) grantGuardRole(backend backends.SimulatedTestBackend, metadata contracts.DeployedContract, contract interface { + GUARDROLE(*bind.CallOpts) ([32]byte, error) + GrantRole(*bind.TransactOpts, [32]byte, common.Address) (*types.Transaction, error) +}) error { + txContext := backend.GetTxContext(i.GetTestContext(), metadata.OwnerPtr()) + guardRole, err := contract.GUARDROLE(&bind.CallOpts{Context: i.GetTestContext()}) + if err != nil { + return fmt.Errorf("failed to get guard role: %w", err) + } + tx, err := contract.GrantRole(txContext.TransactOpts, guardRole, i.guardWallet.Address()) + if err != nil { + return fmt.Errorf("failed to grant guard role: %w", err) + } + backend.WaitForConfirmation(i.GetTestContext(), tx) + return nil +}services/rfq/guard/service/guard.go (1)
37-40: Consider grouping related fields using nested structsThe addition of V1 and V2 specific fields creates parallel structures. Consider organizing these fields into version-specific nested structs for better maintainability:
type Guard struct { cfg guardconfig.Config metrics metrics.Handler db guarddb.Service client omniClient.RPCClient - contractsV1 map[int]*fastbridge.FastBridgeRef - contractsV2 map[int]*fastbridgev2.FastBridgeV2Ref - listenersV1 map[int]listener.ContractListener - listenersV2 map[int]listener.ContractListener + v1 struct { + contracts map[int]*fastbridge.FastBridgeRef + listeners map[int]listener.ContractListener + } + v2 struct { + contracts map[int]*fastbridgev2.FastBridgeV2Ref + listeners map[int]listener.ContractListener + } txSubmitter submitter.TransactionSubmitter otelRecorder iOtelRecorder }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
services/rfq/contracts/fastbridgev2/parser.go(1 hunks)services/rfq/e2e/rfq_test.go(10 hunks)services/rfq/e2e/setup_test.go(9 hunks)services/rfq/guard/service/guard.go(9 hunks)services/rfq/relayer/quoter/quoter.go(10 hunks)
🔇 Additional comments (18)
services/rfq/e2e/rfq_test.go (5)
24-36: LGTM: Import changes are appropriate
The new imports support the FastBridgeV2 functionality and are correctly organized.
495-511: Fix error handling in goroutine
Error handling in goroutines using i.NoError(err) is unsafe and may cause race conditions.
172-179: LGTM: Consistent V2 interface migration
The test functions have been correctly updated to use FastBridgeV2 while maintaining the original test logic and assertions.
Also applies to: 325-334
617-710: Reduce code duplication between dispute tests
TestDisputeV2 shares significant setup code with TestDisputeV1. Consider extracting common setup into helper functions.
910-912: Fix range loop in zapData generation
The range loop uses zapDataSize directly which will cause a panic. It should iterate over indices instead.
services/rfq/relayer/quoter/quoter.go (6)
24-24: LGTM: Import for FastBridgeV2 support
The import is correctly added to support the new FastBridgeV2 functionality.
301-302: Verify the channel buffer size
The buffer size of 1000 might be excessive. Consider:
- Monitoring channel utilization in production to tune this value
- Adding a configuration option for this value
- Implementing backpressure mechanisms if the channel fills up
317-325: LGTM: Improved error handling and channel setup
The changes enhance reliability with:
- Proper error handling with metrics
- Correct span management
- Buffered channel to handle load spikes
436-483: Review default values and assumptions
Several potential issues in the implementation:
- Using max uint256 for deadline might be too permissive
- Empty exclusivity relayer address might cause contract validation issues
- Assuming dest amount equals origin amount might not account for token decimals or exchange rates
Run the following script to verify the impact:
#!/bin/bash
# Check for any validation requirements in the FastBridgeV2 contract
ast-grep --pattern $'contract FastBridgeV2 {
$$$
function validateBridgeTransaction($_, $_) {
$$$
}
$$$
}'667-667: LGTM: QuoteInput struct enhancement
The addition of QuoteRequest field properly supports the FastBridgeV2 functionality.
Line range hint 688-692: LGTM: Fee calculation enhancement
The fee calculation now properly integrates with FastBridgeV2 by utilizing the QuoteRequest parameter.
services/rfq/contracts/fastbridgev2/parser.go (5)
28-28: Fix typo in comment: 'contracat' should be 'contract'.
47-47: High cyclomatic complexity in ParseEvent method.
71-75: Rename variable 'requested' to 'relayed' in 'BridgeRelayedEvent' case for clarity.
89-93: Ensure consistent naming between event type and parsing method in 'BridgeDisputeEvent' case.
94-96: Handle unknown event types explicitly.
services/rfq/e2e/setup_test.go (2)
62-69: LGTM: Bridge configuration is properly structured.
The configuration correctly handles both FastBridgeV1 and FastBridgeV2 contracts with appropriate chain IDs and addresses.
272-309: Verify token approvals for both bridge versions.
The Approve method now handles both bridge versions correctly. However, let's verify that all token approvals are properly set.
✅ Verification successful
Let me verify the token approvals for both bridge versions with additional checks.
Based on the code analysis and verification results, I can now provide the final response:
Token approvals are correctly implemented for both bridge versions
The code properly handles token approvals for both FastBridgeV1 and FastBridgeV2:
- Checks existing allowance before approving
- Uses
abi.MaxUint256for unlimited approval amount - Implements retry mechanism with backoff for failed transactions
- Verifies allowance after approval
- Handles each bridge version independently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify token approvals for both bridge versions
# Search for Approval events in the codebase to ensure consistent implementation
ast-grep --pattern 'function Approve($$$) {
$$$
erc20.$$$
$$$
}'
rg -A 5 'event Approval\('
Length of output: 65638
Script:
#!/bin/bash
# Search for token approval implementations and checks in the codebase
rg -A 5 'func.*Approve' services/rfq/
rg -A 5 'allowance.*=.*0' services/rfq/
Length of output: 93746
| metadataV1, rfqContractV1 := i.manager.GetFastBridge(i.GetTestContext(), backend) | ||
| txContextV1 := backend.GetTxContext(i.GetTestContext(), metadataV1.OwnerPtr()) | ||
|
|
||
| relayerRole, err := rfqContractV1.RELAYERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get relayer role: %w", err) | ||
| } | ||
| proverRole, err := rfqContractV1.RELAYERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get relayer role: %w", err) | ||
| } | ||
| tx, err := rfqContractV1.GrantRole(txContextV1.TransactOpts, relayerRole, i.relayerWallet.Address()) | ||
| if err != nil { | ||
| return fmt.Errorf("could not grant relayer role: %w", err) | ||
| } | ||
| backend.WaitForConfirmation(i.GetTestContext(), tx) | ||
|
|
||
| metadataV2, rfqContractV2 := i.manager.GetFastBridgeV2(i.GetTestContext(), backend) | ||
| txContextV2 := backend.GetTxContext(i.GetTestContext(), metadataV2.OwnerPtr()) | ||
|
|
||
| txContext := backend.GetTxContext(i.GetTestContext(), metadata.OwnerPtr()) | ||
| relayerRole, err := rfqContract.RELAYERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | ||
| tx, err = rfqContractV2.GrantRole(txContextV2.TransactOpts, proverRole, i.relayerWallet.Address()) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get relayer role: %w", err) | ||
| return fmt.Errorf("could not grant prover role: %w", err) | ||
| } | ||
| backend.WaitForConfirmation(i.GetTestContext(), tx) | ||
|
|
||
| tx, err := rfqContract.GrantRole(txContext.TransactOpts, relayerRole, i.relayerWallet.Address()) | ||
| quoterRole, err := rfqContractV2.QUOTERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | ||
| if err != nil { | ||
| return fmt.Errorf("could not grant role: %w", err) | ||
| return fmt.Errorf("could not get quoter role: %w", err) | ||
| } | ||
| tx, err = rfqContractV2.GrantRole(txContextV2.TransactOpts, quoterRole, i.relayerWallet.Address()) | ||
| if err != nil { | ||
| return fmt.Errorf("could not grant quoter role: %w", err) | ||
| } | ||
| backend.WaitForConfirmation(i.GetTestContext(), tx) |
There was a problem hiding this comment.
Fix ineffectual assignment in setupRelayer.
There's an ineffectual assignment to proverRole that needs to be fixed.
- proverRole, err := rfqContractV1.RELAYERROLE(&bind.CallOpts{Context: i.GetTestContext()})
+ proverRole, err := rfqContractV1.PROVERROLE(&bind.CallOpts{Context: i.GetTestContext()})
if err != nil {
return fmt.Errorf("failed to get relayer role: %w", err)
}Committable suggestion skipped: line range outside the PR's diff.
| // setup v1 | ||
| rfqAddrV1, err := cfg.GetRFQAddressV1(chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get rfq address: %w", err) | ||
| } | ||
| if rfqAddrV1 != nil { | ||
| chainClient, err := omniClient.GetChainClient(ctx, chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain client: %w", err) | ||
| } | ||
| contract, err := fastbridge.NewFastBridgeRef(common.HexToAddress(*rfqAddrV1), chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create fast bridge contract: %w", err) | ||
| } | ||
| startBlock, err := contract.DeployBlock(&bind.CallOpts{Context: ctx}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get deploy block: %w", err) | ||
| } | ||
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(*rfqAddrV1), uint64(startBlock.Int64()), //nolint:gosec // Acceptable conversion | ||
| metricHandler, listener.WithName("guard")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain listener: %w", err) | ||
| } | ||
| listenersV1[chainID] = chainListener | ||
| contractsV1[chainID] = contract | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in contract initialization
The initialization logic for V1 and V2 contracts contains significant duplication. Consider extracting the common initialization logic into a helper method:
+ func (g *Guard) initializeContract[T any](
+ ctx context.Context,
+ chainID int,
+ rfqAddr string,
+ newContract func(common.Address, bind.ContractBackend) (T, error),
+ metricHandler metrics.Handler,
+ listenerName string,
+ ) (T, listener.ContractListener, error) {
+ chainClient, err := g.client.GetChainClient(ctx, chainID)
+ if err != nil {
+ return nil, nil, fmt.Errorf("could not get chain client: %w", err)
+ }
+
+ contract, err := newContract(common.HexToAddress(rfqAddr), chainClient)
+ if err != nil {
+ return nil, nil, fmt.Errorf("could not create contract: %w", err)
+ }
+
+ startBlock, err := contract.DeployBlock(&bind.CallOpts{Context: ctx})
+ if err != nil {
+ return nil, nil, fmt.Errorf("could not get deploy block: %w", err)
+ }
+
+ chainListener, err := listener.NewChainListener(
+ chainClient,
+ g.db,
+ common.HexToAddress(rfqAddr),
+ uint64(startBlock.Int64()),
+ metricHandler,
+ listener.WithName(listenerName),
+ )
+ if err != nil {
+ return nil, nil, fmt.Errorf("could not get chain listener: %w", err)
+ }
+
+ return contract, chainListener, nil
+ }This would simplify the initialization code and make it more maintainable.
Also applies to: 94-117
| v2Event := &fastbridgev2.FastBridgeV2BridgeRequested{ | ||
| TransactionId: event.TransactionId, | ||
| Sender: event.Sender, | ||
| Request: event.Request, | ||
| DestChainId: event.DestChainId, | ||
| OriginToken: event.OriginToken, | ||
| DestToken: event.DestToken, | ||
| OriginAmount: event.OriginAmount, | ||
| DestAmount: event.DestAmount, | ||
| SendChainGas: event.SendChainGas, | ||
| Raw: event.Raw, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a type converter utility for event conversions
Manual event conversion between V1 and V2 types is error-prone and could lead to missed fields in future updates. Consider creating a dedicated converter utility:
+ type eventConverter struct{}
+
+ func (c *eventConverter) bridgeRequestedV1ToV2(v1 *fastbridge.FastBridgeBridgeRequested) *fastbridgev2.FastBridgeV2BridgeRequested {
+ return &fastbridgev2.FastBridgeV2BridgeRequested{
+ TransactionId: v1.TransactionId,
+ Sender: v1.Sender,
+ Request: v1.Request,
+ DestChainId: v1.DestChainId,
+ OriginToken: v1.OriginToken,
+ DestToken: v1.DestToken,
+ OriginAmount: v1.OriginAmount,
+ DestAmount: v1.DestAmount,
+ SendChainGas: v1.SendChainGas,
+ Raw: v1.Raw,
+ }
+ }Also applies to: 288-293, 299-303
| func (g *Guard) isV2Address(chainID int, addr common.Address) bool { | ||
| rfqAddr, err := g.cfg.GetRFQAddressV2(chainID) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return addr == common.HexToAddress(rfqAddr) | ||
| } |
There was a problem hiding this comment.
Improve error handling in isV2Address
The method silently fails by returning false when there's an error. Consider either:
- Returning the error to the caller for proper handling
- Logging the error for debugging purposes
- func (g *Guard) isV2Address(chainID int, addr common.Address) bool {
+ func (g *Guard) isV2Address(chainID int, addr common.Address) (bool, error) {
rfqAddr, err := g.cfg.GetRFQAddressV2(chainID)
if err != nil {
- return false
+ return false, fmt.Errorf("failed to get V2 address: %w", err)
}
- return addr == common.HexToAddress(rfqAddr)
+ return addr == common.HexToAddress(rfqAddr), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (g *Guard) isV2Address(chainID int, addr common.Address) bool { | |
| rfqAddr, err := g.cfg.GetRFQAddressV2(chainID) | |
| if err != nil { | |
| return false | |
| } | |
| return addr == common.HexToAddress(rfqAddr) | |
| } | |
| func (g *Guard) isV2Address(chainID int, addr common.Address) (bool, error) { | |
| rfqAddr, err := g.cfg.GetRFQAddressV2(chainID) | |
| if err != nil { | |
| return false, fmt.Errorf("failed to get V2 address: %w", err) | |
| } | |
| return addr == common.HexToAddress(rfqAddr), nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
services/rfq/relayer/pricer/fee_pricer.go (2)
264-267: Enhance error message for gas estimation failures.The error message should include context about the destination chain and transaction details for easier debugging.
- return 0, fmt.Errorf("could not estimate gas: %w", err) + return 0, fmt.Errorf("failed to estimate gas on chain %d for transaction %s: %w", destination, quoteRequest.Transaction.Hash, err)
185-216: Consider adding circuit breaker for fee calculations.The
addZapFeesmethod should have upper bounds for fees to prevent excessive charges. Consider adding configurable maximum fee limits.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/rfq/relayer/pricer/fee_pricer.go(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Generate (Module Changes) (agents)
🔇 Additional comments (7)
services/rfq/relayer/pricer/fee_pricer.go (7)
33-35: LGTM! Interface changes look good.The updated method signatures in the FeePricer interface properly reflect the new requirements for handling zap transactions.
55-56: Add validation for relayerAddress in constructor.The relayerAddress field is critical for transaction signing and fee estimation. A zero address could cause issues.
Also applies to: 76-76
146-146: Fix potential nil pointer dereference.The code accesses
quoteRequest.Transaction.ZapDatawithout checking ifTransactionis nil.
218-235: Fix data race on fastBridgeV2ABI initialization.The global variable
fastBridgeV2ABIis accessed and modified without synchronization, which could lead to data races in concurrent scenarios.
252-262: Add gas limit to CallMsg for accurate estimation.The gas estimation might be inaccurate without a reasonable gas limit set in the CallMsg struct.
414-414: Handle potential precision loss in fee calculations.The code discards the accuracy flag from the Int() conversion, which could silently ignore precision loss.
201-213: Simplify ZapNative value check.The condition has redundant checks for ZapNative value comparison.
| if util.IsGasToken(quoteRequest.Transaction.DestToken) { | ||
| callMsg.Value = quoteRequest.Transaction.DestAmount | ||
| } else { | ||
| callMsg.Value = quoteRequest.Transaction.ZapNative | ||
| } |
There was a problem hiding this comment.
Add error handling for token validation.
The code should validate that DestToken exists and is properly configured before checking if it's a gas token.
+ destToken, err := f.config.GetToken(int(destination), quoteRequest.Transaction.DestToken)
+ if err != nil {
+ return 0, fmt.Errorf("invalid destination token: %w", err)
+ }
- if util.IsGasToken(quoteRequest.Transaction.DestToken) {
+ if util.IsGasToken(destToken) {Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ethergo/submitter/submitter.go(1 hunks)services/rfq/relayer/quoter/quoter.go(11 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 700-700: ethergo/submitter/submitter.go#L700
Added line #L700 was not covered by tests
[warning] 705-708: ethergo/submitter/submitter.go#L705-L708
Added lines #L705 - L708 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: run-goreleaser (services/omnirpc)
- GitHub Check: run-goreleaser (services/explorer)
- GitHub Check: run-goreleaser (contrib/promexporter)
- GitHub Check: run-goreleaser (agents)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
services/rfq/relayer/quoter/quoter.go (4)
24-24: LGTM! Import aligns with FastBridgeV2 support.The addition of the fastbridgev2 import is appropriate for implementing the new bridge functionality.
696-698: Remove temporary debug print statements.These debug statements should be removed as they:
- Could expose sensitive information in logs
- Add unnecessary noise to the output
- Are marked with "tmpdebug" comment
Apply this diff to remove the debug statements:
- // tmpdebug - fmt.Printf("Debug Total Fee Amt: %s\n", fee.String()) - - // tmpdebug - fmt.Printf("Debug originRFQAddr: %s\n", originRFQAddr.String()) - - // tmpdebug - fmt.Printf("Debug destAmount: %s\n", destAmount.String())Also applies to: 705-707, 715-717
404-416: Add validation for minimum viable destination amount.The destination amount adjustment could result in a zero amount, which might not be desirable for all scenarios.
Consider adding validation to ensure the amount remains above a minimum viable threshold:
destAmountAdj := new(big.Int).Sub(destAmountBigInt, fixedFeeBigInt) if destAmountAdj.Sign() < 0 { - destAmountAdj = big.NewInt(0) + return nil, fmt.Errorf("adjusted destination amount would be negative: amount=%s, fee=%s", + destAmountBigInt.String(), fixedFeeBigInt.String()) } +minViableAmount := big.NewInt(1000) // Configure this based on your requirements +if destAmountAdj.Cmp(minViableAmount) < 0 { + return nil, fmt.Errorf("adjusted destination amount %s below minimum viable amount %s", + destAmountAdj.String(), minViableAmount.String()) +}
439-485: Review default values and assumptions in quote conversion.Several potential issues in the implementation:
- Using max uint256 for deadline might be too permissive
- Empty exclusivity relayer address might cause contract validation issues
- Assuming dest amount equals origin amount might not account for token decimals or exchange rates
Consider these improvements:
- deadline := new(big.Int).Sub(new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil), big.NewInt(1)) + deadline := new(big.Int).Add(big.NewInt(time.Now().Unix()), big.NewInt(DefaultQuoteDeadline)) - destAmount := originAmount // assume dest amount same as origin amount for estimation purposes + destAmount := calculateDestAmount(originAmount, quoteData.OriginTokenAddr, quoteData.DestTokenAddr) - exclusivityRelayer := common.HexToAddress("") + exclusivityRelayer := common.HexToAddress(DefaultExclusivityRelayer)
|
|
||
| // tmpdebug | ||
| fmt.Printf("Debug Default Gas Estimate: %d\n", t.config.GetGasEstimate(chainID)) | ||
|
|
There was a problem hiding this comment.
Remove temporary debug statement or replace with proper logging.
The debug print statement appears to be temporary and doesn't use proper formatting for the gas estimate value. Consider removing it or replacing it with structured logging using the package's logger.
- // tmpdebug
- fmt.Printf("Debug Default Gas Estimate: %d\n", t.config.GetGasEstimate(chainID))
+ logger.Debugf("falling back to default gas estimate: %d", t.config.GetGasEstimate(chainID))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 705-708: ethergo/submitter/submitter.go#L705-L708
Added lines #L705 - L708 were not covered by tests
| // tmpdebug | ||
| fmt.Printf("Debug Calling EstimateGas") |
There was a problem hiding this comment.
Remove temporary debug statement or replace with proper logging.
The debug print statement appears to be temporary and doesn't follow the established logging patterns. Consider removing it or replacing it with the package's logger.
- // tmpdebug
- fmt.Printf("Debug Calling EstimateGas")
+ logger.Debug("calling EstimateGas")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 700-700: ethergo/submitter/submitter.go#L700
Added line #L700 was not covered by tests
| // tmpdebug | ||
| fmt.Printf("Debug Calling EstimateGas") | ||
|
|
||
| gasEstimate, err = chainClient.EstimateGas(ctx, *call) | ||
| if err != nil { | ||
| span.AddEvent("could not estimate gas", trace.WithAttributes(attribute.String("error", err.Error()))) | ||
|
|
||
| // tmpdebug | ||
| fmt.Printf("Debug Default Gas Estimate: %d\n", t.config.GetGasEstimate(chainID)) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for gas estimation error handling in submitter package
The review comment is valid. While there are EstimateGas tests in other packages, the error handling path in ethergo/submitter/submitter.go lacks test coverage. Specifically:
- The error case where gas estimation fails
- The fallback to default gas estimate
- The debug logging behavior
🔗 Analysis chain
Add test coverage for gas estimation error handling.
The added debug statements and error handling path are not covered by tests. Consider adding test cases to verify the behavior when gas estimation fails.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing test coverage of gas estimation error handling
rg -A 5 "TestGetGasEstimate.*Error" || echo "No test found for gas estimation error handling"
Length of output: 142
Script:
#!/bin/bash
# Search for test files in submitter package
echo "=== Searching for test files in submitter package ==="
fd ".*_test.go" submitter/
echo "=== Searching for EstimateGas usage in tests ==="
rg -l "EstimateGas.*" --type go | grep "_test.go" || echo "No EstimateGas usage found in test files"
echo "=== Searching for mock chain client ==="
rg -l "type.*Chain.*Client.*interface" --type go || echo "No chain client interface found"
Length of output: 914
Script:
#!/bin/bash
# Find the correct path of submitter.go
echo "=== Finding submitter.go location ==="
fd submitter.go
echo "=== Checking EstimateGas test implementations ==="
rg -A 10 "func.*Test.*EstimateGas" --type go || echo "No EstimateGas test functions found"
echo "=== Checking for error case testing ==="
rg "EstimateGas.*err.*!= nil" --type go || echo "No error handling tests found"
Length of output: 4366
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 700-700: ethergo/submitter/submitter.go#L700
Added line #L700 was not covered by tests
[warning] 705-708: ethergo/submitter/submitter.go#L705-L708
Added lines #L705 - L708 were not covered by tests
Summary by CodeRabbit
New Features
fastbridgev2contract across various components.Bug Fixes
Documentation
Tests
fastbridgev2contract and updated fee pricing logic.