fix(rfq-relayer): gas estimation for zaps #3413
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/arb-call-fee-pricer #3413 +/- ##
==================================================================
+ Coverage 30.57730% 31.37797% +0.80067%
==================================================================
Files 457 463 +6
Lines 30019 30378 +359
Branches 82 82
==================================================================
+ Hits 9179 9532 +353
- Misses 19990 19996 +6
Partials 850 850
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: |
8d8491a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0379cdfd.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-arb-call-fee-pricer-enc.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
services/rfq/api/model/request.go (2)
62-63: Consider adding address validation tagsThe new
OriginSenderandDestRecipientfields store blockchain addresses. Consider adding validation tags to ensure these addresses are properly formatted before processing.- OriginSender string `json:"origin_sender"` - DestRecipient string `json:"dest_recipient"` + OriginSender string `json:"origin_sender" validate:"required,eth_addr"` + DestRecipient string `json:"dest_recipient" validate:"required,eth_addr"`
62-64: Document the relationship with gas estimationSince these changes are related to "gas estimation for zaps", consider adding documentation comments explaining how these fields (
OriginSender,DestRecipient, and nullableDestAmount) are used in the gas estimation process.+// OriginSender is the address initiating the zap transaction, used for gas estimation OriginSender string `json:"origin_sender"` +// DestRecipient is the address receiving the zapped tokens DestRecipient string `json:"dest_recipient"` +// DestAmount is the target amount for the zap, nullable for gas estimation scenarios DestAmount *string `json:"dest_amount"`services/rfq/relayer/pricer/fee_pricer.go (1)
316-316: Consider making the version number configurableThe version number (2) is hardcoded. Consider making it configurable through the config system to facilitate future protocol version changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
services/rfq/api/model/request.go(1 hunks)services/rfq/relayer/pricer/fee_pricer.go(5 hunks)services/rfq/relayer/quoter/quoter.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
services/rfq/relayer/pricer/fee_pricer.go
315-315: error returned from external package is unwrapped: sig: func (github.com/ethereum/go-ethereum/accounts/abi.Arguments).Pack(args ...interface{}) ([]byte, error)
(wrapcheck)
🪛 GitHub Check: Lint (services/rfq)
services/rfq/relayer/pricer/fee_pricer.go
[failure] 315-315:
error returned from external package is unwrapped: sig: func (github.com/ethereum/go-ethereum/accounts/abi.Arguments).Pack(args ...interface{}) ([]byte, error) (wrapcheck)
services/rfq/relayer/quoter/quoter.go
[failure] 452-452:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 453-453:
G115: integer overflow conversion int -> uint32 (gosec)
🔇 Additional comments (5)
services/rfq/api/model/request.go (1)
63-63: Verify the impact of nullable DestAmount
The DestAmount field has been made nullable (*string). This change could affect:
- Gas estimation calculations that depend on the destination amount
- Existing code that assumes this field is always present
Also applies to: 64-64
services/rfq/relayer/pricer/fee_pricer.go (3)
145-145: LGTM: Improved condition for fee calculation
The enhanced condition now properly checks both ZapNative and ZapData fields, ensuring more accurate fee calculation logic.
187-187: LGTM: Simplified ZapData validation
The condition has been simplified to a nil check, which is the correct approach as empty ZapData is still valid for processing.
236-240: LGTM: Improved error handling
The error handling for quote request encoding is comprehensive and provides clear context in the error messages.
services/rfq/relayer/quoter/quoter.go (1)
384-388: Refactoring enhances code clarity and reusability
By extracting the logic into the quoteDataToQuoteRequestV2 helper function, the generateActiveRFQ method becomes cleaner and more maintainable. This promotes code reusability and simplifies future modifications.
| return args.Pack( | ||
| uint16(2), // VERSION | ||
| tx.OriginChainId, | ||
| tx.DestChainId, | ||
| tx.OriginSender, | ||
| tx.DestRecipient, | ||
| tx.OriginToken, | ||
| tx.DestToken, | ||
| tx.OriginAmount, | ||
| tx.DestAmount, | ||
| tx.OriginFeeAmount, | ||
| tx.Deadline, | ||
| tx.Nonce, | ||
| tx.ExclusivityRelayer, | ||
| tx.ExclusivityEndTime, | ||
| tx.ZapNative, | ||
| tx.ZapData, | ||
| ) |
There was a problem hiding this comment.
Wrap error returned from external package
The error returned from args.Pack() should be wrapped with additional context.
Apply this change:
- return args.Pack(
+ packedData, err := args.Pack(
uint16(2), // VERSION
tx.OriginChainId,
tx.DestChainId,
tx.OriginSender,
tx.DestRecipient,
tx.OriginToken,
tx.DestToken,
tx.OriginAmount,
tx.DestAmount,
tx.OriginFeeAmount,
tx.Deadline,
tx.Nonce,
tx.ExclusivityRelayer,
tx.ExclusivityEndTime,
tx.ZapNative,
tx.ZapData,
)
+ if err != nil {
+ return nil, fmt.Errorf("failed to pack quote request: %w", err)
+ }
+ return packedData, 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.
| return args.Pack( | |
| uint16(2), // VERSION | |
| tx.OriginChainId, | |
| tx.DestChainId, | |
| tx.OriginSender, | |
| tx.DestRecipient, | |
| tx.OriginToken, | |
| tx.DestToken, | |
| tx.OriginAmount, | |
| tx.DestAmount, | |
| tx.OriginFeeAmount, | |
| tx.Deadline, | |
| tx.Nonce, | |
| tx.ExclusivityRelayer, | |
| tx.ExclusivityEndTime, | |
| tx.ZapNative, | |
| tx.ZapData, | |
| ) | |
| packedData, err := args.Pack( | |
| uint16(2), // VERSION | |
| tx.OriginChainId, | |
| tx.DestChainId, | |
| tx.OriginSender, | |
| tx.DestRecipient, | |
| tx.OriginToken, | |
| tx.DestToken, | |
| tx.OriginAmount, | |
| tx.DestAmount, | |
| tx.OriginFeeAmount, | |
| tx.Deadline, | |
| tx.Nonce, | |
| tx.ExclusivityRelayer, | |
| tx.ExclusivityEndTime, | |
| tx.ZapNative, | |
| tx.ZapData, | |
| ) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to pack quote request: %w", err) | |
| } | |
| return packedData, nil |
🧰 Tools
🪛 golangci-lint (1.62.2)
315-315: error returned from external package is unwrapped: sig: func (github.com/ethereum/go-ethereum/accounts/abi.Arguments).Pack(args ...interface{}) ([]byte, error)
(wrapcheck)
🪛 GitHub Check: Lint (services/rfq)
[failure] 315-315:
error returned from external package is unwrapped: sig: func (github.com/ethereum/go-ethereum/accounts/abi.Arguments).Pack(args ...interface{}) ([]byte, error) (wrapcheck)
| OriginChainId: uint32(quoteData.OriginChainID), | ||
| DestChainId: uint32(quoteData.DestChainID), |
There was a problem hiding this comment.
Potential integer overflow when converting int to uint32
Casting quoteData.OriginChainID and quoteData.DestChainID from int to uint32 without validation may cause integer overflows if the values are negative or exceed the uint32 maximum. This can lead to unexpected behavior or security vulnerabilities. Validate the values before casting to ensure they are within the valid uint32 range.
Apply this diff to add validation:
+ if quoteData.OriginChainID < 0 || quoteData.OriginChainID > math.MaxUint32 {
+ return nil, fmt.Errorf("OriginChainID out of range: %d", quoteData.OriginChainID)
+ }
+ if quoteData.DestChainID < 0 || quoteData.DestChainID > math.MaxUint32 {
+ return nil, fmt.Errorf("DestChainID out of range: %d", quoteData.DestChainID)
+ }
quoteRequest := &reldb.QuoteRequest{
Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{
OriginChainId: uint32(quoteData.OriginChainID),
DestChainId: uint32(quoteData.DestChainID),Alternatively, if OriginChainID and DestChainID are guaranteed to be non-negative and within uint32 limits, consider changing their types to uint32 in the QuoteData struct to prevent unnecessary casting and potential errors.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Lint (services/rfq)
[failure] 452-452:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 453-453:
G115: integer overflow conversion int -> uint32 (gosec)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
services/rfq/e2e/rfq_test.go (1)
777-831: Consider completing or removing the commented-out test function.The test function
TestEncodeBridgeTransactionParityis currently commented out. Keeping large blocks of commented code can clutter the codebase and reduce maintainability. If this test is intended to verify the encoding parity of bridge transactions, consider implementing and enabling it to enhance test coverage. Otherwise, if it's not needed, removing it would help keep the codebase clean.Would you like assistance in implementing this test function or opening a GitHub issue to track this task?
services/rfq/contracts/bridgetransactionv2/helper.go (2)
9-15: Fix typo in documentation and improve linter directive.There's a typo in the documentation and the linter directive could be more specific:
-// BridgeTransactionV2Ref is a bound fast bridge contract that returns the address of the contract. +// BridgeTransactionV2Ref is a bound fast bridge contract that returns the address of the contract. // -//nolint:golint +//nolint:golint // Reason: This type name is required to match the contract nameThe struct composition is well-designed, embedding the harness and adding the necessary address field.
17-20: Fix typo in Address method documentation.-// Address gets the ocntract address. +// Address gets the contract address.The method implementation is correct and follows Go conventions.
services/rfq/relayer/chain/encoding.go (2)
10-34: Improve comment style and documentation.The constants section is well-structured, but the comments should be enhanced for better documentation.
Apply these changes to improve the comments:
const ( - // Field sizes in bytes + // Field sizes in bytes. sizeVersion = 2 sizeChainID = 4 sizeAddress = 20 sizeUint256 = 32 - // Field offsets in bytes + // Field offsets in bytes.🧰 Tools
🪛 golangci-lint (1.62.2)
11-11: Comment should end in a period
(godot)
17-17: Comment should end in a period
(godot)
🪛 GitHub Check: Lint (services/rfq)
[failure] 11-11:
Comment should end in a period (godot)
[failure] 17-17:
Comment should end in a period (godot)
36-45: Enhance function documentation and consider error handling.While the implementation is correct, the documentation could be improved, and we might want to add bounds checking for extremely large numbers.
Apply these changes:
-// Helper function to properly encode uint256 +// padUint256 left-pads a big.Int to 32 bytes for uint256 encoding. +// If the input exceeds 32 bytes, it will be truncated. func padUint256(b *big.Int) []byte { + if b == nil { + b = new(big.Int) + } // Convert big.Int to bytes bytes := b.Bytes() + if len(bytes) > 32 { + return bytes[len(bytes)-32:] + } // Create 32-byte array (initialized to zeros) result := make([]byte, 32) // Copy bytes to right side of array (left-pad with zeros)🧰 Tools
🪛 golangci-lint (1.62.2)
36-36: Comment should end in a period
(godot)
🪛 GitHub Check: Lint (services/rfq)
[failure] 36-36:
Comment should end in a period (godot)services/rfq/testutil/deployers.go (2)
32-32: Consider improving readability with line breaksThe deployer list is getting quite long. Consider formatting it across multiple lines for better readability:
- parentManager := manager.NewDeployerManager(t, NewFastBridgeDeployer, NewMockERC20Deployer, NewMockFastBridgeDeployer, NewRecipientMockDeployer, NewBridgeTransactionV2Deployer, NewWETH9Deployer, NewUSDTDeployer, NewUSDCDeployer, NewDAIDeployer) + parentManager := manager.NewDeployerManager(t, + NewFastBridgeDeployer, + NewMockERC20Deployer, + NewMockFastBridgeDeployer, + NewRecipientMockDeployer, + NewBridgeTransactionV2Deployer, + NewWETH9Deployer, + NewUSDTDeployer, + NewUSDCDeployer, + NewDAIDeployer, + )
171-171: Fix incorrect documentation commentThe comment incorrectly states "deploys a mock recipient contract". It should describe the BridgeTransactionV2 deployment instead.
-// NewBridgeTransactionV2Deployer deploys a mock recipient contract. +// NewBridgeTransactionV2Deployer deploys a bridge transaction v2 contract.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
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(1 hunks)services/rfq/relayer/chain/encoding.go(1 hunks)services/rfq/relayer/pricer/fee_pricer.go(4 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/bridgetransactionv2.metadata.go
- services/rfq/contracts/bridgetransactionv2/generate.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
services/rfq/relayer/chain/encoding.go
11-11: Comment should end in a period
(godot)
17-17: Comment should end in a period
(godot)
36-36: Comment should end in a period
(godot)
80-80: append to slice result with non-zero initialized length
(makezero)
🪛 GitHub Check: Lint (services/rfq)
services/rfq/relayer/chain/encoding.go
[failure] 11-11:
Comment should end in a period (godot)
[failure] 17-17:
Comment should end in a period (godot)
[failure] 36-36:
Comment should end in a period (godot)
[failure] 80-80:
append to slice result with non-zero initialized length (makezero)
🔇 Additional comments (15)
services/rfq/testutil/contracttypeimpl_string.go (2)
22-22: Verify string representation consistency
The string representation data has been updated correctly to include the new type. The name string and index array are properly aligned, maintaining the format required by the stringer tool.
Also applies to: 24-24
15-19: Verify enum value declarations
The indices for the contract types have been shifted to accommodate the new BridgeTransactionV2Type. Please ensure that all corresponding enum value declarations in the main contract type file match these indices.
✅ Verification successful
Enum values match the generated stringer file
Looking at services/rfq/testutil/contracttype.go, the enum values are defined using iota + 1 with FastBridgeType as the first value. Following the sequence:
- FastBridgeType = 1
- MockERC20Type = 2
- FastBridgeMockType = 3
- RecipientMockType = 4
- BridgeTransactionV2Type = 5
- WETH9Type = 6
- USDTType = 7
- USDCType = 8 (implied in sequence)
- DAIType = 9 (implied in sequence)
These values perfectly match the indices in the stringer-generated file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the enum values in the main contract type file match these indices
# Search for the contract type declarations
ast-grep --pattern 'const (
$$$
BridgeTransactionV2Type contractTypeImpl = 5
WETH9Type contractTypeImpl = 6
USDTType contractTypeImpl = 7
USDCType contractTypeImpl = 8
DAIType contractTypeImpl = 9
$$$
)'
Length of output: 219
Script:
#!/bin/bash
# Let's try a different approach to find the contract type declarations
# First, let's find the relevant file containing the enum declarations
fd "contracttype.*\.go" --type f --exclude "*_string.go" --exec rg -l "type contractTypeImpl"
# Then, let's search for the actual enum constant declarations
rg "const \(" -A 15 $(fd "contracttype.*\.go" --type f --exclude "*_string.go")
Length of output: 9853
services/rfq/contracts/bridgetransactionv2/helper.go (3)
1-7: LGTM! Package and imports are well-structured.
The package name matches the directory name, and imports are appropriate for Ethereum contract interaction.
22-33: LGTM! Constructor is well-implemented.
The constructor follows Go best practices with proper error handling and clean initialization.
35-35: Verify ContractRef interface implementation.
The compile-time interface check is good practice. Let's verify all required methods are implemented.
✅ Verification successful
Based on the previous search results, I can generate a final response. The codebase shows that:
BridgeTransactionV2Refproperly implements thevm.ContractRefinterface through the compile-time check:var _ vm.ContractRef = &BridgeTransactionV2Ref{}- It has the required
Address()method implementation:
func (f *BridgeTransactionV2Ref) Address() common.Address {
return f.address
}- This follows the same pattern as numerous other contract references in the codebase that implement
vm.ContractRef
Interface implementation is correctly verified
The compile-time interface check is properly implemented and the required Address() method is present with the correct signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check vm.ContractRef interface requirements and implementation
# Search for ContractRef interface definition
rg -A 5 "type ContractRef interface"
# Search for Address method implementations to ensure consistency
ast-grep --pattern 'func ($x *BridgeTransactionV2Ref) Address() common.Address'
Length of output: 120
Script:
#!/bin/bash
# Let's try searching for ContractRef in a broader way
rg -l "ContractRef"
# Then let's look at the content of files that mention ContractRef
rg "ContractRef" -A 5 -B 5
# Also search for BridgeTransactionV2Ref implementation
rg "BridgeTransactionV2Ref" -A 5 -B 5
Length of output: 66515
Script:
#!/bin/bash
# Let's search for the vm.ContractRef interface definition
rg -p "type ContractRef interface" -A 5 -B 5 --glob '**/*.go'
# Also search for Address() implementation in BridgeTransactionV2Ref
ast-grep --pattern 'func ($x *BridgeTransactionV2Ref) Address() common.Address'
Length of output: 145
services/rfq/testutil/typecast.go (2)
9-9: LGTM: Import follows established patterns
The new import for bridgetransactionv2 is properly placed and follows the same pattern as other contract imports.
48-53: LGTM: Method implementation follows best practices
The new GetBridgeTransactionV2 method:
- Follows the established pattern of other getter methods
- Properly implements error handling via
t.Helper() - Uses the standard
manager.GetContractpattern - Has clear documentation
services/rfq/testutil/contracttype.go (2)
7-7: LGTM: Import statement is correctly placed
The new import for bridgetransactionv2 is properly organized with other contract imports.
106-107: Verify contract path and naming convention
A few points to verify:
- The contract path includes ".t.sol" which typically indicates a test contract. Confirm if this is intentional.
- The contract name "BridgeTransactionV2Lib" differs from the type name pattern. Other contracts typically don't use the "Lib" suffix.
✅ Verification successful
Contract path and naming are correct
The verification shows that:
- The contract path with ".t.sol" in the code refers to the test file, but the actual library is correctly defined in
packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol - The "Lib" suffix in
BridgeTransactionV2Libfollows the project's convention, as evidenced by numerous other library definitions (e.g., UniversalTokenLib, SwapQueryLib, ActionLib)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check contract file existence and naming pattern
fd -e sol "BridgeTransactionV2" | while read -r file; do
echo "Found contract file: $file"
# Check if it's a test file
if [[ $file == *.t.sol ]]; then
echo "Warning: File appears to be a test contract"
fi
# Check contract definition
rg "contract.*BridgeTransactionV2" "$file"
done
Length of output: 1450
Script:
#!/bin/bash
# Check the actual contract definition in BridgeTransactionV2.sol
rg "library.*BridgeTransactionV2|contract.*BridgeTransactionV2" "packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol"
# Check if there are any other non-test contracts with similar naming pattern
fd -e sol -E "*.t.sol" . | xargs rg "library.*Lib|contract.*Lib"
Length of output: 2823
services/rfq/testutil/deployers.go (2)
17-17: LGTM!
The import for bridgetransactionv2 is correctly placed and follows project conventions.
166-185: LGTM! Implementation follows established patterns
The BridgeTransactionV2Deployer implementation correctly follows the project's deployer pattern:
- Embeds BaseDeployer
- Provides constructor with required dependencies
- Implements Deploy method using DeploySimpleContract
services/rfq/relayer/pricer/fee_pricer.go (4)
18-18: LGTM!
The new import for the chain package is correctly placed and follows Go's import grouping conventions.
145-145: LGTM!
The condition has been improved to handle both ZapNative and ZapData fields, ensuring more accurate fee calculation for different transaction types.
187-187: LGTM!
The condition has been simplified to an idiomatic nil check, which is the preferred way in Go to check for optional fields.
236-241: LGTM! Verify the encoding implementation.
The code has been improved by:
- Moving the encoding logic to a dedicated function in the
chainpackage - Adding proper error handling with context
Let's verify the implementation of the encoding function:
✅ Verification successful
Implementation verified and looks robust
The encoding implementation in services/rfq/relayer/chain/encoding.go follows a well-structured approach:
- Uses proper binary encoding for chain IDs
- Correctly handles addresses and uint256 values
- Maintains version control (v2)
- Has test coverage in e2e tests
- Uses clear offset-based byte array construction
The implementation matches the contract's expected format and includes all necessary transaction fields: chain IDs, addresses, amounts, deadlines, and exclusivity parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of EncodeQuoteRequest function
# Check the implementation of EncodeQuoteRequest
ast-grep --pattern 'func EncodeQuoteRequest(tx *reldb.Transaction) ([]byte, error) {
$$$
}'
# Check for any test coverage
rg -l "TestEncodeQuoteRequest|Test_EncodeQuoteRequest" -g "**/*_test.go"
Length of output: 170
Script:
#!/bin/bash
# Let's try a broader search to locate the encoding function and its implementation
# Search for files containing EncodeQuoteRequest
rg -l "EncodeQuoteRequest"
# Search for the actual implementation with context
rg -A 10 "func.*EncodeQuoteRequest"
# Look for related test files
rg -l "EncodeQuote" -g "**/*_test.go"
Length of output: 1094
Script:
#!/bin/bash
# Let's examine the complete implementation and test coverage
# Get the full implementation of EncodeQuoteRequest
rg -B 5 -A 30 "func EncodeQuoteRequest" services/rfq/relayer/chain/encoding.go
# Check the test implementation
rg -B 5 -A 20 "EncodeQuoteRequest" services/rfq/e2e/rfq_test.go
# Check for any constants or types used in the encoding
rg "^const.*(offset|size)" services/rfq/relayer/chain/encoding.go
Length of output: 2172
| // EncodeQuoteRequest encodes a quote request into a byte array. | ||
| func EncodeQuoteRequest(tx fastbridgev2.IFastBridgeV2BridgeTransactionV2) ([]byte, error) { | ||
| result := make([]byte, offsetZapData) | ||
|
|
||
| // Version | ||
| result[offsetVersion] = 0 | ||
| result[offsetVersion+1] = 2 | ||
|
|
||
| // Chain IDs | ||
| binary.BigEndian.PutUint32(result[offsetOriginChainID:offsetOriginChainID+sizeChainID], tx.OriginChainId) | ||
| binary.BigEndian.PutUint32(result[offsetDestChainID:offsetDestChainID+sizeChainID], tx.DestChainId) | ||
|
|
||
| // Addresses | ||
| copy(result[offsetOriginSender:offsetOriginSender+sizeAddress], tx.OriginSender.Bytes()) | ||
| copy(result[offsetDestRecipient:offsetDestRecipient+sizeAddress], tx.DestRecipient.Bytes()) | ||
| copy(result[offsetOriginToken:offsetOriginToken+sizeAddress], tx.OriginToken.Bytes()) | ||
| copy(result[offsetDestToken:offsetDestToken+sizeAddress], tx.DestToken.Bytes()) | ||
|
|
||
| // uint256 values | ||
| copy(result[offsetOriginAmount:offsetOriginAmount+sizeUint256], padUint256(tx.OriginAmount)) | ||
| copy(result[offsetDestAmount:offsetDestAmount+sizeUint256], padUint256(tx.DestAmount)) | ||
| copy(result[offsetOriginFeeAmount:offsetOriginFeeAmount+sizeUint256], padUint256(tx.OriginFeeAmount)) | ||
| copy(result[offsetDeadline:offsetDeadline+sizeUint256], padUint256(tx.Deadline)) | ||
| copy(result[offsetNonce:offsetNonce+sizeUint256], padUint256(tx.Nonce)) | ||
|
|
||
| // Exclusivity address | ||
| copy(result[offsetExclusivityRelayer:offsetExclusivityRelayer+sizeAddress], tx.ExclusivityRelayer.Bytes()) | ||
|
|
||
| // More uint256 values | ||
| copy(result[offsetExclusivityEndTime:offsetExclusivityEndTime+sizeUint256], padUint256(tx.ExclusivityEndTime)) | ||
| copy(result[offsetZapNative:offsetZapNative+sizeUint256], padUint256(tx.ZapNative)) | ||
|
|
||
| // Append ZapData | ||
| result = append(result, tx.ZapData...) | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Add security validations for critical transaction fields.
The function should validate critical transaction fields to prevent potential security issues.
Consider adding these validations:
- Chain ID validation
- Address validation (non-zero)
- Amount validation (non-negative)
- Deadline validation (future timestamp)
Example implementation:
func EncodeQuoteRequest(tx fastbridgev2.IFastBridgeV2BridgeTransactionV2) ([]byte, error) {
if err := validateTransaction(tx); err != nil {
return nil, fmt.Errorf("invalid transaction: %w", err)
}
// ... rest of the function
}
func validateTransaction(tx fastbridgev2.IFastBridgeV2BridgeTransactionV2) error {
if tx.OriginChainId == 0 || tx.DestChainId == 0 {
return fmt.Errorf("invalid chain IDs")
}
if tx.OriginAmount != nil && tx.OriginAmount.Sign() < 0 {
return fmt.Errorf("negative origin amount")
}
// ... more validations
return nil
}🧰 Tools
🪛 golangci-lint (1.62.2)
80-80: append to slice result with non-zero initialized length
(makezero)
🪛 GitHub Check: Lint (services/rfq)
[failure] 80-80:
append to slice result with non-zero initialized length (makezero)
🛠️ Refactor suggestion
Add input validation and optimize slice operations.
The encoding logic is well-structured, but there are a few improvements that could be made:
- Add input validation
- Optimize the slice append operation
- Consider adding error handling for potential nil fields
Apply these changes:
func EncodeQuoteRequest(tx fastbridgev2.IFastBridgeV2BridgeTransactionV2) ([]byte, error) {
+ if tx == nil {
+ return nil, fmt.Errorf("transaction cannot be nil")
+ }
+
+ zapDataLen := len(tx.ZapData)
- result := make([]byte, offsetZapData)
+ result := make([]byte, offsetZapData+zapDataLen)
// Version
result[offsetVersion] = 0
@@ -77,7 +82,7 @@
copy(result[offsetZapNative:offsetZapNative+sizeUint256], padUint256(tx.ZapNative))
// Append ZapData
- result = append(result, tx.ZapData...)
+ copy(result[offsetZapData:], tx.ZapData)📝 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.
| // EncodeQuoteRequest encodes a quote request into a byte array. | |
| func EncodeQuoteRequest(tx fastbridgev2.IFastBridgeV2BridgeTransactionV2) ([]byte, error) { | |
| result := make([]byte, offsetZapData) | |
| // Version | |
| result[offsetVersion] = 0 | |
| result[offsetVersion+1] = 2 | |
| // Chain IDs | |
| binary.BigEndian.PutUint32(result[offsetOriginChainID:offsetOriginChainID+sizeChainID], tx.OriginChainId) | |
| binary.BigEndian.PutUint32(result[offsetDestChainID:offsetDestChainID+sizeChainID], tx.DestChainId) | |
| // Addresses | |
| copy(result[offsetOriginSender:offsetOriginSender+sizeAddress], tx.OriginSender.Bytes()) | |
| copy(result[offsetDestRecipient:offsetDestRecipient+sizeAddress], tx.DestRecipient.Bytes()) | |
| copy(result[offsetOriginToken:offsetOriginToken+sizeAddress], tx.OriginToken.Bytes()) | |
| copy(result[offsetDestToken:offsetDestToken+sizeAddress], tx.DestToken.Bytes()) | |
| // uint256 values | |
| copy(result[offsetOriginAmount:offsetOriginAmount+sizeUint256], padUint256(tx.OriginAmount)) | |
| copy(result[offsetDestAmount:offsetDestAmount+sizeUint256], padUint256(tx.DestAmount)) | |
| copy(result[offsetOriginFeeAmount:offsetOriginFeeAmount+sizeUint256], padUint256(tx.OriginFeeAmount)) | |
| copy(result[offsetDeadline:offsetDeadline+sizeUint256], padUint256(tx.Deadline)) | |
| copy(result[offsetNonce:offsetNonce+sizeUint256], padUint256(tx.Nonce)) | |
| // Exclusivity address | |
| copy(result[offsetExclusivityRelayer:offsetExclusivityRelayer+sizeAddress], tx.ExclusivityRelayer.Bytes()) | |
| // More uint256 values | |
| copy(result[offsetExclusivityEndTime:offsetExclusivityEndTime+sizeUint256], padUint256(tx.ExclusivityEndTime)) | |
| copy(result[offsetZapNative:offsetZapNative+sizeUint256], padUint256(tx.ZapNative)) | |
| // Append ZapData | |
| result = append(result, tx.ZapData...) | |
| return result, nil | |
| } | |
| // EncodeQuoteRequest encodes a quote request into a byte array. | |
| func EncodeQuoteRequest(tx fastbridgev2.IFastBridgeV2BridgeTransactionV2) ([]byte, error) { | |
| if tx == nil { | |
| return nil, fmt.Errorf("transaction cannot be nil") | |
| } | |
| zapDataLen := len(tx.ZapData) | |
| result := make([]byte, offsetZapData+zapDataLen) | |
| // Version | |
| result[offsetVersion] = 0 | |
| result[offsetVersion+1] = 2 | |
| // Chain IDs | |
| binary.BigEndian.PutUint32(result[offsetOriginChainID:offsetOriginChainID+sizeChainID], tx.OriginChainId) | |
| binary.BigEndian.PutUint32(result[offsetDestChainID:offsetDestChainID+sizeChainID], tx.DestChainId) | |
| // Addresses | |
| copy(result[offsetOriginSender:offsetOriginSender+sizeAddress], tx.OriginSender.Bytes()) | |
| copy(result[offsetDestRecipient:offsetDestRecipient+sizeAddress], tx.DestRecipient.Bytes()) | |
| copy(result[offsetOriginToken:offsetOriginToken+sizeAddress], tx.OriginToken.Bytes()) | |
| copy(result[offsetDestToken:offsetDestToken+sizeAddress], tx.DestToken.Bytes()) | |
| // uint256 values | |
| copy(result[offsetOriginAmount:offsetOriginAmount+sizeUint256], padUint256(tx.OriginAmount)) | |
| copy(result[offsetDestAmount:offsetDestAmount+sizeUint256], padUint256(tx.DestAmount)) | |
| copy(result[offsetOriginFeeAmount:offsetOriginFeeAmount+sizeUint256], padUint256(tx.OriginFeeAmount)) | |
| copy(result[offsetDeadline:offsetDeadline+sizeUint256], padUint256(tx.Deadline)) | |
| copy(result[offsetNonce:offsetNonce+sizeUint256], padUint256(tx.Nonce)) | |
| // Exclusivity address | |
| copy(result[offsetExclusivityRelayer:offsetExclusivityRelayer+sizeAddress], tx.ExclusivityRelayer.Bytes()) | |
| // More uint256 values | |
| copy(result[offsetExclusivityEndTime:offsetExclusivityEndTime+sizeUint256], padUint256(tx.ExclusivityEndTime)) | |
| copy(result[offsetZapNative:offsetZapNative+sizeUint256], padUint256(tx.ZapNative)) | |
| // Append ZapData | |
| copy(result[offsetZapData:], tx.ZapData) | |
| return result, nil | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
80-80: append to slice result with non-zero initialized length
(makezero)
🪛 GitHub Check: Lint (services/rfq)
[failure] 80-80:
append to slice result with non-zero initialized length (makezero)
| // BridgeTransactionV2Type is a bridge transaction contract for testing fast bridge interactions. | ||
| BridgeTransactionV2Type // BridgeTransactionV2 |
There was a problem hiding this comment.
💡 Codebase verification
Update stringer for the new BridgeTransactionV2Type contract type
The stringer generation is not up to date with the newly added contract type. Please run:
go generate ./...to update the stringer implementation and include the BridgeTransactionV2Type in the String() method.
🔗 Analysis chain
Verify stringer generation for the new contract type
The new contract type is properly defined and documented. Since this file uses stringer generation, ensure you run:
go generate ./...🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if stringer has been updated for the new type
rg -A 1 "func.*String.*contractTypeImpl" | grep -q "BridgeTransactionV2"
if [ $? -eq 0 ]; then
echo "Stringer is up to date"
else
echo "Stringer needs to be updated"
fi
Length of output: 157
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
services/rfq/e2e/rfq_test.go (2)
785-785: Ensure consistentDeadlinevalues in test structsIn
bridgeTx,Deadlineis set tobig.NewInt(0), while intx, it's set todeadline, which is a large value (2^256 - 1). This inconsistency might lead to mismatches in the encoded data.Consider using the same
deadlinevariable for both structs to maintain consistency:- Deadline: big.NewInt(0), + Deadline: deadline,This change should be applied to line 798 in
bridgeTx.Also applies to: 821-821
806-807: Remove debuggingfmt.Printfstatements from testsThe
fmt.Printfstatements used to print addresses and encoded values are unnecessary in committed test code and can clutter test output.Consider removing these lines:
- fmt.Printf("address: %v\n", handle.Address()) - fmt.Printf("Expected: %x\n", expectedEncoded)This will clean up the test output and focus on assert statements.
Also applies to: 809-810
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
services/rfq/contracts/bridgetransactionv2/generate.go(1 hunks)services/rfq/e2e/rfq_test.go(2 hunks)services/rfq/testutil/contracttype.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/rfq/contracts/bridgetransactionv2/generate.go
- services/rfq/testutil/contracttype.go
🧰 Additional context used
🪛 GitHub Check: Lint (services/rfq)
services/rfq/e2e/rfq_test.go
[failure] 789-789:
G115: integer overflow conversion uint -> uint32 (gosec)
[failure] 790-790:
G115: integer overflow conversion uint -> uint32 (gosec)
[failure] 812-812:
G115: integer overflow conversion uint -> uint32 (gosec)
🔇 Additional comments (1)
services/rfq/e2e/rfq_test.go (1)
801-801: Verify the ExclusivityRelayer address for consistency
In bridgeTx, ExclusivityRelayer is set to i.relayerWallet.Address(), whereas in tx, it's set to the zero address (common.HexToAddress("")). This difference could cause the parity test to fail due to mismatched encoded values.
Please confirm if setting ExclusivityRelayer to the zero address in tx is intentional. If not, update it to match bridgeTx:
- ExclusivityRelayer: common.HexToAddress(""),
+ ExclusivityRelayer: i.relayerWallet.Address(),This ensures both structs have the same ExclusivityRelayer value.
Also applies to: 824-824
services/rfq/e2e/rfq_test.go
Outdated
| OriginChainId: uint32(i.originBackend.GetChainID()), | ||
| DestChainId: uint32(i.destBackend.GetChainID()), |
There was a problem hiding this comment.
Fix potential integer overflow when converting chain IDs to uint32
Converting chain IDs directly to uint32 may lead to integer overflow if the chain ID exceeds the uint32 maximum value. This can cause unexpected behavior in the tests.
Apply this diff to safely handle the chain ID conversion:
- OriginChainId: uint32(i.originBackend.GetChainID()),
- DestChainId: uint32(i.destBackend.GetChainID()),
+ OriginChainId: uint32(i.originBackend.GetChainID().Uint64()),
+ DestChainId: uint32(i.destBackend.GetChainID().Uint64()),And similarly for the second instance:
- OriginChainId: uint32(i.originBackend.GetChainID()),
+ OriginChainId: uint32(i.originBackend.GetChainID().Uint64()),This ensures that the chain IDs are properly converted from *big.Int to uint32 without risking overflow.
Also applies to: 812-812
🧰 Tools
🪛 GitHub Check: Lint (services/rfq)
[failure] 789-789:
G115: integer overflow conversion uint -> uint32 (gosec)
[failure] 790-790:
G115: integer overflow conversion uint -> uint32 (gosec)
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)
440-440: Document the assumption about destination amount.The code assumes the destination amount equals the origin amount for estimation purposes. This assumption should be documented with a comment explaining the rationale.
- destAmount := originAmount // assume dest amount same as origin amount for estimation purposes + // For gas estimation purposes, we initially assume the destination amount + // equals the origin amount. This will be adjusted later with proper fees and offsets. + destAmount := originAmount
448-448: Extract magic number into a named constant.The deadline calculation uses a magic number (2^256 - 1). This should be extracted into a named constant for better maintainability.
+ // MaxUint256 represents the maximum value for uint256 (2^256 - 1) + const MaxUint256 = "115792089237316195423570985008687907853269984665640564039457584007913129639935" - deadline := new(big.Int).Sub(new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil), big.NewInt(1)) + deadline, _ := new(big.Int).SetString(MaxUint256, 10)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
packages/contracts-rfq/package.json(1 hunks)services/rfq/e2e/rfq_test.go(2 hunks)services/rfq/relayer/chain/encoding.go(1 hunks)services/rfq/relayer/quoter/quoter.go(2 hunks)
🔇 Additional comments (8)
services/rfq/relayer/quoter/quoter.go (2)
384-388: LGTM! Error handling follows best practices.
The error handling implementation properly wraps the error with context using fmt.Errorf, making it easier to trace issues in production.
453-454:
Add validation for chain ID conversions.
The conversion of chain IDs from int to uint32 could cause integer overflow. This issue was previously identified and should be addressed.
Apply this diff to add validation:
+ if quoteData.OriginChainID < 0 || quoteData.OriginChainID > math.MaxUint32 {
+ return nil, fmt.Errorf("OriginChainID out of range: %d", quoteData.OriginChainID)
+ }
+ if quoteData.DestChainID < 0 || quoteData.DestChainID > math.MaxUint32 {
+ return nil, fmt.Errorf("DestChainID out of range: %d", quoteData.DestChainID)
+ }services/rfq/relayer/chain/encoding.go (2)
80-81: Optimized ZapData appending by replacing append with copy.
The code now correctly uses copy to append ZapData directly into the preallocated byte slice, which is more efficient.
48-51:
Add input validation for EncodeQuoteRequest function.
Currently, the function does not validate the tx parameter. Adding input validation prevents potential nil pointer dereferences and ensures that critical fields are properly set before encoding.
Apply this diff to add input validation:
func EncodeQuoteRequest(tx fastbridgev2.IFastBridgeV2BridgeTransactionV2) ([]byte, error) {
+ if tx == nil {
+ return nil, fmt.Errorf("transaction cannot be nil")
+ }
+
// Initialize with total size including ZapData
result := make([]byte, offsetZapData+len(tx.ZapData))
// ... rest of the functionpackages/contracts-rfq/package.json (1)
29-29: Include test/harnesses directory in build:go script.
Updating the build:go script to include test/harnesses/*.sol ensures that all relevant Solidity files are processed during the build. This change is appropriate and maintains consistency.
services/rfq/e2e/rfq_test.go (3)
824-824: Verify the use of zero address for ExclusivityRelayer.
Assigning ExclusivityRelayer to the zero address by using common.HexToAddress("") may have unintended consequences if the contract logic treats the zero address differently. Ensure this is intentional or consider providing a valid address.
785-785: Ensure consistent Deadline values in the test.
Deadline is initially set to the maximum possible uint256 value, but in bridgeTx and tx, it is set to big.NewInt(0). Confirm that this inconsistency is intentional and that the Deadline is correctly set for the test cases.
Also applies to: 799-799, 822-822
790-791:
Prevent potential integer overflow when casting chain IDs to uint32.
Casting chain IDs directly to uint32 can cause integer overflow if the chain ID exceeds 4,294,967,295. Consider validating the chain IDs or using a larger data type to avoid potential errors.
Apply this diff to safely handle chain IDs:
- OriginChainId: uint32(i.originBackend.GetChainID()),
- DestChainId: uint32(i.destBackend.GetChainID()),
+ OriginChainId: i.originBackend.GetChainID().Uint64(),
+ DestChainId: i.destBackend.GetChainID().Uint64(),And in the second instance:
- OriginChainId: uint32(i.originBackend.GetChainID()),
+ OriginChainId: i.originBackend.GetChainID().Uint64(),Also applies to: 812-813
| OriginSender: common.HexToAddress(quoteData.OriginSender), | ||
| DestRecipient: common.HexToAddress(quoteData.DestRecipient), |
There was a problem hiding this comment.
Validate sender and recipient addresses.
The function should validate that the sender and recipient addresses are not zero addresses to prevent potential issues with transactions.
+ if quoteData.OriginSender == "" || common.HexToAddress(quoteData.OriginSender) == (common.Address{}) {
+ return nil, errors.New("invalid origin sender address")
+ }
+ if quoteData.DestRecipient == "" || common.HexToAddress(quoteData.DestRecipient) == (common.Address{}) {
+ return nil, errors.New("invalid destination recipient address")
+ }📝 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.
| OriginSender: common.HexToAddress(quoteData.OriginSender), | |
| DestRecipient: common.HexToAddress(quoteData.DestRecipient), | |
| if quoteData.OriginSender == "" || common.HexToAddress(quoteData.OriginSender) == (common.Address{}) { | |
| return nil, errors.New("invalid origin sender address") | |
| } | |
| if quoteData.DestRecipient == "" || common.HexToAddress(quoteData.DestRecipient) == (common.Address{}) { | |
| return nil, errors.New("invalid destination recipient address") | |
| } | |
| OriginSender: common.HexToAddress(quoteData.OriginSender), | |
| DestRecipient: common.HexToAddress(quoteData.DestRecipient), |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
services/rfq/testutil/deployers.go (2)
32-32: Consider improving readability by breaking down the long line.While the addition of NewBridgeTransactionV2Deployer is correct, the line is becoming quite long. Consider improving readability by splitting the deployers across multiple lines.
- parentManager := manager.NewDeployerManager(t, NewFastBridgeDeployer, NewMockERC20Deployer, NewMockFastBridgeDeployer, NewRecipientMockDeployer, NewBridgeTransactionV2Deployer, NewWETH9Deployer, NewUSDTDeployer, NewUSDCDeployer, NewDAIDeployer) + parentManager := manager.NewDeployerManager(t, + NewFastBridgeDeployer, + NewMockERC20Deployer, + NewMockFastBridgeDeployer, + NewRecipientMockDeployer, + NewBridgeTransactionV2Deployer, + NewWETH9Deployer, + NewUSDTDeployer, + NewUSDCDeployer, + NewDAIDeployer, + )
171-171: Fix incorrect documentation comment.The comment states "deploys a mock recipient contract" which appears to be a copy-paste error.
-// NewBridgeTransactionV2Deployer deploys a mock recipient contract. +// NewBridgeTransactionV2Deployer deploys a bridge transaction v2 contract.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
services/rfq/e2e/rfq_test.go(2 hunks)services/rfq/relayer/chain/encoding.go(1 hunks)services/rfq/relayer/pricer/fee_pricer.go(4 hunks)services/rfq/testutil/deployers.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
services/rfq/e2e/rfq_test.go
814-814: unnecessary conversion
(unconvert)
🪛 GitHub Check: Lint (services/rfq)
services/rfq/e2e/rfq_test.go
[failure] 814-814:
unnecessary conversion (unconvert)
🔇 Additional comments (3)
services/rfq/testutil/deployers.go (2)
17-17: LGTM!
The import for bridgetransactionv2 follows project conventions and is correctly placed with other contract imports.
180-182: Verify the use of Harness variant.
The deployment uses DeployBridgeTransactionV2Harness instead of DeployBridgeTransactionV2. Please verify if this is intentional and if the Harness variant is required for testing purposes.
✅ Verification successful
Let me analyze the results. The search shows that BridgeTransactionV2Harness is specifically used in test files and harness contracts:
- It's defined in
packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol - It's used in
packages/contracts-rfq/test/libs/BridgeTransactionV2.t.solfor testing - The harness contract exposes internal functions from
BridgeTransactionV2Libfor testing purposes
Let me verify if this is the only variant used in tests by checking for any other deployment patterns.
Based on the search results, I can now provide a definitive response. The code shows that:
BridgeTransactionV2Harnessis a test harness contract that exposes internal functions fromBridgeTransactionV2Libfor testing purposes- The harness is used in
packages/contracts-rfq/test/libs/BridgeTransactionV2.t.solfor testing the library functions - The only other variant is
BridgeTransactionV2Libwhich is the actual library being tested - The harness pattern is consistently used across the test files
The use of Harness variant is intentional and correct.
The Harness contract is specifically designed for testing purposes to expose the internal functions of the BridgeTransactionV2Lib library, which is a common testing pattern. The actual library's internal functions would not be accessible directly without this harness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Harness variant is used consistently in tests
# Search for other usages of Harness variants
rg -A 2 "DeployBridgeTransactionV2(Harness)?"
# Check if there are any tests specifically requiring the Harness variant
rg -A 5 "BridgeTransactionV2Harness"
Length of output: 72352
Script:
#!/bin/bash
# Check for any non-harness BridgeTransactionV2 deployments
rg -A 2 "DeployBridgeTransactionV2[^H]"
# Check if there are any test files using the non-harness variant
rg -A 2 "BridgeTransactionV2[^H].*new "
Length of output: 73543
services/rfq/relayer/chain/encoding.go (1)
50-86:
Add input validation to prevent nil pointer dereferences.
The EncodeBridgeTx function should validate the input transaction fields to ensure none are nil, preventing potential nil pointer dereferences and panics.
Apply this diff to add input validation:
func EncodeBridgeTx(tx fastbridgev2.IFastBridgeV2BridgeTransactionV2) ([]byte, error) {
+ if tx == (fastbridgev2.IFastBridgeV2BridgeTransactionV2{}) {
+ return nil, fmt.Errorf("transaction cannot be empty")
+ }
+ // Check required fields for nil
+ if tx.OriginAmount == nil || tx.DestAmount == nil || tx.OriginFeeAmount == nil || tx.Deadline == nil || tx.Nonce == nil || tx.ExclusivityEndTime == nil || tx.ZapNative == nil || tx.ZapData == nil {
+ return nil, fmt.Errorf("transaction contains nil fields")
+ }
// Initialize with total size including ZapData
result := make([]byte, offsetZapData+len(tx.ZapData))|
|
||
| encodedData, err := fastBridgeV2ABI.Pack(methodName, quoteRequest.RawRequest, f.relayerAddress) | ||
| rawRequest, err := chain.EncodeBridgeTx(quoteRequest.Transaction) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("could not encode quote data: %w", err) | ||
| } | ||
|
|
||
| encodedData, err := fastBridgeV2ABI.Pack(methodName, rawRequest, f.relayerAddress) |
There was a problem hiding this comment.
Fix potential data race on fastBridgeV2ABI during concurrent access.
The package-level variable fastBridgeV2ABI is accessed without synchronization, which can lead to data races if getZapGasEstimate is called concurrently. Use sync.Once or a mutex to ensure thread-safe initialization.
Apply this diff to fix the issue:
+var fastBridgeV2ABIOnce sync.Once
// Existing variable
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 {
+ fastBridgeV2ABIOnce.Do(func() {
parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI))
if err != nil {
- return 0, fmt.Errorf("could not parse ABI: %w", err)
+ panic(fmt.Sprintf("could not parse ABI: %v", err))
}
fastBridgeV2ABI = &parsedABI
+ })Committable suggestion skipped: line range outside the PR's diff.
services/rfq/e2e/rfq_test.go
Outdated
| for i := range zapDataSize { | ||
| zapData[i] = byte(gofakeit.Uint8()) | ||
| } |
There was a problem hiding this comment.
Fix the loop to correctly initialize zapData.
The loop for i := range zapDataSize is incorrect because zapDataSize is an integer. You should iterate over the zapData slice instead.
Apply this diff to fix the loop:
-for i := range zapDataSize {
+for i := range zapData {
zapData[i] = byte(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] = byte(gofakeit.Uint8()) | |
| } | |
| for i := range zapData { | |
| zapData[i] = byte(gofakeit.Uint8()) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
814-814: unnecessary conversion
(unconvert)
🪛 GitHub Check: Lint (services/rfq)
[failure] 814-814:
unnecessary conversion (unconvert)
…SLT-320] (#3299) * WIP: incorporate call params into fee * WIP: decompose getFee() * Feat: account for CallValue in getDestinationFee() * Fix: build * Feat: test call value and call param calculation in fee pricer * Feat: add context on request body in rpc fwd errs * Fix: zap estimate gas * Cleanup: move gas estimate into own func * Fix: quoter tests * Cleanup: lint * Cleanup: lint * Fix: tests * Cleanup: decompose func * Cleanup: lint * Fix: tests * Cleanup: lint * Feat: always use quote fee multiplier * WIP: abi encode pack relay() * Feat: pass full RawRequest for gas estimation * Cleanup: lint * Fix: pricer tests * Feat: ignore static l2 fee when incorporating call params * Fix: tests * Clarifying comment * Feat: add extra check for call param len * Attempt to fix flake * Cleanup: lint * Fix: build * feat(rfq-relayer): apply zap fee to dest amount for active quotes [SLT-465] (#3395) * Feat: set zap params in supply to fee pricer * Feat: adjust dest amount by fee for active quote * Feat: add TestGenerateActiveRFQ * Feat: add nonzero case * Cleanup: lint * fix(rfq-relayer): gas estimation for zaps (#3413) * WIP: use QuoteData instead of QuoteRequest * WIP: impl conversion * Feat: use QuoteRequest instead of QuoteData for encoding * WIP: another encoding impl * WIP: add bridgetransactionv2 * WIP: regenerate with running test * Working zap e2e * Cleanup: encoding * Cleanup: move to new encoding file * ABIgen from harness instead of lib * Cleanup: lint * Fix: CI build * Fix: deploy harness * Fix: rfq test * Feat: use gofakeit to mock * Cleanup: test structure * Replace: EncodeQuoteRequest -> EncodeBridgeTx * Feat: add Decode func and add to parity test * Fix: eth address mocks * Cleanup: lint * Cleanup: lint --------- Co-authored-by: parodime <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
These changes improve the robustness and clarity of the quote request and fee calculation functionalities.