feat(rfq-relayer): regenerate with updated fastbridgev2 and zap terminology [SLT-442] #3394
Conversation
|
Warning Rate limit exceeded@dwasse has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve a comprehensive refactoring of the Ethereum contract bindings within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="[config_reader] The configuration option Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/relayer-arb-call #3394 +/- ##
===============================================================
- Coverage 26.64515% 24.82483% -1.82033%
===============================================================
Files 214 208 -6
Lines 13631 13273 -358
Branches 82 82
===============================================================
- Hits 3632 3295 -337
+ Misses 9715 9692 -23
- Partials 284 286 +2
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: |
6dfb170
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://01aa3e63.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-relayer-arb-call-zap-me.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: 0
🧹 Outside diff range and nitpick comments (6)
services/rfq/relayer/chain/chain.go (1)
80-81: Update comments to reflect the new ZapNative conditionThe existing comment "Check to see if ETH should be sent to destination" doesn't accurately describe all the conditions for setting
gasAmount. Consider updating the comments to clearly document each case.// Check to see if ETH should be sent to destination if util.IsGasToken(request.Transaction.DestToken) { gasAmount = request.Transaction.DestAmount - } else if request.Transaction.ZapNative != nil { + } else if request.Transaction.ZapNative != nil { // Handle native token amount for Zap operations gasAmount = request.Transaction.ZapNativeservices/rfq/contracts/testcontracts/recipientmock/recipientmock.contractinfo.json (1)
1-1: Consider aligning Solidity version requirementsThere's a discrepancy between the source code pragma (
^0.8.4) and the actual compiler version used (0.8.20). While this works due to semantic versioning, it's better to align these versions to prevent potential compatibility issues.Consider updating the pragma to match the compiler version:
-pragma solidity ^0.8.4; +pragma solidity ^0.8.20;services/rfq/relayer/reldb/base/model.go (2)
81-82: Consider enhancing field documentationThe comments for the new fields could be more descriptive to better explain their purpose:
- What does the "native value" represent in the context of zap operations?
- What kind of data is expected in the ZapData byte array?
Also applies to: 94-95
242-243: LGTM! Consider caching pow10 calculationsThe conversion logic is correct and maintains symmetry with FromQuoteRequest. However, since math.Pow10 is used multiple times with the same values, consider caching these calculations to improve performance.
func (r RequestForQuote) ToQuoteRequest() (*reldb.QuoteRequest, error) { + // Cache pow10 calculations + originPow10 := big.NewInt(int64(math.Pow10(int(r.OriginTokenDecimals)))) + destPow10 := big.NewInt(int64(math.Pow10(int(r.DestTokenDecimals)))) + nativePow10 := big.NewInt(int64(math.Pow10(int(nativeTokenDecimals)))) + // ... existing code ... return &reldb.QuoteRequest{ // ... existing fields ... - OriginAmount: new(big.Int).Div(r.OriginAmount.BigInt(), big.NewInt(int64(math.Pow10(int(r.OriginTokenDecimals))))), - DestAmount: new(big.Int).Div(r.DestAmount.BigInt(), big.NewInt(int64(math.Pow10(int(r.DestTokenDecimals))))), - OriginFeeAmount: new(big.Int).Div(r.OriginFeeAmount.BigInt(), big.NewInt(int64(math.Pow10(int(r.OriginTokenDecimals))))), - ZapNative: new(big.Int).Div(r.ZapNative.BigInt(), big.NewInt(int64(math.Pow10(int(nativeTokenDecimals))))), + OriginAmount: new(big.Int).Div(r.OriginAmount.BigInt(), originPow10), + DestAmount: new(big.Int).Div(r.DestAmount.BigInt(), destPow10), + OriginFeeAmount: new(big.Int).Div(r.OriginFeeAmount.BigInt(), originPow10), + ZapNative: new(big.Int).Div(r.ZapNative.BigInt(), nativePow10), // ... remaining fields ... }, nil }services/rfq/contracts/testcontracts/recipientmock/recipientmock.abigen.go (1)
415-417: Consider using consistent parameter naming.The
zapfunction parameters are named differently across implementations:
- IZapRecipient uses descriptive names:
token,amount,zapData- RecipientMock uses generic names:
arg0,arg1,arg2Consider using consistent parameter names for better maintainability.
- func (_RecipientMock *RecipientMockTransactor) Zap(opts *bind.TransactOpts, arg0 common.Address, arg1 *big.Int, arg2 []byte) (*types.Transaction, error) { + func (_RecipientMock *RecipientMockTransactor) Zap(opts *bind.TransactOpts, token common.Address, amount *big.Int, zapData []byte) (*types.Transaction, error) {Also applies to: 422-424, 429-431
services/rfq/e2e/rfq_test.go (1)
478-479: Consider adding test assertions for Zap parameters.While the test includes the new
ZapDataandZapNativeparameters, it doesn't verify if these values are correctly processed by the recipient contract. Consider adding assertions to validate that the recipient contract correctly receives and processes these parameters.i.Eventually(func() bool { destUSDCBalance, err := destUSDCHandle.BalanceOf(&bind.CallOpts{Context: i.GetTestContext()}, destRecipient.Address()) i.NoError(err) - return destUSDCBalance.Cmp(big.NewInt(0)) > 0 + if destUSDCBalance.Cmp(big.NewInt(0)) <= 0 { + return false + } + + // Verify that the recipient received the correct zap parameters + lastZapData, err := destRecipient.LastZapData(&bind.CallOpts{Context: i.GetTestContext()}) + i.NoError(err) + if !bytes.Equal(lastZapData, []byte("Hello, world!")) { + return false + } + + lastZapNative, err := destRecipient.LastZapNative(&bind.CallOpts{Context: i.GetTestContext()}) + i.NoError(err) + return lastZapNative.Cmp(big.NewInt(1_337_420)) == 0 })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
services/rfq/contracts/testcontracts/recipientmock/recipientmock.abigen.go(3 hunks)services/rfq/contracts/testcontracts/recipientmock/recipientmock.contractinfo.json(1 hunks)services/rfq/e2e/rfq_test.go(2 hunks)services/rfq/relayer/chain/chain.go(1 hunks)services/rfq/relayer/reldb/base/model.go(4 hunks)
🔇 Additional comments (8)
services/rfq/relayer/chain/chain.go (1)
80-81: Consider reordering conditions for better maintainability
The current order mixes new (Zap) and legacy (TransactionV1) functionality. Consider grouping related conditions together and documenting the migration path.
Let's verify the usage of these fields across the codebase:
services/rfq/contracts/testcontracts/recipientmock/recipientmock.contractinfo.json (2)
1-1: Interface changes look good!
The renamed interface IZapRecipient with the new zap function is well-designed:
- Proper parameter typing and naming
- Payable modifier allows ETH transfers
- Clear bytes4 return type for selector-based verification
1-1: Implementation provides good test coverage
The RecipientMock implementation is well-structured for testing:
- Correctly implements the IZapRecipient interface
- Includes receive() for ETH handling
- Returns correct selector in zap()
- Smart addition of testRecipientMock() to exclude from coverage
services/rfq/relayer/reldb/base/model.go (1)
151-152: Consider adding validation for ZapData
The ZapData is assigned directly without any validation. Consider adding checks for:
- Maximum size limits to prevent excessive data storage
- Data format validation if specific encoding is expected
services/rfq/contracts/testcontracts/recipientmock/recipientmock.abigen.go (3)
32-46: LGTM: Contract metadata and ABI definitions are consistent.
The contract metadata correctly defines the zap function interface with appropriate parameters and payable modifier.
48-89: Verify the contract interface renaming impact.
The contract interface has been consistently renamed from IFastBridgeRecipient to IZapRecipient. Let's verify that all dependent contracts have been updated accordingly.
#!/bin/bash
# Search for any remaining references to the old interface name
rg "IFastBridgeRecipient" -l
# Search for potential interface implementations
ast-grep --pattern 'contract $_ is IZapRecipient'Line range hint 1-1: Note: This is an auto-generated file.
As indicated by the header comment, this file is auto-generated. Any manual changes should be made to the source contract instead of this file.
✅ Verification successful
Auto-generated file correctly reflects its source contract
The auto-generated file is properly derived from packages/contracts-rfq/test/mocks/RecipientMock.sol, which implements the IZapRecipient interface. The source contract and its generated bindings are consistently using the updated Zap terminology. No manual changes are needed in the generated file.
- Source location:
packages/contracts-rfq/test/mocks/RecipientMock.sol - Used in test files across rfq and core packages
- Implements
IZapRecipientinterface correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the source contract exists
fd -t f "RecipientMock.sol"
Length of output: 567
Script:
#!/bin/bash
# Check the content of the source contract to verify inheritance and interface usage
cat packages/contracts-rfq/test/mocks/RecipientMock.sol
# Also check for any other files that might be using this mock
rg "RecipientMock" -l
Length of output: 1597
services/rfq/e2e/rfq_test.go (1)
397-397: LGTM! Test name updated to reflect new terminology.
The test function has been appropriately renamed from TestArbitraryCall to TestZap to align with the updated FastBridgeV2 terminology.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/rfq/relayer/reldb/base/model_test.go (1)
39-40: Verify test coverage for new fieldsThe test initializes the new fields with zero values. Consider adding test cases with non-zero values to ensure proper handling of the new fields.
- ZapNative: big.NewInt(0), - ZapData: []byte{}, + ZapNative: big.NewInt(1000000), + ZapData: []byte{0x1, 0x2, 0x3},
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
services/rfq/relayer/relapi/server_test.go(1 hunks)services/rfq/relayer/reldb/base/model_test.go(1 hunks)
| ZapNative: big.NewInt(0), | ||
| ZapData: []byte{}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for new Zap fields
The test data initializes ZapNative to zero and ZapData to empty, which might not adequately exercise the new functionality. Consider adding test cases that cover:
- Non-zero
ZapNativevalues - Non-empty
ZapDatapayloads - Edge cases for both fields
Here's a suggested enhancement to the test data:
func (c *RelayerServerSuite) getTestQuoteRequest(status reldb.QuoteRequestStatus) reldb.QuoteRequest {
+ // Add test cases with different ZapNative and ZapData values
+ testCases := []struct {
+ zapNative *big.Int
+ zapData []byte
+ }{
+ {big.NewInt(0), []byte{}}, // Current case
+ {big.NewInt(1000), []byte{0x1, 0x2, 0x3}}, // Non-zero values
+ {big.NewInt(1e18), []byte("test payload")}, // Large number and string payload
+ }
// ... rest of the function
}Committable suggestion skipped: line range outside the PR's diff.
dwasse
force-pushed
the
feat/relayer-arb-call-zap-merge
branch
from
f409d18 to
6dfb170
Compare
Summary by CodeRabbit
New Features
zapfunction in theRecipientMockcontract, enhancing token transfer capabilities.ZapNativeandZapDatain the bridging process.Bug Fixes
ZapNativefield.Refactor
IZapRecipientinterface.RequestForQuotestructure to replaceCallValueandCallParamswithZapNativeandZapData.Tests
zapfunctionality.