exclusivityEndtime default 0 [SLT-391] #3336
Conversation
WalkthroughThe changes in this pull request focus on 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3336 +/- ##
===================================================
+ Coverage 33.25951% 34.85109% +1.59158%
===================================================
Files 543 529 -14
Lines 34769 33778 -991
Branches 82 82
===================================================
+ Hits 11564 11772 +208
+ Misses 22182 20986 -1196
+ Partials 1023 1020 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (2 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (1 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/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (1)
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.packages/contracts-rfq/test/FastBridgeV2.t.sol (4)
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.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: In Solidity test contracts under the `test` directory, such as `FastBridgeV2.t.sol`, contracts are not deployed, so gas consumption is not a concern.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.
🔇 Additional comments (6)
packages/contracts-rfq/test/FastBridgeV2.Src.Exclusivity.Negative.t.sol (2)
37-40: ETH bridge test consistently updated with token bridge test.The ETH bridge test mirrors the changes made to the token bridge test, ensuring consistent behavior verification across both token and ETH bridges.
26-29: Test name and behavior updated to reflect new requirements.The test has been correctly updated to verify that setting
quoteExclusivitySecondsto-block.timestampis now a valid operation, aligning with the PR's objective of allowingexclusivityEndtimeto be 0 for non-exclusive bridges.Let's verify the test coverage for this new behavior:
✅ Verification successful
Let me try a different approach to verify the test coverage and implementation.
Test changes correctly align with contract implementation
The verification results show that:
- The contract explicitly handles zero exclusivity by setting
exclusivityEndTime = 0whenquoteRelayeris not set (FastBridgeV2.sol)- The interface documentation confirms this behavior: "quoteRelayer and quoteExclusivitySeconds are either both zero (indicating no exclusivity) or both non-zero"
- The test changes align with this implementation by verifying that setting
quoteExclusivitySecondsto-block.timestampresults in valid non-exclusive bridge operations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are corresponding positive test cases for the new behavior # and verify the contract implementation matches the test assumptions # Search for related test cases echo "Searching for related test cases..." rg -l "quoteExclusivitySeconds.*block\.timestamp" --type sol # Search for the contract implementation echo "Checking contract implementation..." ast-grep --pattern 'function _validateBridgeParams($$$) { $$$ if ($exclusivityEndTime == 0) { $$$ } $$$ }'Length of output: 373
Script:
#!/bin/bash # Search for related test cases and implementation with ripgrep echo "Searching for test cases and implementation..." rg -g "*.{sol,t.sol}" "quoteExclusivitySeconds" -A 3 echo -e "\nChecking for exclusivity validation..." rg -g "*.sol" "exclusivityEndTime.*0" -A 3Length of output: 10977
packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
176-176: LGTM! Changes align with PR objectives.Setting
exclusivityEndTimeto 0 in test fixtures correctly reflects the new default behavior for non-exclusive bridges, which helps prevent potential issues during blockchain reorganization events by using a static value instead ofblock.timestamp.Also applies to: 178-178
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (2)
53-59: LGTM! Changes align with PR objectives.The modification to set
exclusivityEndTimeto 0 for all test transactions aligns with the PR's goal to mitigate blockchain reorganization risks by using a static value instead ofblock.timestampfor non-exclusive bridges.
53-59: Verify test coverage for exclusivity scenarios.While the changes to
exclusivityEndTimeare correct, let's ensure the test coverage is comprehensive for both exclusive and non-exclusive scenarios. The test functionstest_bridge_token_withExclusivity()andtest_bridge_eth_withExclusivity()cover the exclusive case, but we should verify if we need additional assertions for the non-exclusive case.Also applies to: 174-177, 182-185
✅ Verification successful
Test coverage for exclusivity scenarios is comprehensive.
The test coverage is complete with both exclusive and non-exclusive scenarios:
- Non-exclusive scenarios:
test_bridge_token()andtest_bridge_eth()- Exclusive scenarios:
test_bridge_token_withExclusivity()andtest_bridge_eth_withExclusivity()Each test verifies the correct state after bridging using
checkAfterBridgeToken()andcheckAfterBridgeEth()helper functions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for non-exclusive bridge scenarios # Look for test functions that verify exclusivityEndTime behavior # Search for test functions and their assertions rg -U "test_bridge.*\{.*\}" --type solidityLength of output: 78
Script:
#!/bin/bash # Search for test functions related to bridge functionality rg -U "function test_bridge.*?\{.*?\}" -g "*.sol" -A 10 # Also search for exclusivity-related test functions rg -U "function.*?exclusivity.*?\{.*?\}" -g "*.sol" -A 10Length of output: 117
Script:
#!/bin/bash # First, let's find all test files related to FastBridgeV2 fd "FastBridgeV2.*\.t\.sol$" # Then let's look at the content of these test files fd "FastBridgeV2.*\.t\.sol$" -x cat {} # Additionally, search for any mentions of exclusivity in test files rg "exclusivity" -g "*.t.sol"Length of output: 168531
Script:
#!/bin/bash # Search for test functions that verify both exclusive and non-exclusive scenarios rg -B 2 -A 10 "test_bridge.*withExclusivity" packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol # Also search for test functions that verify non-exclusive scenarios rg -B 2 -A 10 "test_bridge_token\(\)" packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol rg -B 2 -A 10 "test_bridge_eth\(\)" packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.solLength of output: 2432
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
180-183: LGTM! Change aligns with PR objectives.The modification correctly addresses the reorg risk by setting exclusivityEndTime to 0 when no exclusivity is intended (quoteRelayer is address(0)), instead of defaulting to block.timestamp.
| @@ -444,7 +447,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { | |||
| revert NativeTokenCallValueNotSupported(); | |||
| } | |||
| // exclusivityEndTime must be in range (0 .. params.deadline] | |||
There was a problem hiding this comment.
Let's update the comment here to reflect that the range now includes zero.
There was a problem hiding this comment.
i actually thought it was incorrect before (0 was not technically in range before, but was specified in the comment) - but now happens to be accurate & left it alone.
There was a problem hiding this comment.
@parodime I meant purely in terms of the exclusive/inclusive range notation: (0 .. X] vs [0 .. X]
There was a problem hiding this comment.
i'm blind - thanks
Deploying sanguine-fe with
|
| Latest commit: |
4d77ec2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ed963cbc.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-exclusoffsetdefzer.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
451-452: Update the comment to use inclusive range notation.The comment should reflect that 0 is included in the valid range.
Apply this change:
- // exclusivityEndTime must be in range [0 .. params.deadline] + // exclusivityEndTime must be in range [0 .. params.deadline]Note: The implementation is correct as it properly validates the range including 0, but the comment's range notation could be clearer.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (3 hunks)
🧰 Additional context used
📓 Learnings (1)
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.
🔇 Additional comments (1)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
180-185: LGTM! The exclusivity logic change aligns with the PR objective.The implementation correctly addresses the reorg risk by:
- Defaulting exclusivityEndTime to 0 for non-exclusive bridges
- Only using block.timestamp when exclusivity is intended (quoteRelayer != address(0))
Previously if bridge was not intended to be exclusive it would still end up with exclusivityEndtime = block.timestamp as a default from excl offset being 0.
This is generally fine & efficient, except it adds reorg risk to even non-exclusive relays whereby the bridge params are dependent upon block.timestamp and will change if reorged into a different block.
This approach will instead pay slightly more gas to default to a static exclusivityEndtime value (0) if no exclusivity is intended - inferred by quoteRelayer being address 0
Summary by CodeRabbit
New Features
exclusivityEndTime.Bug Fixes
Tests