feat(contracts-rfq): relayer exclusivity [SLT-187] #3202
Conversation
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3202 +/- ##
====================================================
+ Coverage 40.97804% 90.35088% +49.37284%
====================================================
Files 459 60 -399
Lines 25643 1254 -24389
Branches 343 150 -193
====================================================
- Hits 10508 1133 -9375
+ Misses 14383 116 -14267
+ Partials 752 5 -747
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: |
ad5bd73
|
| Status: | 🚫 Build failed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (9)
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (3)
7-7: LGTM! Consider adding parameters for more detailed error reporting.The
ExclusivityParamsIncorrecterror is a good addition for handling issues related to exclusivity parameters. It aligns well with the PR objective of introducing relayer exclusivity.To provide more context when this error is thrown, consider adding parameters to specify which exclusivity parameter(s) are incorrect. For example:
error ExclusivityParamsIncorrect(string paramName, uint256 providedValue, uint256 expectedValue);This would allow for more detailed error reporting and easier debugging.
18-18: LGTM! Consider adding a timestamp parameter for consistency.The
ExclusivityPeriodNotPassederror is a valuable addition for handling timing issues related to the exclusivity feature. Its placement near other time-related errors (DeadlineExceeded,DisputePeriodNotPassed, etc.) is appropriate.For consistency with other time-related errors and to provide more context, consider adding a timestamp parameter:
error ExclusivityPeriodNotPassed(uint256 currentTime, uint256 exclusivityEndTime);This would align with the pattern used in errors like
DeadlineExceededand provide useful information for debugging.
Line range hint
7-18: Summary: Good additions for relayer exclusivity error handling.The two new errors,
ExclusivityParamsIncorrectandExclusivityPeriodNotPassed, are well-aligned with the PR objective of introducing relayer exclusivity. They enhance the contract's ability to handle specific error cases related to this new feature.The additions maintain the existing style and structure of the interface, making them a seamless integration. These focused changes improve error reporting capabilities without introducing risks to existing functionality.
As the relayer exclusivity feature is implemented, ensure that these errors are used consistently throughout the relevant contracts. Consider creating unit tests that specifically trigger these error conditions to verify their correct usage.
packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (3)
9-11: Approved: Good explanation for making the contract abstract.The change to make
FastBridgeV2ParityTestan abstract contract is well-justified by the comment explaining thatFastBridgeV2is no longer fully backwards compatible withFastBridge. This prevents these tests from running, which is appropriate given the compatibility issues.Consider adding more detailed documentation about the specific incompatibilities between
FastBridgeV2andFastBridge. This could help developers understand the exact nature of the breaking changes and guide them in updating any dependent code.
Line range hint
22-41: Changes to test functions reflect updated contract behavior.The modifications to the test functions provide valuable insights into the changes made in the
FastBridgeV2contract:
test_failedRelayNotRelayeris skipped, indicating that the relay function is no longer permissioned.test_failedClaimNotRelayeris skipped, suggesting that the claim function is no longer permissioned by role.test_failedClaimNotOldRelayeris modified to remove role assignment, but still tests for sender correctness.These changes are consistent with the comment about backwards incompatibility and provide a clear picture of how the permission model has evolved in
FastBridgeV2.Consider adding new test cases that specifically target the new behavior of the
FastBridgeV2contract, especially around the updated permission model. This will ensure that the new functionality is properly tested and documented.
Line range hint
1-41: Summary: Appropriate updates to reflect FastBridgeV2 changesThe modifications to
FastBridgeV2ParityTesteffectively address the backwards incompatibility issues betweenFastBridgeV2andFastBridge:
- Making the contract abstract prevents unintended test execution.
- Skipping and modifying specific tests reflect changes in the permission model.
- Comments provide clear explanations for the changes.
These updates ensure that the test suite accurately represents the current state of the
FastBridgeV2contract.To further improve the codebase:
- Consider creating a new test suite specifically for
FastBridgeV2that covers its unique features and behavior.- Update the main
FastBridgeV2contract documentation to clearly outline the breaking changes and new features compared toFastBridge.- If not already done, ensure that migration guides are available for users of the original
FastBridgecontract.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
29-29: Address the TODO to optimize the encoding schemeThere's a TODO comment suggesting a change to the encoding scheme to prevent extra gas consumption during decoding. Optimizing the encoding could lead to significant gas savings and improve contract efficiency.
Would you like me to propose an optimized encoding scheme or open a GitHub issue to track this task?
packages/contracts-rfq/test/FastBridgeV2.t.sol (2)
36-37: Restrict visibility of state variables for better encapsulationThe state variables
tokenTxandethTxare declared aspublic. If external access to these variables is not required, consider changing their visibility tointernalorprivateto enhance encapsulation and reduce potential security risks.
41-42: Align visibility of new parameters with best practicesSimilarly,
tokenParamsV2andethParamsV2are declared aspublic. If these parameters are only used within the contract or by inheriting contracts, consider setting their visibility tointernalorprivatefor consistency and improved security.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (5 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (4 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ProtocolFees.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (17 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (6 hunks)
🔇 Additional comments (69)
packages/contracts-rfq/test/FastBridgeV2.Src.ProtocolFees.t.sol (3)
Line range hint
1-24: Summary: Transaction structure refactoring implemented, awaiting further contextThe changes in this file introduce a new
txV1structure for organizing transaction data in the test contract. While the modifications are consistent and don't alter the test logic, they suggest a broader refactoring effort in the main contracts.Key points:
- The changes are well-implemented and consistent across both token and ETH transactions.
- The overall behavior of the test contract remains unchanged.
- More context is needed to understand the full scope of these changes, particularly their relation to the "relayer exclusivity" feature mentioned in the PR title.
Next steps:
- Provide additional context on the
txV1structure and its purpose.- Clarify how these changes contribute to the relayer exclusivity feature.
- Ensure consistency across the entire codebase and update documentation as necessary.
Once these points are addressed, the changes can be approved from a technical standpoint.
16-22: Refactoring of transaction structure noted. Verify consistency across the codebase.The changes introduce a new
txV1nested structure for bothtokenTxandethTxobjects. This refactoring suggests an update to the transaction data structure, possibly to accommodate new features or improve organization.While the test logic remains unchanged, this modification reflects changes in the main contract being tested. To ensure system-wide consistency:
- Verify that all other test files and main contract files have been updated to use this new structure.
- Check if documentation needs to be updated to reflect this change in transaction structure.
- Consider adding a comment explaining the purpose of the
txV1structure for better code readability.To ensure consistency across the codebase, run the following script:
16-22: Request for additional context on txV1 structure and relayer exclusivityThe introduction of the
txV1structure appears to be part of a larger refactoring effort. While these changes improve the organization of transaction data, a few points need clarification:
- Could you provide more context on the purpose of the
txV1structure? Is this related to a new version of the transaction format?- Are there any implications for backwards compatibility with existing systems or contracts?
- The PR title mentions "relayer exclusivity", but these changes don't seem directly related to that feature. Could you explain how these modifications contribute to the relayer exclusivity feature?
Adding comments in the code or updating the PR description with this information would greatly help in understanding the full scope and impact of these changes.
To gather more context, let's check for any related changes or documentation:
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (5)
8-8: Definition ofEXCLUSIVITY_PERIODEnhances ReadabilityThe use of
60 secondsforEXCLUSIVITY_PERIODimproves code readability by explicitly specifying the time unit.
10-22:createFixturesV2Correctly Initializes Exclusivity ParametersThe
createFixturesV2function properly overrides the base function and initializes the exclusivity parameters for both token and ETH transactions. AssigningquoteRelayer,quoteExclusivitySeconds, andquoteIdensures that the tests accurately reflect the intended behavior.
24-27: OverloadedbridgeFunction Appropriately Selects ParametersThe overloaded
bridgefunction effectively determines the correct set of parameters (ethParamsV2ortokenParamsV2) based on theoriginToken. This ensures that the bridging logic uses the appropriate configuration for each token type.
29-39: Maintains Caller Context Usingvm.prankUtilizing
vm.prank(caller)preserves the caller's context during thefastBridge.bridgeinvocation. This is essential for accurately simulating transaction conditions and testing access control within the contract.
41-63: Comprehensive Revert Tests for Exclusivity ParametersThe test functions:
test_bridge_revert_quoteRelayerSet_exclusivityPeriodNotSettest_bridge_revert_quoteRelayerNotSet_exclusivityPeriodSettest_bridge_eth_revert_quoteRelayerSet_exclusivityPeriodNotSettest_bridge_eth_revert_quoteRelayerNotSet_exclusivityPeriodSetcorrectly verify that the contract reverts when exclusivity parameters are incorrectly configured. This ensures the contract enforces the requirement that both
quoteRelayerandquoteExclusivitySecondsmust be set appropriately.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2)
42-42: Verify backward compatibility of the updatedbridgefunctionThe
bridgefunction signature now includes an additional parameterBridgeParamsV2 memory paramsV2. Please ensure that this change maintains backward compatibility with existing contracts and systems that implement this interface.To confirm, you might review dependent contracts for compatibility.
64-66: LGTM: New functiongetBridgeTransactionV2addedThe addition of the
getBridgeTransactionV2function enhances the interface by allowing decoding of bridge requests intoBridgeTransactionV2structs effectively.packages/contracts-rfq/test/FastBridgeV2.t.sol (4)
5-5: Addition ofIFastBridgeV2import is appropriateThe new import statement correctly includes the
IFastBridgeV2interface, which is necessary for integrating the updated bridge functionality into the test contract.
48-48: EnsurecreateFixturesV2()initializes all necessary fieldsAfter invoking
createFixturesV2(), verify that all the required fields intokenTxandethTxare properly initialized. This is crucial to prevent uninitialized data from affecting the outcome of the tests.
Line range hint
84-99: Confirm correct assignment totxV1intokenTxandethTxAssigning
IFastBridge.BridgeTransactiontotokenTx.txV1andethTx.txV1should be compatible with theBridgeTransactionV2struct. Ensure that thetxV1field exists withinBridgeTransactionV2and that this assignment aligns with the updated contract interfaces to prevent type mismatches.
128-130: Verification of transaction ID hashing methodThe
getTxIdfunction correctly computes the transaction ID usingkeccak256(abi.encode(bridgeTx)). Ensure that this hashing method is consistent with the one used in production contracts to maintain integrity between the tests and the actual implementation.packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (21)
1-7: LGTM!License header, pragma statement, imports, and contract declaration are correctly implemented.
8-26: Correct initialization in 'createFixturesV2'.The
createFixturesV2function properly initializes the bridge parameters and transaction data with the appropriate relayers and exclusivity periods.
32-35: Test 'test_relay_token_exclusivityLastSecond' is correctly implemented.Function correctly advances the time to the end of the exclusivity period and calls
test_relay_tokento verify expected behavior.
37-40: Test 'test_relay_token_exclusivityOver' is correctly implemented.Function correctly advances time beyond the exclusivity period and calls
test_relay_tokento ensure proper functionality.
42-45: Revert test 'test_relay_token_notQuotedRelayer_revert' is correctly implemented.Function correctly expects a revert when a non-quoted relayer attempts to relay during the exclusivity period.
47-51: Revert test 'test_relay_token_notQuotedRelayer_exclusivityLastSecond_revert' is correctly implemented.Function skips to the end of the exclusivity period and correctly expects a revert when a non-quoted relayer attempts to relay.
53-62: Test 'test_relay_token_notQuotedRelayer_exclusivityOver' is correctly implemented.Function correctly simulates a relay after the exclusivity period has passed, ensuring that a non-quoted relayer can relay successfully and asserts the correct balances.
64-67: Test 'test_relay_token_withRelayerAddress_exclusivityLastSecond' is correctly implemented.Function advances time to the end of the exclusivity period and tests relay with a specified relayer address.
69-72: Test 'test_relay_token_withRelayerAddress_exclusivityOver' is correctly implemented.Function advances time beyond the exclusivity period and tests relay with a specified relayer address, ensuring correct behavior.
74-77: Revert test 'test_relay_token_withRelayerAddress_notQuotedRelayer_revert' is correctly implemented.Function correctly expects a revert when a non-quoted relayer attempts to relay with a specified relayer address during the exclusivity period.
79-83: Revert test 'test_relay_token_withRelayerAddress_notQuotedRelayer_exclusivityLastSecond_revert' is correctly implemented.Function skips to the end of the exclusivity period and correctly expects a revert when a non-quoted relayer attempts to relay with a specified relayer address.
96-99: Comment section for ETH relay tests is appropriate.Comments clearly indicate the upcoming tests for ETH relays and the exclusivity rights of Relayer B.
100-103: Test 'test_relay_eth_exclusivityLastSecond' is correctly implemented.Function advances time to the end of the exclusivity period and calls
test_relay_ethto verify expected behavior.
105-108: Test 'test_relay_eth_exclusivityOver' is correctly implemented.Function advances time beyond the exclusivity period and calls
test_relay_ethto ensure correct functionality.
110-113: Revert test 'test_relay_eth_notQuotedRelayer_revert' is correctly implemented.Function correctly expects a revert when a non-quoted relayer attempts to relay ETH during the exclusivity period.
115-119: Revert test 'test_relay_eth_notQuotedRelayer_exclusivityLastSecond_revert' is correctly implemented.Function skips to the end of the exclusivity period and correctly expects a revert when a non-quoted relayer attempts to relay ETH.
121-130: Test 'test_relay_eth_notQuotedRelayer_exclusivityOver' is correctly implemented.Function correctly simulates a relay of ETH after the exclusivity period has passed, ensuring that a non-quoted relayer can relay successfully and asserts the correct balances.
132-135: Test 'test_relay_eth_withRelayerAddress_exclusivityLastSecond' is correctly implemented.Function advances time to the end of the exclusivity period and tests ETH relay with a specified relayer address.
137-140: Test 'test_relay_eth_withRelayerAddress_exclusivityOver' is correctly implemented.Function advances time beyond the exclusivity period and tests ETH relay with a specified relayer address, ensuring correct behavior.
142-145: Revert test 'test_relay_eth_withRelayerAddress_notQuotedRelayer_revert' is correctly implemented.Function correctly expects a revert when a non-quoted relayer attempts to relay ETH with a specified relayer address during the exclusivity period.
147-151: Revert test 'test_relay_eth_withRelayerAddress_notQuotedRelayer_exclusivityLastSecond_revert' is correctly implemented.Function skips to the end of the exclusivity period and correctly expects a revert when a non-quoted relayer attempts to relay ETH with a specified relayer address.
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (12)
4-4: Update import statement to useIFastBridgeV2The import statement now includes
IFastBridgeV2, reflecting the transition to the updated interface. This ensures compatibility with the new contract changes.
32-33: Mint tokens for bothrelayerAandrelayerBinmintTokensBy minting tokens for both relayers, you enhance the test coverage to include scenarios involving multiple relayers. This is a good practice for comprehensive testing.
38-39: Approve tokens forrelayerBAdding an approval for
relayerBensures thatfastBridgecan spend tokens on behalf of both relayers, aligning with the tokens minted earlier.
42-48: UpdateexpectBridgeRelayedfunction to acceptBridgeTransactionV2The function signature now accepts
IFastBridgeV2.BridgeTransactionV2, matching the updated interface. This change is consistent and necessary for the new transaction structure.
63-63: Updaterelayfunction to useBridgeTransactionV2The
relayfunction now acceptsIFastBridgeV2.BridgeTransactionV2, aligning it with the new interface. This update ensures that the function handles the new transaction data structure.
73-73: UpdaterelayWithAddressfunction to useBridgeTransactionV2Similarly,
relayWithAddressnow acceptsBridgeTransactionV2. This change is consistent with the interface updates and maintains compatibility.
82-88: IntroducecheckRelayedViewsfor enhanced test assertionsThe new
checkRelayedViewsfunction encapsulates checks for relayed transactions, improving modularity and readability of your tests.
93-97: Enhancetest_relay_tokenwith additional checksIncluding
checkRelayedViewsand balance assertions strengthens the test by verifying that the transaction was relayed correctly and that balances are updated as expected.
134-149: Test block data recording intest_relay_eth_withRelayerAddress_checkBlockDataThe test accurately sets custom block numbers and timestamps, then verifies that
fastBridgerecords them correctly. This adds valuable coverage for block data tracking in your tests.
123-130:⚠️ Potential issueVerify
callerandrelayerhandling in ETH relay testsIn
test_relay_eth_withRelayerAddress,relayerAcallsrelayWithAddresswithrelayerBas the relayer. Ensure that this behavior is intended and that the contract properly handles scenarios where the caller and relayer are different.Investigate if there are checks in place to prevent misuse when the caller specifies a different relayer. Use the script provided in the previous comment to assist in this verification.
104-108:⚠️ Potential issuePotential issue with mismatched
callerandrelayerinrelayWithAddressIn
test_relay_token_withRelayerAddress,relayerBcallsrelayWithAddressusingrelayerAas the relayer. This allows one relayer to relay a transaction and attribute it to another relayer. Verify if the protocol permits this behavior and assess potential security implications, such as misattribution or unauthorized relaying.Run the following script to examine how
relayWithAddresshandles therelayerparameter and check for verification mechanisms:#!/bin/bash # Description: Check for validation in relayWithAddress regarding caller and relayer # Search for the relayWithAddress function and look for any authorization checks ast-grep --lang solidity --pattern $'function relay(bytes memory request, address relayer) public {\n $$$\n}' \ packages/contracts-rfq/contracts/* | rg --after-context=10 'require|msg.sender|validation'
53-58: Access transaction details usingbridgeTx.txV1You're accessing properties through
bridgeTx.txV1. Verify thattxV1is correctly defined withinBridgeTransactionV2and that all the accessed fields are properly mapped.Run the following script to confirm that
BridgeTransactionV2containstxV1with the expected fields:✅ Verification successful
BridgeTransactionV2 correctly includes
txV1as a member.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of BridgeTransactionV2 and ensure it contains txV1 # Search for the definition of BridgeTransactionV2 and check for txV1 ast-grep --lang solidity --pattern $'struct BridgeTransactionV2 {\n $$$\n BridgeTransaction txV1;\n $$$\n}' \ packages/contracts-rfq/contracts/* # Expectation: The output should show that txV1 is a member of BridgeTransactionV2Length of output: 651
Script:
#!/bin/bash # Description: Verify that BridgeTransactionV2 contains txV1 using ripgrep # Search for the BridgeTransactionV2 struct definition rg 'struct\s+BridgeTransactionV2\s*\{([^}]*)\}' packages/contracts-rfq/contracts/*.sol # Within the struct definition, search for txV1 rg 'struct\s+BridgeTransactionV2\s*\{([^}]*)\}' packages/contracts-rfq/contracts/*.sol | rg 'BridgeTransaction\s+txV1;'Length of output: 414
Script:
#!/bin/bash # Description: Locate the definition of BridgeTransactionV2 in Solidity files # Search for the definition of the struct BridgeTransactionV2 rg 'struct\s+BridgeTransactionV2\s*\{' packages/contracts-rfq/contracts/**/*.sol # If BridgeTransactionV2 is found, display its contents rg -A 10 'struct\s+BridgeTransactionV2\s*\{' packages/contracts-rfq/contracts/**/*.solLength of output: 2270
packages/contracts-rfq/contracts/FastBridgeV2.sol (7)
78-81: Addition ofgetBridgeTransactionV2function is appropriateThe new
getBridgeTransactionV2function correctly decodes aBridgeTransactionV2from the providedbytesrequest. This addition aligns with the introduction of the new transaction structure and maintains code clarity.
85-88: Ensures backward compatibility by overloadingbridgefunctionThe
bridgefunction is appropriately overloaded to accept the newBridgeParamsV2. By initializing default values forBridgeParamsV2when they are not provided, it preserves backward compatibility with existing implementations.
100-103: Validation of exclusivity parameters is correctly implementedThe condition ensures that
quoteRelayerandquoteExclusivitySecondsare either both set or both unset, preventing inconsistent exclusivity configurations. This validation is crucial for the correct operation of exclusivity logic.
114-116: Calculation ofexclusivityEndTimeis appropriateThe calculation correctly handles cases where exclusivity parameters are not set by defaulting
exclusivityEndTimeto zero. This ensures that the exclusivity period is only applied when specified.
119-135: Proper encoding ofBridgeTransactionV2with nestedtxV1The encapsulation of
BridgeTransactionastxV1withinBridgeTransactionV2effectively extends the existing transaction structure to include exclusivity parameters. This approach maintains compatibility while adding new functionality.
169-176: Exclusivity checks inrelayfunction are correctly implementedThe added logic in the
relayfunction enforces the exclusivity period effectively. It ensures that during the exclusivity window, only the specifiedexclusivityRelayercan relay the transaction, thereby honoring exclusivity agreements.
152-152: EnsureBridgeRequestedV2event is defined and documentedThe
emit BridgeRequestedV2statement emits a new event withtransactionIdandparamsV2.quoteId. Verify that theBridgeRequestedV2event is properly defined and documented in the contract to avoid any issues during event handling.Run the following script to confirm that the
BridgeRequestedV2event is defined:packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (15)
4-4: Import statement updated correctlyThe addition of
IFastBridgeV2ensures access to the new interface required for updated function signatures.
30-30: Addition ofBridgeRequestedV2event is appropriateThe new event
BridgeRequestedV2correctly capturestransactionIdandquoteId. SincequoteIdis of typebytes, it cannot be indexed, which is acceptable for variable-length data.
75-77: Explicit visibility and mutability specifiers addedThe
bridgefunction now explicitly declares its visibility aspublicand is markedvirtualto allow overriding in derived contracts, enhancing extensibility.
85-87: Updatedprovefunction to useBridgeTransactionV2The function signature has been updated to accept
IFastBridgeV2.BridgeTransactionV2, aligning with the new interface and ensuring compatibility with the updated bridge transaction structure.
90-92: Updatedclaimfunction to useBridgeTransactionV2The function now accepts
IFastBridgeV2.BridgeTransactionV2, maintaining consistency with the updated interface.
95-97: Updated overloadedclaimfunction to useBridgeTransactionV2The overloaded
claimfunction is appropriately updated to acceptIFastBridgeV2.BridgeTransactionV2, ensuring consistency across function implementations.
105-107: Updatedrefundfunction to useBridgeTransactionV2The
refundfunction signature now acceptsIFastBridgeV2.BridgeTransactionV2, aligning with the updated transaction structure for refunds.
110-121: UpdatedexpectBridgeRequestedto use nestedtxV1fields inBridgeTransactionV2The function correctly adapts to the new
BridgeTransactionV2structure by accessing the required fields from the nestedtxV1member to emit theBridgeRequestedevent.
148-149: UpdatedexpectBridgeDepositClaimedto usebridgeTx.txV1fieldsThe function correctly retrieves
originTokenandoriginAmountfrombridgeTx.txV1, aligning with the updatedBridgeTransactionV2structure.
499-502: Updated balance assertions to usetokenTx.txV1fieldsThe assertions now reference
tokenTx.txV1.originFeeAmountandtokenTx.txV1.originAmount, reflecting the updated transaction structure.
565-568: Updated ETH balance assertions to useethTx.txV1fieldsThe assertions correctly reference
ethTx.txV1.originFeeAmountandethTx.txV1.originAmount, ensuring consistency with the new transaction structure.
864-864: Verify usage ofethParamsinstead ofethParamsV2The assertion uses
ethParams.originAmount. Given the update toBridgeTransactionV2, please verify thatethParamscontains values consistent with the new transaction structure. If necessary, update toethParamsV2to ensure accuracy.
878-879: Potential inconsistency in balance calculationsThe balance assertions use
ethParams.originAmount. Verify that these values are accurate in light of the updated transaction parameters, and ensure consistent usage ofethParamsV2if appropriate.
892-892: Verify usage ofethParamsinstead ofethParamsV2The assertion references
ethParams.originAmount. Confirm thatethParamsis aligned with the updated transaction structure, and update toethParamsV2if necessary.
906-906: Verify usage ofethParamsinstead ofethParamsV2In this assertion,
ethParams.originAmountis used. Please ensure thatethParamsreflects the values from the updatedBridgeTransactionV2structure. Consider usingethParamsV2if needed.
Bundle ReportChanges will increase total bundle size by 407.1kB (1.14%) ⬆️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
|
Gas diffs will be much closer to the true values once #3204 is merged |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function bridge(BridgeParams memory params, BridgeParamsV2 memory paramsV2) public payable { | ||
| // check bridge params | ||
| if (params.dstChainId == block.chainid) revert ChainIncorrect(); | ||
| if (params.originAmount == 0 || params.destAmount == 0) revert AmountIncorrect(); | ||
| if (params.sender == address(0) || params.to == address(0)) revert ZeroAddress(); | ||
| if (params.originToken == address(0) || params.destToken == address(0)) revert ZeroAddress(); | ||
| if (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); | ||
| // quoteRelayer and quoteExclusivitySeconds must be both set or both not set | ||
| if ((paramsV2.quoteRelayer == address(0)) != (paramsV2.quoteExclusivitySeconds == 0)) { | ||
| revert ExclusivityParamsIncorrect(); | ||
| } | ||
|
|
||
| // transfer tokens to bridge contract | ||
| // @dev use returned originAmount in request in case of transfer fees | ||
| uint256 originAmount = _pullToken(address(this), params.originToken, params.originAmount); | ||
|
|
||
| // track amount of origin token owed to protocol | ||
| uint256 originFeeAmount; | ||
| if (protocolFeeRate > 0) originFeeAmount = (originAmount * protocolFeeRate) / FEE_BPS; | ||
| originAmount -= originFeeAmount; // remove from amount used in request as not relevant for relayers | ||
|
|
||
| // Calculate exclusivity end time only if exclusivity params are set | ||
| uint256 exclusivityEndTime = | ||
| paramsV2.quoteExclusivitySeconds == 0 ? 0 : block.timestamp + paramsV2.quoteExclusivitySeconds; | ||
| // set status to requested | ||
| bytes memory request = abi.encode( | ||
| BridgeTransactionV2({ | ||
| originChainId: uint32(block.chainid), | ||
| destChainId: params.dstChainId, | ||
| originSender: params.sender, | ||
| destRecipient: params.to, | ||
| originToken: params.originToken, | ||
| destToken: params.destToken, | ||
| originAmount: originAmount, | ||
| destAmount: params.destAmount, | ||
| originFeeAmount: originFeeAmount, | ||
| sendChainGas: params.sendChainGas, | ||
| deadline: params.deadline, | ||
| nonce: nonce++, // increment nonce on every bridge | ||
| exclusivityRelayer: paramsV2.quoteRelayer, | ||
| exclusivityEndTime: exclusivityEndTime | ||
| }) | ||
| ); | ||
| bytes32 transactionId = keccak256(request); | ||
| bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED; | ||
|
|
||
| emit BridgeRequested( | ||
| transactionId, | ||
| params.sender, | ||
| request, | ||
| params.dstChainId, | ||
| params.originToken, | ||
| params.destToken, | ||
| originAmount, | ||
| params.destAmount, | ||
| params.sendChainGas | ||
| ); | ||
| emit BridgeQuoteDetails(transactionId, paramsV2.quoteId); | ||
| } |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
| function bridge(BridgeParams memory params, BridgeParamsV2 memory paramsV2) public payable { | ||
| // check bridge params | ||
| if (params.dstChainId == block.chainid) revert ChainIncorrect(); | ||
| if (params.originAmount == 0 || params.destAmount == 0) revert AmountIncorrect(); | ||
| if (params.sender == address(0) || params.to == address(0)) revert ZeroAddress(); | ||
| if (params.originToken == address(0) || params.destToken == address(0)) revert ZeroAddress(); | ||
| if (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); | ||
| // quoteRelayer and quoteExclusivitySeconds must be both set or both not set | ||
| if ((paramsV2.quoteRelayer == address(0)) != (paramsV2.quoteExclusivitySeconds == 0)) { | ||
| revert ExclusivityParamsIncorrect(); | ||
| } | ||
|
|
||
| // transfer tokens to bridge contract | ||
| // @dev use returned originAmount in request in case of transfer fees | ||
| uint256 originAmount = _pullToken(address(this), params.originToken, params.originAmount); | ||
|
|
||
| // track amount of origin token owed to protocol | ||
| uint256 originFeeAmount; | ||
| if (protocolFeeRate > 0) originFeeAmount = (originAmount * protocolFeeRate) / FEE_BPS; | ||
| originAmount -= originFeeAmount; // remove from amount used in request as not relevant for relayers | ||
|
|
||
| // Calculate exclusivity end time only if exclusivity params are set | ||
| uint256 exclusivityEndTime = | ||
| paramsV2.quoteExclusivitySeconds == 0 ? 0 : block.timestamp + paramsV2.quoteExclusivitySeconds; | ||
| // set status to requested | ||
| bytes memory request = abi.encode( | ||
| BridgeTransactionV2({ | ||
| originChainId: uint32(block.chainid), | ||
| destChainId: params.dstChainId, | ||
| originSender: params.sender, | ||
| destRecipient: params.to, | ||
| originToken: params.originToken, | ||
| destToken: params.destToken, | ||
| originAmount: originAmount, | ||
| destAmount: params.destAmount, | ||
| originFeeAmount: originFeeAmount, | ||
| sendChainGas: params.sendChainGas, | ||
| deadline: params.deadline, | ||
| nonce: nonce++, // increment nonce on every bridge | ||
| exclusivityRelayer: paramsV2.quoteRelayer, | ||
| exclusivityEndTime: exclusivityEndTime | ||
| }) | ||
| ); | ||
| bytes32 transactionId = keccak256(request); | ||
| bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED; | ||
|
|
||
| emit BridgeRequested( | ||
| transactionId, | ||
| params.sender, | ||
| request, | ||
| params.dstChainId, | ||
| params.originToken, | ||
| params.destToken, | ||
| originAmount, | ||
| params.destAmount, | ||
| params.sendChainGas | ||
| ); | ||
| emit BridgeQuoteDetails(transactionId, paramsV2.quoteId); | ||
| } |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
| function bridge(BridgeParams memory params, BridgeParamsV2 memory paramsV2) public payable { | ||
| // check bridge params | ||
| if (params.dstChainId == block.chainid) revert ChainIncorrect(); | ||
| if (params.originAmount == 0 || params.destAmount == 0) revert AmountIncorrect(); | ||
| if (params.sender == address(0) || params.to == address(0)) revert ZeroAddress(); | ||
| if (params.originToken == address(0) || params.destToken == address(0)) revert ZeroAddress(); | ||
| if (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); | ||
| // quoteRelayer and quoteExclusivitySeconds must be both set or both not set | ||
| if ((paramsV2.quoteRelayer == address(0)) != (paramsV2.quoteExclusivitySeconds == 0)) { | ||
| revert ExclusivityParamsIncorrect(); | ||
| } | ||
|
|
||
| // transfer tokens to bridge contract | ||
| // @dev use returned originAmount in request in case of transfer fees | ||
| uint256 originAmount = _pullToken(address(this), params.originToken, params.originAmount); | ||
|
|
||
| // track amount of origin token owed to protocol | ||
| uint256 originFeeAmount; | ||
| if (protocolFeeRate > 0) originFeeAmount = (originAmount * protocolFeeRate) / FEE_BPS; | ||
| originAmount -= originFeeAmount; // remove from amount used in request as not relevant for relayers | ||
|
|
||
| // Calculate exclusivity end time only if exclusivity params are set | ||
| uint256 exclusivityEndTime = | ||
| paramsV2.quoteExclusivitySeconds == 0 ? 0 : block.timestamp + paramsV2.quoteExclusivitySeconds; | ||
| // set status to requested | ||
| bytes memory request = abi.encode( | ||
| BridgeTransactionV2({ | ||
| originChainId: uint32(block.chainid), | ||
| destChainId: params.dstChainId, | ||
| originSender: params.sender, | ||
| destRecipient: params.to, | ||
| originToken: params.originToken, | ||
| destToken: params.destToken, | ||
| originAmount: originAmount, | ||
| destAmount: params.destAmount, | ||
| originFeeAmount: originFeeAmount, | ||
| sendChainGas: params.sendChainGas, | ||
| deadline: params.deadline, | ||
| nonce: nonce++, // increment nonce on every bridge | ||
| exclusivityRelayer: paramsV2.quoteRelayer, | ||
| exclusivityEndTime: exclusivityEndTime | ||
| }) | ||
| ); | ||
| bytes32 transactionId = keccak256(request); | ||
| bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED; | ||
|
|
||
| emit BridgeRequested( | ||
| transactionId, | ||
| params.sender, | ||
| request, | ||
| params.dstChainId, | ||
| params.originToken, | ||
| params.destToken, | ||
| originAmount, | ||
| params.destAmount, | ||
| params.sendChainGas | ||
| ); | ||
| emit BridgeQuoteDetails(transactionId, paramsV2.quoteId); | ||
| } |
Check notice
Code scanning / Slither
Block timestamp Low
| uint256 originAmount = _pullToken(address(this), params.originToken, params.originAmount); | ||
|
|
||
| // track amount of origin token owed to protocol | ||
| uint256 originFeeAmount; |
Check warning
Code scanning / Slither
Uninitialized local variables Medium
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
157-173: LGTM:extractV1()implementationThe
extractV1()function correctly extracts V1 transaction data from a V2 structure, which is crucial for backward compatibility and testing. The implementation looks good and properly ignores V2-specific fields.As a minor suggestion, consider adding a comment explaining that V2-specific fields are intentionally omitted in this extraction process. This will help future maintainers understand the purpose and limitations of this function.
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
116-121: LGTM: Backward-compatible transaction decodingThe
getBridgeTransactionfunction provides backward compatibility for V1 requests while also handling V2 requests. The comment explaining the behavior with V2 requests is helpful for maintainers.Consider adding a
@devtag to the comment for better documentation:- // Note: when passing V2 request, this will decode the V1 fields correctly since the new fields were - // added as the last fields of the struct and hence the ABI decoder will simply ignore the extra data. + /// @dev When passing a V2 request, this will decode the V1 fields correctly since the new fields were + /// added as the last fields of the struct and hence the ABI decoder will simply ignore the extra data.
298-300: LGTM: Implementation of V2 transaction decodingThe
getBridgeTransactionV2function correctly provides a way to decode V2 transactions, which include the new exclusivity fields. The implementation is straightforward and usesabi.decodeappropriately.Consider adding a brief comment explaining the purpose of this function and how it differs from
getBridgeTransaction:/// @notice Decodes a request into a BridgeTransactionV2 structure /// @dev This function is used for V2 transactions that include exclusivity fields /// @param request The encoded bridge transaction request /// @return A decoded BridgeTransactionV2 structure function getBridgeTransactionV2(bytes memory request) public pure returns (BridgeTransactionV2 memory) { return abi.decode(request, (BridgeTransactionV2)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (5 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (3 hunks)
🔇 Additional comments (10)
packages/contracts-rfq/test/FastBridgeV2.t.sol (5)
18-18: Reconsider disabling themax-states-countSolhint ruleWhile disabling linter rules can be necessary in some cases, it's generally not recommended, especially for rules like
max-states-count. This rule helps maintain contract simplicity and gas efficiency. Consider refactoring the contract to reduce the number of state variables or provide a clear justification for disabling this rule.
39-45: Ensure consistent initialization and usage of V1 and V2 typesThe introduction of V2 types (
IFastBridgeV2.BridgeTransactionV2andIFastBridgeV2.BridgeParamsV2) alongside V1 types suggests a transition or backward compatibility. Ensure that both V1 and V2 types are properly initialized in thecreateFixtures()andcreateFixturesV2()functions, and used consistently throughout the test suite to cover all scenarios.
51-51: Verify completeness ofcreateFixturesV2()The addition of
createFixturesV2()in thesetUp()function is a good step to ensure V2 fixtures are created. However, it's crucial to verify that this function properly initializes all V2-specific fields, including the new exclusivity parameters. Ensure that all test scenarios, including edge cases related to the new V2 functionality, are covered.
87-122: Ensure complete initialization of V2 transaction structuresThe
createFixtures()function now usessetStorageBridgeTxV2()to set up transaction data, which is a good approach for transitioning to V2 structures. However, ensure that all V2-specific fields (such asexclusivityRelayerandexclusivityEndTime) are also properly initialized here. Consider adding these fields to thesetStorageBridgeTxV2()function to ensure complete initialization of V2 transaction structures.
176-176: Verify impact of updatedgetTxId()functionThe update of
getTxId()to acceptIFastBridgeV2.BridgeTransactionV2is necessary and correct for the V2 implementation. However, ensure that this change doesn't break any existing tests or functionality that might still be using the V1 transaction structure. Consider adding test cases that specifically verify the behavior ofgetTxId()with both V1 and V2 transaction structures to ensure backward compatibility.packages/contracts-rfq/contracts/FastBridgeV2.sol (5)
44-47: LGTM: Backward-compatible implementation of new bridge functionalityThe changes to the
bridgefunction maintain backward compatibility while introducing new functionality. The use of default values forBridgeParamsV2ensures that existing calls tobridgewill continue to work without modifications.
254-254: LGTM: Updated transaction decoding in claim functionThe change to use
getBridgeTransactionV2in theclaimfunction is consistent with the introduction of the V2 transaction structure. This ensures that the claim process uses the updated transaction information, including any new fields introduced in the V2 structure.
186-202: LGTM: Implementation of relayer exclusivity in relay processThe changes to the
relayfunction successfully implement the relayer exclusivity feature in the relay process. The new checks ensure that only the designated relayer can relay during the exclusivity period.Consider extracting the exclusivity check into a separate internal function to improve readability:
function _checkExclusivity(BridgeTransactionV2 memory transaction, address relayer) internal view { if (block.timestamp <= transaction.exclusivityEndTime && relayer != transaction.exclusivityRelayer) { revert ExclusivityPeriodNotPassed(); } }This would allow you to simplify the
relayfunction and make it easier to reuse the exclusivity check logic if needed elsewhere in the contract.To ensure that the
bridgeRelaysfunction is correctly implemented and used, run the following script:#!/bin/bash # Check the implementation and usage of bridgeRelays function ast-grep --lang solidity --pattern 'function bridgeRelays(bytes32 transactionId) public view returns (bool)' rg -n 'bridgeRelays\(' packages/contracts-rfq/contracts/FastBridgeV2.sol
Line range hint
1-350: Overall LGTM: Comprehensive implementation of relayer exclusivityThe changes in this file successfully implement the relayer exclusivity feature throughout the bridging process. The new structures and functions added support this feature well, and existing functions have been appropriately modified to accommodate the new functionality. The implementation maintains backward compatibility where possible and follows good coding practices.
Consider the following suggestions for further improvement:
Add comprehensive unit tests to cover all new functionality, especially edge cases around the exclusivity period and different combinations of exclusivity parameters.
Update the contract's documentation (e.g., NatSpec comments) to reflect the new relayer exclusivity feature and how it affects the bridging process.
Consider implementing a mechanism to allow the contract owner or a designated role to modify the exclusivity parameters (e.g., maximum exclusivity duration) if needed in the future.
Evaluate the gas cost impact of these changes, especially for the
bridgeandrelayfunctions, and optimize if necessary.To ensure that all new events are properly emitted and all new error types are used, run the following script:
#!/bin/bash # Check for new events and error types echo "Checking for new events:" ast-grep --lang solidity --pattern 'emit BridgeQuoteDetails($_, $_);' echo "Checking for new error types:" ast-grep --lang solidity --pattern 'revert ExclusivityParamsIncorrect();' ast-grep --lang solidity --pattern 'revert ExclusivityPeriodNotPassed();'
124-184: LGTM: Implementation of relayer exclusivity featureThe changes to the
bridgefunction successfully implement the relayer exclusivity feature. The new validations ensure that the exclusivity parameters are used correctly, and the use ofBridgeTransactionV2allows for the inclusion of exclusivity information.Consider the following improvements:
- Extract the validation logic into a separate internal function to reduce the complexity of the
bridgefunction:function _validateBridgeParams(BridgeParams memory params, BridgeParamsV2 memory paramsV2) internal view { if (params.dstChainId == block.chainid) revert ChainIncorrect(); if (params.originAmount == 0 || params.destAmount == 0) revert AmountIncorrect(); if (params.sender == address(0) || params.to == address(0)) revert ZeroAddress(); if (params.originToken == address(0) || params.destToken == address(0)) revert ZeroAddress(); if (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); if ((paramsV2.quoteRelayer == address(0)) != (paramsV2.quoteExclusivitySeconds == 0)) { revert ExclusivityParamsIncorrect(); } }
- Consider using SafeMath or Solidity 0.8+ checked arithmetic for the
originFeeAmountcalculation to prevent potential overflow issues.To ensure that the
BridgeQuoteDetailsevent is properly indexed and can be efficiently queried, run the following script:✅ Verification successful
Verified: BridgeQuoteDetails event is properly indexed
The
BridgeQuoteDetailsevent is correctly indexed withtransactionIdas an indexed parameter in the relevant interface and test files. No issues were found regarding the event indexing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the BridgeQuoteDetails event is properly indexed ast-grep --lang solidity --pattern 'event BridgeQuoteDetails(bytes32 indexed transactionId, bytes quoteId);'Length of output: 702
Script:
#!/bin/bash # Check if the BridgeQuoteDetails event is properly indexed using ripgrep rg 'event BridgeQuoteDetails\(bytes32 indexed transactionId, bytes quoteId\);' --type solidityLength of output: 129
Script:
#!/bin/bash # Check if the BridgeQuoteDetails event is properly indexed using ripgrep rg 'event BridgeQuoteDetails\(bytes32 indexed transactionId, bytes quoteId\);' --glob '*.sol'Length of output: 360
| function createFixturesV2() public virtual { | ||
| // Override in tests with exclusivity params | ||
| tokenParamsV2 = | ||
| IFastBridgeV2.BridgeParamsV2({quoteRelayer: address(0), quoteExclusivitySeconds: 0, quoteId: ""}); | ||
| ethParamsV2 = IFastBridgeV2.BridgeParamsV2({quoteRelayer: address(0), quoteExclusivitySeconds: 0, quoteId: ""}); | ||
|
|
||
| tokenTx.exclusivityRelayer = address(0); | ||
| tokenTx.exclusivityEndTime = 0; | ||
| ethTx.exclusivityRelayer = address(0); | ||
| ethTx.exclusivityEndTime = 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand createFixturesV2() to cover more test scenarios
While createFixturesV2() sets default values for exclusivity parameters, consider expanding this function to initialize a wider range of test scenarios. This could include:
- Cases with non-zero
quoteExclusivitySeconds - Different
quoteRelayeraddresses - Various
quoteIdvalues
Additionally, ensure that tokenTx and ethTx are fully initialized with all V2-specific fields. This will help in testing the new exclusivity features more comprehensively.
| function setStorageBridgeTxV2( | ||
| IFastBridgeV2.BridgeTransactionV2 storage txV2, | ||
| IFastBridge.BridgeTransaction memory txV1 | ||
| ) | ||
| internal | ||
| { | ||
| txV2.originChainId = txV1.originChainId; | ||
| txV2.destChainId = txV1.destChainId; | ||
| txV2.originSender = txV1.originSender; | ||
| txV2.destRecipient = txV1.destRecipient; | ||
| txV2.originToken = txV1.originToken; | ||
| txV2.destToken = txV1.destToken; | ||
| txV2.originAmount = txV1.originAmount; | ||
| txV2.destAmount = txV1.destAmount; | ||
| txV2.originFeeAmount = txV1.originFeeAmount; | ||
| txV2.sendChainGas = txV1.sendChainGas; | ||
| txV2.deadline = txV1.deadline; | ||
| txV2.nonce = txV1.nonce; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add V2-specific field initialization in setStorageBridgeTxV2()
The setStorageBridgeTxV2() function effectively copies data from V1 to V2 transaction structures, which is good for maintaining compatibility. However, it doesn't initialize the V2-specific fields exclusivityRelayer and exclusivityEndTime. Consider adding parameters for these fields and initializing them in this function to ensure complete V2 transaction setup. This will make the function more versatile for various test scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
116-125: LGTM: New function added for backward compatibility.The
getBridgeTransactionfunction provides a way to decode V1 transactions, ensuring backward compatibility. This is a good practice for maintaining existing integrations.Consider addressing the TODO comment in a future iteration.
The TODO comment about reducing cyclomatic complexity should be addressed in a future update to improve code maintainability.
126-182: LGTM: Enhanced bridge functionality with new features and validations.The updated
bridgefunction now supports exclusivity parameters and uses the newBridgeTransactionV2structure. The additional validations improve the robustness of the function.Consider refactoring to reduce complexity.
As noted in the TODO comment, the function's cyclomatic complexity has increased. Consider breaking down this function into smaller, more manageable pieces in a future update to improve readability and maintainability.
Line range hint
1-348: Overall assessment: Well-implemented changes with room for future improvements.The changes to
FastBridgeV2.solsuccessfully introduce new functionality for exclusivity in bridging while maintaining backward compatibility. The code is consistent in its approach to handling the newBridgeTransactionV2structure across different functions.Recommendations for future iterations:
- Address the TODO comments about reducing cyclomatic complexity, particularly in the
bridgeandrelayfunctions.- Consider breaking down larger functions into smaller, more manageable pieces to improve readability and maintainability.
- Ensure comprehensive testing of the new exclusivity features and their interaction with existing functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (5 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (4 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol
- packages/contracts-rfq/test/FastBridgeV2.Dst.Exclusivity.t.sol
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Dst.Excl.t.sol
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.t.sol
- packages/contracts-rfq/test/FastBridgeV2.t.sol
🔇 Additional comments (11)
packages/contracts-rfq/contracts/FastBridgeV2.sol (6)
44-47: LGTM: Backward-compatible implementation of new bridge functionality.The overloaded
bridgefunction maintains compatibility with existing calls while introducing new functionality throughBridgeParamsV2. The default values ensure that the contract's behavior remains consistent for existing integrations.
185-205: LGTM: Relay function updated with new exclusivity checks.The
relayfunction has been successfully updated to work withBridgeTransactionV2and implement the new exclusivity feature. The use ofbridgeRelaysinstead of directly accessingbridgeRelayDetailsis a good practice for code organization.
191-191: LGTM: Improved encapsulation in relay check.The use of
bridgeRelays(transactionId)instead of directly accessingbridgeRelayDetailsimproves code organization and encapsulation. This change makes the code more maintainable and flexible for future updates.
193-193: LGTM: Updated to use new transaction structure.The use of
getBridgeTransactionV2ensures that the relay function correctly decodes and works with the newBridgeTransactionV2structure. This change is consistent with the overall updates to the contract.
257-257: LGTM: Claim function updated to use new transaction structure.The
claimfunction has been correctly updated to usegetBridgeTransactionV2, ensuring consistency with the new transaction structure used throughout the contract.
301-303: LGTM: New function added for decoding V2 transactions.The
getBridgeTransactionV2function is a necessary addition for working with the newBridgeTransactionV2structure. It's consistent with the existinggetBridgeTransactionfunction and supports the contract's evolution.packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (5)
12-16: Ensure negative values are handled correctly inquoteExclusivitySeconds.Assigning negative values to
quoteExclusivitySeconds(lines 12 and 15) may lead to unintended behavior if the contract does not properly handle negative exclusivity periods. While this may be intentional for testing negative scenarios, please confirm that the contract logic is designed to handle negative values without causing integer underflows or other issues.To verify that negative
quoteExclusivitySecondsvalues are appropriately handled, please double-check the contract's input validation and error handling for negative exclusivity periods.
19-21: Confirm intentional setting of pastexclusivityEndTime.Setting
exclusivityEndTimeto a past timestamp usingblock.timestamp - EXCLUSIVITY_PERIOD_ABS(lines 19 and 21) results in an exclusivity period that has already expired. Ensure this is intentional for testing purposes to verify the contract's behavior when the exclusivity period is over.
30-32: Check for integer underflow with negative exclusivity seconds.In
test_bridge_revert_exclusivityEndTimeZero(line 30), assigningtokenParamsV2.quoteExclusivitySeconds = -int256(block.timestamp)may cause integer underflow, resulting in a large positive value. Confirm that this value triggers the expectedExclusivityParamsIncorrectrevert and that the test accurately represents the intended negative scenario.Ensure that the contract handles negative exclusivity periods correctly and that the test case effectively checks for the expected revert behavior.
48-50: Address integer underflow risks in ETH exclusivity period underflow test.Assigning
ethParamsV2.quoteExclusivitySeconds = -int256(block.timestamp + 1)(line 48) can lead to integer underflow. Ensure that the contract's logic includes safeguards against such cases and that the test accurately reflects the scenario.Review the contract's input validation for exclusivity periods to prevent negative values and ensure consistent behavior across different transaction types.
24-26: Verify correct override of thebridgefunction.The overridden
bridgefunction (line 24) should match the base class's function signature exactly. Ensure that the parameters and their order are consistent with the base contract to prevent any unexpected behavior.Run the following script to compare the
bridgefunction signatures:This will help confirm that the function signatures are correctly matched.
| tokenParamsV2.quoteExclusivitySeconds = -int256(block.timestamp + 1); | ||
| vm.expectRevert(ExclusivityParamsIncorrect.selector); | ||
| bridge({caller: userA, msgValue: 0, params: tokenParams, paramsV2: tokenParamsV2}); |
There was a problem hiding this comment.
Avoid potential integer underflow in exclusivity period calculation.
Setting tokenParamsV2.quoteExclusivitySeconds = -int256(block.timestamp + 1) (line 36) could result in integer underflow, leading to unexpected behavior. It's important to verify that the contract logic properly handles such edge cases.
Consider adding input validation in the contract to prevent negative exclusivity periods or underflows. This will enhance the contract's robustness against invalid inputs.
| ethParamsV2.quoteExclusivitySeconds = -int256(block.timestamp); | ||
| vm.expectRevert(ExclusivityParamsIncorrect.selector); | ||
| bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams, paramsV2: ethParamsV2}); |
There was a problem hiding this comment.
Ensure proper handling of negative exclusivity periods in ETH tests.
In test_bridge_eth_revert_exclusivityEndTimeZero (line 42), setting ethParamsV2.quoteExclusivitySeconds = -int256(block.timestamp) may cause integer underflow similar to the token test. Verify that this test correctly triggers the expected revert and that the contract handles negative values appropriately.
It's essential to confirm that negative exclusivity periods are adequately managed in ETH transactions to prevent unforeseen issues.
Description
User should be able to provide new fields for the
bridge()function (field names are tentative):Old
bridge()function should be preserved, by using the default empty/zero values for these fields.Objective:
quoteRelayerto have exclusive fill rights until exclusivity deadline.Summary by CodeRabbit
New Features
FastBridgeV2contract with enhanced transaction handling capabilities.BridgeQuoteDetailsto log bridge quote details with unique identifiers.Bug Fixes
Tests