refactor(contracts-rfq): mark new methods as V2
#3410
Conversation
WalkthroughThe pull request implements significant updates 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
|
![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 (8)
packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.sol (1)
53-53: LGTM, but ensure consistency across helper functions.The update to use
relayV2is correct, however, there's an inconsistency with therelayhelper function (line 39) which still uses the oldrelaymethod.Consider updating both helper functions consistently:
function relay(address caller, uint256 msgValue, IFastBridgeV2.BridgeTransactionV2 memory bridgeTx) public { bytes memory request = BridgeTransactionV2Lib.encodeV2(bridgeTx); vm.prank({msgSender: caller, txOrigin: caller}); - fastBridge.relay{value: msgValue}(request); + fastBridge.relayV2{value: msgValue}(request, caller); }packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol (1)
Line range hint
1-115: Comprehensive test coverage for V2 bridging scenariosThe test suite effectively covers various bridging scenarios including:
- Token deposits
- Native token deposits
- Zap operations
- Error cases
The test structure maintains good separation of concerns with distinct setup, execution, and verification phases.
Consider adding test cases for:
- Edge cases with maximum values
- Cross-chain gas fee scenarios
- Failed zap operations
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (1)
456-468: Consider adding migration test cases.While the tests cover the V2 functionality well, consider adding test cases that verify the migration path from V1 to V2, ensuring backward compatibility if required.
Example test case to add:
+ function test_relay_v1_to_v2_migration() public { + // First relay with V1 + bytes memory v1Request = // ... create v1 request + vm.prank(relayerA); + fastBridge.relay(v1Request); + + // Then relay with V2 + bytes memory v2Request = // ... create v2 request + vm.prank(relayerA); + fastBridge.relayV2(v2Request, relayerB); + + // Verify both transactions are marked as relayed + assertTrue(fastBridge.bridgeRelays(getTxId(v1Request))); + assertTrue(fastBridge.bridgeRelays(getTxId(v2Request))); + }packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
1001-1013: Consider updating test names for consistency with V2 methods.The test functions verify
claimV2behavior but their names don't reflect the V2 suffix. Consider updating the test names to match the method being tested.Apply this diff to maintain consistency:
- function test_claim_revert_requestV1() public { + function test_claimV2_revert_requestV1() public { expectRevertUnsupportedVersion(0); vm.prank({msgSender: relayerA, txOrigin: relayerA}); fastBridge.claimV2(mockRequestV1); } - function test_claim_revert_invalidRequestV2() public { + function test_claimV2_revert_invalidRequestV2() public { expectRevertInvalidEncodedTx(); vm.prank({msgSender: relayerA, txOrigin: relayerA}); fastBridge.claimV2(invalidRequestV2); } - function test_claim_revert_requestV3() public { + function test_claimV2_revert_requestV3() public { expectRevertUnsupportedVersion(3); vm.prank({msgSender: relayerA, txOrigin: relayerA}); fastBridge.claimV2(mockRequestV3); }
1039-1051: Consider updating test names for consistency with V2 methods.The test functions verify
cancelV2behavior but their names don't reflect the V2 suffix. Consider updating the test names to match the method being tested.Apply this diff to maintain consistency:
- function test_cancel_revert_requestV1() public { + function test_cancelV2_revert_requestV1() public { expectRevertUnsupportedVersion(0); vm.prank({msgSender: relayerA, txOrigin: relayerA}); fastBridge.cancelV2(mockRequestV1); } - function test_cancel_revert_invalidRequestV2() public { + function test_cancelV2_revert_invalidRequestV2() public { expectRevertInvalidEncodedTx(); vm.prank({msgSender: relayerA, txOrigin: relayerA}); fastBridge.cancelV2(invalidRequestV2); } - function test_cancel_revert_requestV3() public { + function test_cancelV2_revert_requestV3() public { expectRevertUnsupportedVersion(3); vm.prank({msgSender: relayerA, txOrigin: relayerA}); fastBridge.cancelV2(mockRequestV3); }packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
Line range hint
180-243: Ensure comprehensive validation inbridgeV2parametersThe new
bridgeV2function introduces additional parameters withinBridgeParamsV2. Verify that_validateBridgeParamsthoroughly checks all new and existing parameters to prevent potential security vulnerabilities or misuse.Would you like assistance in reviewing or enhancing the validation logic to ensure robust parameter checks?
Line range hint
281-349: Reentrancy consideration inrelayV2functionWhile
relayV2follows the checks-effects-interactions pattern, the transfer of tokens and execution of Zap involve external calls. To enhance security, consider implementing reentrancy guards to protect against potential reentrancy attacks.Apply this diff to add reentrancy protection:
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; -contract FastBridgeV2 is AdminV2, MulticallTarget, IFastBridgeV2, IFastBridgeV2Errors { +contract FastBridgeV2 is AdminV2, MulticallTarget, IFastBridgeV2, IFastBridgeV2Errors, ReentrancyGuard { ... - function relayV2(bytes calldata request, address relayer) public payable { + function relayV2(bytes calldata request, address relayer) public payable nonReentrant { // existing code... } ... }
Line range hint
243-279: Potential issue with refund logic incancelV2In
cancelV2, when calculating the refund amount, the full amount (collateral plus protocol fees) is returned to the user. If the protocol fees are meant to be accumulated regardless of transaction cancellation, this could impact revenue. Verify if returning the protocol fees is the intended behavior.If protocol fees should be retained upon cancellation, consider modifying the refund logic:
-uint256 amount = request.originAmount() + request.originFeeAmount(); +uint256 amount = request.originAmount(); +// Accumulate protocol fees even if the transaction is cancelled +if (request.originFeeAmount() > 0) protocolFees[token] += request.originFeeAmount();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
packages/contracts-rfq/contracts/FastBridgeV2.sol(6 hunks)packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Dst.Base.t.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol(3 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.t.sol(2 hunks)packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (5)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (4)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-11-12T03:37:02.230Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993
Timestamp: 2024-11-12T03:37:02.230Z
Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol:97-97
Timestamp: 2024-11-12T03:37:08.148Z
Learning: In `packages/contracts-rfq/test/FastBridgeV2.t.sol`, when testing expected revert behaviors, `abi.encodeWithSelector` is used with custom errors and should not be replaced with `BridgeTransactionV2Lib.encodeV2`.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol (2)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3300
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:451-453
Timestamp: 2024-11-12T03:37:02.230Z
Learning: In the `FastBridgeV2` contract (`packages/contracts-rfq/contracts/FastBridgeV2.sol`), `destToken` is used for the Zap, so when validating parameters in `_validateBridgeParams`, we should check `params.destToken` when verifying if `ZapNativeNotSupported`.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993
Timestamp: 2024-11-12T03:37:02.230Z
Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.
🔇 Additional comments (18)
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol (1)
40-40: LGTM! Function call updated to match V2 interface
The change from bridge to bridgeV2 correctly aligns with the interface updates while maintaining the same parameter structure.
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (4)
71-71: LGTM: Correctly updated to use V2 bridge method
The change appropriately updates the test helper to use the new bridgeV2 method while maintaining backward compatibility by accepting both V1 and V2 parameters.
76-76: LGTM: Correctly updated to use V2 prove method
The change appropriately updates the test helper to use the new proveV2 method while maintaining the same parameter structure.
86-86: LGTM: Correctly updated to use V2 claim method
The change appropriately updates the test helper to use the new claimV2 method while maintaining the correct encoding of V2 bridge transactions.
101-101: LGTM: Correctly updated to use V2 cancel method
The change appropriately updates the test helper to use the new cancelV2 method while maintaining the correct encoding of V2 bridge transactions.
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (2)
456-456: LGTM! Function calls updated consistently to V2.
The renaming from relay to relayV2 is consistent with the PR objectives of marking new methods as V2. The test cases properly validate the behavior of the V2 implementation.
Also applies to: 462-462, 468-468
Line range hint 1-600: Verify test coverage for V2-specific features.
The test file comprehensively covers various scenarios, but let's verify that all V2-specific features are properly tested.
✅ Verification successful
Let me try a different approach to search for V2-specific test coverage.
Let me try another search approach to verify the V2-specific test coverage.
Based on the search results, I can now provide a final response about the test coverage for V2-specific features.
Test coverage for V2-specific features is comprehensive and well-structured.
The test suite includes:
- Core relay functionality tests for both tokens and ETH
- Version-specific tests including V1, V2, and V3 request handling
- Extensive error cases and edge conditions
- Various recipient types and their behaviors
- Comprehensive testing of relayer address handling
- ZapNative functionality testing
- Protocol fee handling
- Integration tests with various components
The test file structure shows thorough coverage across different aspects:
- Base functionality tests
- Gas benchmarking
- Encoding tests
- Integration tests
- Exclusivity tests
- Protocol fee tests
- Zap functionality tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for V2-specific test cases and features
echo "Searching for V2-specific test cases..."
rg -l "V2" --type solidity
echo "Searching for relay function implementations..."
ast-grep --pattern 'function relay$$$V2($$$) {
$$$
}'
Length of output: 311
Script:
#!/bin/bash
# Search for V2-specific test cases and features
echo "Searching for V2-specific test cases..."
rg -l "FastBridgeV2" .
echo "Searching for V2 test implementations..."
rg "test.*V2" -A 2 .
echo "Searching for relay function implementations..."
rg "function relay.*V2" -A 2 .
Length of output: 134065
Script:
#!/bin/bash
# Search for test files and their contents
echo "Searching for test files..."
find . -name "FastBridgeV2*.t.sol" -type f
echo "Searching for test functions in FastBridgeV2.Dst.t.sol..."
rg "^ function test" ./packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol
echo "Searching for V2-specific relay implementations..."
rg "function relay.*V2|function test.*relay.*V2" ./packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol
Length of output: 8094
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (5)
73-73: Function bridgeV2 signature aligns with updated parameters
The bridgeV2 function correctly introduces the new BridgeParamsV2 parameters, allowing for extended functionality while maintaining backward compatibility.
78-78: Function relayV2 appropriately updated with V2 specifications
The relayV2 function updates the relay mechanism in line with V2 requirements, ensuring it accommodates the enhanced bridge transaction process.
84-84: Function proveV2 accurately reflects new proofing parameters
The proveV2 function correctly incorporates the V2 parameters needed for transaction proofing, maintaining consistency with the updated protocol.
89-89: claimV2 function enables proper claiming under V2
The claimV2 function provides a clear method for relayers to claim deposited capital in accordance with the V2 structure.
94-94: cancelV2 function facilitates secure transaction cancellation
The cancelV2 function appropriately allows users to cancel outstanding bridge transactions and ensures the safe return of funds if optimistic bridging fails.
packages/contracts-rfq/contracts/FastBridgeV2.sol (6)
Line range hint 68-75: Updated bridge function delegates to bridgeV2 correctly
The bridge function now calls bridgeV2 with default BridgeParamsV2 values. This ensures backward compatibility while integrating the new parameters introduced in bridgeV2.
84-86: refund function now redirects to cancelV2
By updating refund to call cancelV2, the code maintains existing functionality and aligns with the new versioning scheme.
92-94: relay function delegates to relayV2 with appropriate parameters
The relay function now passes request and msg.sender as relayer to relayV2, ensuring consistent behavior with the updated contract logic.
98-100: prove function incorporates V2 validation before delegation
The prove function now validates the request using validateV2() before calling proveV2. This addition ensures that only properly formatted V2 requests are processed.
102-104: Verify the use of address(0) in claimV2 function
In claimV2, the function calls claim with the to parameter set to address(0). Confirm that using address(0) correctly defaults the claim to the proven relayer and does not introduce unintended behavior.
Line range hint 350-357: Proper access control in proveV2 function
The proveV2 function is secured with onlyRole(PROVER_ROLE), ensuring that only authorized entities can call it. This is a good practice for maintaining contract security.
Deploying sanguine-fe with
|
| Latest commit: |
209fbb1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f6b6cda4.sanguine-fe.pages.dev |
| Branch Preview URL: | https://refactor-fbv2-new-methods.sanguine-fe.pages.dev |
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 #3410 +/- ##
=============================================
Coverage 41.80602% 41.80602%
=============================================
Files 87 87
Lines 2990 2990
Branches 82 82
=============================================
Hits 1250 1250
Misses 1737 1737
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
bridgeV2,claimV2).BridgeParamsV2structure to enhance transaction handling.Bug Fixes
Documentation