feat(contracts-rfq): Multicall in FastBridgeV2 [SLT-369] #3315
Conversation
WalkthroughThe 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3315 +/- ##
===================================================
- Coverage 31.92469% 28.74454% -3.18015%
===================================================
Files 238 184 -54
Lines 14553 12131 -2422
Branches 356 82 -274
===================================================
- Hits 4646 3487 -1159
+ Misses 9614 8361 -1253
+ Partials 293 283 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
Line range hint
110-112: Potential Integer Overflow in Exclusivity End Time Calculation.In
_validateBridgeParams, castingblock.timestamptoint256and addingparamsV2.quoteExclusivitySecondscould result in integer overflow ifblock.timestampexceeds2^255. Although this is unlikely in the near future, it's safer to useuint256arithmetic to prevent potential issues.Apply this diff to use
uint256instead ofint256:-function _validateBridgeParams( - BridgeParams memory params, - BridgeParamsV2 memory paramsV2, - int256 exclusivityEndTime -) internal +) internal view +{ + uint256 exclusivityEndTime = block.timestamp + paramsV2.quoteExclusivitySeconds; // Check V1 (legacy) params ... - if (exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline)) { + if (exclusivityEndTime > params.deadline) { revert ExclusivityParamsIncorrect(); } }
Line range hint
138-154: Handle Fee-on-Transfer Tokens Appropriately During Relay.In the
relayfunction, for tokens with transfer fees, the recipient may receive less than the expectedamount, potentially causing issues if the full amount is required. Consider implementing logic to account for fee-on-transfer tokens or restricting such tokens to prevent unexpected behavior.Would you like assistance in modifying the relay logic to handle fee-on-transfer tokens effectively?
Line range hint
180-192: Potential Reentrancy Risk in_checkedCallRecipient.The
_checkedCallRecipientfunction performs an external call usingAddress.functionCallWithValue, which could introduce reentrancy risks. Although state changes precede this call, consider adding thenonReentrantmodifier or implementing reentrancy guards to enhance security.Apply the
nonReentrantmodifier to therelayor_checkedCallRecipientfunction to mitigate potential reentrancy vulnerabilities.+import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; ... -contract FastBridgeV2 is Admin, MulticallTarget, IFastBridgeV2, IFastBridgeV2Errors { +contract FastBridgeV2 is Admin, MulticallTarget, ReentrancyGuard, IFastBridgeV2, IFastBridgeV2Errors { ... -function relay(bytes calldata request, address relayer) public payable { +function relay(bytes calldata request, address relayer) public payable nonReentrant {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (1 hunks)
- packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol (1 hunks)
- packages/contracts-rfq/test/integration/MulticallTarget.t.sol (7 hunks)
🧰 Additional context used
📓 Learnings (2)
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/integration/FastBridgeV2.MulticallTarget.t.sol (3)
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/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 (12)
packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol (3)
11-11: Confirm the use ofaddress(this)inFastBridgeV2constructor.Ensure that passing
address(this)to theFastBridgeV2constructor is appropriate in this test context. Verify that it correctly initializes the contract as intended.
22-35: Assignment of fields is acceptable per previous guidance.While the field-by-field assignment from
bridgeTxtobridgeTxV2is repetitive, this approach aligns with previous learnings that code duplication in test functions is acceptable to maintain readability and maintainability.
34-34: Verify thatexclusivityEndTimeset toblock.timestampis appropriate.Ensure that setting
exclusivityEndTimeto the current block timestamp reflects the intended test scenario for transaction exclusivity.packages/contracts-rfq/test/integration/MulticallTarget.t.sol (7)
36-38: Addition of encoded bridge transaction variables is appropriateThe introduction of internal byte arrays
encodedBridgedTokenTx,encodedProvenEthTx, andencodedRemoteTokenTxenhances efficiency by storing pre-encoded transaction data for use in tests, streamlining transaction handling.
53-53: Ensure allprovefunction calls are updatedIn the
setUpfunction, the callprove(encodedProvenEthTx);reflects the updatedprovefunction signature acceptingbytesinstead of aBridgeTransactionstruct. Verify that all other calls toprovewithin the test suite have been updated accordingly to prevent potential issues.
123-128: Computing transaction IDs from encoded data ensures consistencyUsing
keccak256on the encoded transactions to computebridgedTokenTxId,provenEthTxId, andremoteTokenTxIdaligns the identifiers directly with the transaction data, promoting consistency and reducing discrepancies.
149-151: Updatedprovefunction signature aligns with contract changesThe
provefunction now acceptsbytes memory encodedBridgeTx, matching the updated interface inFastBridgeV2. The call toIFastBridge(fastBridge).prove(encodedBridgeTx, hex"01");correctly passes the encoded transaction data.
156-158: Correct use ofabi.encodeCallwith encoded transactionsIn the
getDatafunction, the use ofabi.encodeCallwith the encoded transactions prepares the data array appropriately for multicall operations, ensuring function calls are correctly encoded.
168-169: Function calls updated to use encoded transactionsIn
test_sequentialExecution, the function calls toproveandclaimnow useencodedBridgedTokenTxandencodedProvenEthTx, respectively, consistent with the updated function signatures and enhancing test accuracy.
98-99:⚠️ Potential issueSwapping
originSenderanddestRecipientmay affect test logicIn
provenEthTx, swappingoriginSender: useranddestRecipient: userRemotechanges the direction of the transaction. Confirm that this change is intentional and aligns with the intended behavior of the test cases.packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
16-19: Review Security Implications of InheritingMulticallTarget.The addition of
MulticallTargetas a base contract introduces multicall capabilities toFastBridgeV2. Ensure that this inclusion does not inadvertently expose any sensitive functions or allow unauthorized access. Review all inherited functions for potential security risks and apply appropriate access controls if necessary.
Line range hint
41-46: Ensure Access Control is Appropriately Applied to theproveFunction.The
provefunction now includes theonlyRole(RELAYER_ROLE)modifier. Verify that all legitimate relayers have been assigned theRELAYER_ROLEand that unauthorized parties cannot access this function. Ensure that this change aligns with the desired access control policies.
packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol
Show resolved
Hide resolved
Deploying sanguine-fe with
|
| Latest commit: |
b6b8708
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://82e0b3f5.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-multicall.sanguine-fe.pages.dev |
* add manyActions multicall test * increase manyAction count * comment cleanup
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/contracts-rfq/test/integration/MulticallTarget.t.sol (3)
73-76: Add input validation to prevent potential overflows.While the function works correctly, consider adding validation for the
amountUnitsparameter to prevent potential overflows when converting to wei.function dealTokens(address to, uint8 amountUnits) public { + require(to != address(0), "Invalid address"); + require(amountUnits > 0, "Amount must be positive"); uint256 amountWei = amountUnits * 1 ether; token.mint(to, amountWei); deal(to, amountWei);
132-136: Add NatSpec documentation for the struct.Consider adding NatSpec documentation to explain the purpose and usage of this struct, which will improve code maintainability.
+/// @notice A struct that combines a bridge transaction with its encoded form and transaction ID +/// @dev Used to track and verify transaction states throughout the test suite struct TestBridgeTransactionWithMetadata { IFastBridge.BridgeTransaction transaction; bytes encodedData; bytes32 txId; }
13-13: Consider parameterizing chain configuration.The contract uses hardcoded chain IDs (1337, 7331). Consider making these configurable through constructor parameters or a configuration struct to improve test suite flexibility across different environments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/contracts-rfq/test/integration/MulticallTarget.t.sol (8 hunks)
🔇 Additional comments (1)
packages/contracts-rfq/test/integration/MulticallTarget.t.sol (1)
36-38: LGTM! Good optimization with cached encoded transactions.Storing encoded transactions as state variables is a good practice that reduces redundant encoding operations and improves test execution efficiency.
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
FastBridgeV2contract with multicall capabilities for improved transaction handling.BridgeParamsV2.FastBridgeV2.Bug Fixes
Tests
FastBridgeV2to validate the updated functionality and transaction encoding.