feat(contracts-rfq): save destination chain ID for requested bridges [SLT-423] #3356
Conversation
WalkthroughThe changes in this pull request involve 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
|
Deploying sanguine-fe with
|
| Latest commit: |
c417fce
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b579967c.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-save-dest-chain-id.sanguine-fe.pages.dev |
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
428-435: LGTM: Well-documented timestamp handling with gas optimization.The _timeSince function is well implemented with:
- Clear documentation of the rollover case
- Safe use of unchecked block for gas optimization
- Proper type casting for timestamp comparison
Consider adding an example in the documentation to illustrate the rollover case:
/// @dev proof.timestamp stores casted uint56(block.timestamp) block timestamps for gas optimization /// _timeSince(proof) can accomodate rollover case when block.timestamp > type(uint56).max but -/// proof.timestamp < type(uint56).max via unchecked statement +/// proof.timestamp < type(uint56).max via unchecked statement +/// Example: block.timestamp = 2^56 + 100, proof.timestamp = 50 => delta = 50packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
97-108: LGTM! Consider adding documentation for the helper functions.The new helper functions effectively encapsulate common assertion patterns for different bridge states. They follow a consistent structure and help improve test maintainability.
Consider adding NatSpec documentation to describe the expected state for each helper function:
+/// @notice Verifies that a bridge transaction is in the REQUESTED state with no proof details function checkStatusAndProofAfterBridge(bytes32 txId) public view {Also applies to: 285-296, 473-484, 702-705, 807-818
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (5)
Line range hint
36-36: Useuint256instead ofint256forquoteExclusivitySecondsSince
quoteExclusivitySecondsrepresents a duration and should not be negative, using an unsigned integer prevents negative values which could cause unexpected behavior.Apply this diff to fix the type:
- int256 quoteExclusivitySeconds; + uint256 quoteExclusivitySeconds;
Line range hint
28-28: Clarify thedestTokenaddress for native tokensThe address
0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeEis commonly used to represent native tokens. Ensure that this convention is consistently applied and that the code correctly handles scenarios involving native tokens andzapNative.
Line range hint
66-66: Consider overloading thebridgefunction for better readabilityPassing both
BridgeParamsandBridgeParamsV2structs may lead to confusion. Overloading thebridgefunction or creating a single comprehensive struct could improve code clarity and usability.Example of function overloading:
- function bridge(BridgeParams memory params, BridgeParamsV2 memory paramsV2) external payable; + function bridge(BridgeParams memory params) external payable; + function bridge(BridgeParamsV2 memory paramsV2) external payable;Alternatively, combine the structs:
+ struct BridgeParamsCombined { + // Fields from BridgeParams + // Fields from BridgeParamsV2 + } + function bridge(BridgeParamsCombined memory params) external payable;
Line range hint
61-61: Add an event forbridgefunction to emit transaction detailsEmitting an event in the
bridgefunction can enhance transparency and facilitate tracking of bridge transactions.Example of adding an event:
event BridgeInitiated(bytes32 indexed transactionId, address sender, uint32 destChainId, uint256 amount);And emit it in the
bridgefunction implementation.
Line range hint
93-95: Ensure proper documentation forgetBridgeTransactionV2The
getBridgeTransactionV2function lacks detailed documentation on its behavior and return values. Adding comprehensive comments will improve maintainability and ease of use.Consider expanding the documentation:
/// @notice Decodes bridge request into a bridge transaction V2 struct used by FastBridgeV2 +/// @dev Parses the encoded request and returns a structured `BridgeTransactionV2` /// @param request The bridge request to decode +/// @return bridgeTx The decoded bridge transaction details as `BridgeTransactionV2` function getBridgeTransactionV2(bytes memory request) external view returns (BridgeTransactionV2 memory);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/contracts-rfq/contracts/FastBridgeV2.sol(5 hunks)packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.t.sol(36 hunks)
🧰 Additional context used
📓 Learnings (3)
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-10-14T14:45:57.214Z
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-10-14T14:45:57.214Z
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.t.sol (3)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993
Timestamp: 2024-10-14T14:48:01.520Z
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.t.sol:53-56
Timestamp: 2024-10-14T13:24:45.293Z
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-10-14T14:45:57.214Z
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.
🔇 Additional comments (4)
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
92-92: LGTM: Improved timestamp handling with uint56.
The change from uint40 to uint56 for proofBlockTimestamp provides a larger buffer against timestamp overflow, extending support from year 2104 to year 4147.
Also applies to: 317-317, 331-331
223-225: LGTM: Efficient storage of destination chain ID.
The implementation efficiently stores the destination chain ID alongside the status update, providing crucial tracking information for cross-chain transactions.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
123-123: LGTM! Helper functions are well integrated.
The integration of the new helper functions across test cases improves code organization and makes the test assertions more consistent and maintainable.
Also applies to: 134-134, 156-156, 172-172, 191-191, 502-502, 515-515, 528-528, 540-540, 553-553, 574-574, 588-588, 602-602, 615-615, 628-628, 713-713, 725-725, 737-737, 750-750, 826-826, 839-839, 852-852, 865-865, 878-878, 892-892, 906-906, 920-920
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
Line range hint 42-42: Double-check the exclusivity logic in BridgeTransactionV2
Ensure that the exclusivity mechanism using exclusivityRelayer and exclusivityEndTime is correctly implemented and that edge cases are handled, such as when exclusivity expires.
Run the following script to search for usages and implementations of exclusivity fields:
✅ Verification successful
Let me gather more information about the exclusivity validation logic.
Let me check the actual validation logic in FastBridgeV2.sol.
Let me check one final piece - the validation of exclusivityEndTime.
Based on the comprehensive analysis of the exclusivity mechanism in FastBridgeV2, I can now provide the final response.
Exclusivity mechanism is properly implemented with comprehensive safeguards
The exclusivity logic in BridgeTransactionV2 is correctly implemented with multiple layers of protection:
- The
exclusivityEndTimeis validated to be within range[0 .. params.deadline] - The relay validation ensures only the designated
exclusivityRelayercan process the transaction untilexclusivityEndTime - After
exclusivityEndTime, any relayer can process the transaction - Extensive test coverage verifies both positive and negative cases, including edge cases like:
- Last second of exclusivity period
- Expired exclusivity period
- Unauthorized relayer attempts during exclusivity
- Zero exclusivity time handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of `exclusivityRelayer` and `exclusivityEndTime`
# Test: Search for all references to exclusivity fields in the codebase.
# Expect: Proper handling of exclusivity logic.
rg 'exclusivity(Relayer|EndTime)'
Length of output: 6272
Script:
#!/bin/bash
# Search for the validation logic in FastBridgeV2.sol
rg -A 10 '_validateBridgeParams' packages/contracts-rfq/contracts/FastBridgeV2.sol
# Search for tests related to exclusivity validation
rg -A 10 'test.*exclusivity' packages/contracts-rfq/test/
Length of output: 19777
Script:
#!/bin/bash
# Search for the relay validation logic in FastBridgeV2.sol
rg -A 10 'if \(exclRelayer' packages/contracts-rfq/contracts/FastBridgeV2.sol
# Search for the validation function implementation
rg -B 5 -A 15 'function _validateBridgeParams' packages/contracts-rfq/contracts/FastBridgeV2.sol
Length of output: 1476
Script:
#!/bin/bash
# Search for exclusivity validation in FastBridgeV2.sol
rg -B 2 -A 5 'exclusivityEndTime.*params\.deadline' packages/contracts-rfq/contracts/FastBridgeV2.sol
# Search for any custom errors related to exclusivity
rg 'error.*Exclusivity' packages/contracts-rfq/contracts/FastBridgeV2.sol
Length of output: 972
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3356 +/- ##
===================================================
+ Coverage 90.44834% 92.29025% +1.84190%
===================================================
Files 54 60 +6
Lines 1026 1323 +297
Branches 82 82
===================================================
+ Hits 928 1221 +293
- Misses 95 99 +4
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. |
Description
Swaps out proof block number for destination chain ID in the
BridgeTxDetailsstruct that tracks relevant fields for a bridge request on the origin chain.Summary by CodeRabbit
New Features
Bug Fixes
Documentation