LF-13902 Callbacks for HyperswapV, LaminarV3, XSwap and RabbitSwap [IHyperswapV3Factory v1.0.0,IHyperswapV3QuoterV2 v1.0.0,LiFiDEXAggregator v1.10.0] #1170
Conversation
WalkthroughThis update introduces new DEX integrations to the Changes
Possibly related PRs
Suggested labels
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
lifi-action-bot
changed the title
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
Test Coverage ReportLine Coverage: 83.93% (2289 / 2727 lines) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/Interfaces/IHyperswapV3QuoterV2.sol (2)
8-14: Document struct fields for clarity
Consider adding@paramNatSpec tags for each member ofQuoteExactInputSingleParamsto enhance generated documentation and readability.
16-36: Fix NatSpec tags for parameters
The detailed parameter descriptions lack@paramprefixes, which are required for proper NatSpec parsing. For example:-/// tokenIn The token being swapped in +/// @param tokenIn The token being swapped intest/solidity/Periphery/LiFiDEXAggregator.t.sol (1)
2008-2015: Multi-hop swaps are not supported for HyperswapV3.The test correctly identifies that HyperswapV3 doesn't support multi-hop swaps due to the
require(amountSpecified != 0, 'AS')check in the pool's swap function. SinceProcessOnePoolalways passesamountSpecified = 0, this prevents chaining V3 pools.Consider documenting this limitation in the main contract or creating a tracking issue for future enhancement if multi-hop support is needed.
Would you like me to create an issue to track this limitation or document it in the contract?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deployments/_deployments_log_file.json(2 hunks)deployments/arbitrum.diamond.staging.json(1 hunks)deployments/arbitrum.staging.json(1 hunks)deployments/xdc.staging.json(1 hunks)src/Interfaces/IHyperswapV3Factory.sol(1 hunks)src/Interfaces/IHyperswapV3QuoterV2.sol(1 hunks)src/Periphery/LiFiDEXAggregator.sol(2 hunks)test/solidity/Periphery/LiFiDEXAggregator.t.sol(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
deployments/xdc.staging.json (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
deployments/_deployments_log_file.json (1)
undefined
<retrieved_learning>
Learnt from: 0xDEnYO
PR: #832
File: deployments/_deployments_log_file.json:23712-23720
Timestamp: 2024-12-04T01:59:34.045Z
Learning: In deployments/_deployments_log_file.json, duplicate deployment entries for the same version and address may occur because they correspond to deployments on different networks. These entries are acceptable and should not be flagged as duplicates in future reviews.
</retrieved_learning>
src/Periphery/LiFiDEXAggregator.sol (3)
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/bsc.json:48-48
Timestamp: 2024-10-04T09:00:06.744Z
Learning: The `LiFiDEXAggregator` contract does not include a `swapAndCompare` function.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/bsc.json:48-48
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator` contract does not include a `swapAndCompare` function.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
test/solidity/Periphery/LiFiDEXAggregator.t.sol (2)
Learnt from: mirooon
PR: lifinance/contracts#1117
File: test/solidity/Periphery/LiFiDEXAggregator.t.sol:111-127
Timestamp: 2025-05-06T09:09:38.108Z
Learning: The `setupApechain()` function in LiFiDEXAggregator tests intentionally avoids calling `initTestBase()` to prevent needing to define standard contracts (like ADDRESS_USDC_PROXY and Uniswap) that aren't needed for Apechain-specific tests.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
🪛 GitHub Actions: VersionControlAndAuditVerification
src/Periphery/LiFiDEXAggregator.sol
[error] 1-1: Could not find any logged audit for contract LiFiDEXAggregator in version 1.10.0.
🪛 GitHub Actions: Security Alerts Review
src/Periphery/LiFiDEXAggregator.sol
[error] 542-542: Casting a large uint to int (even at same byte width) may lead to overflow.
[error] 539-539: Calling a function without checking the return value may lead to silent failures.
[error] 539-539: External calls in a loop may lead to denial of service if those calls revert.
[error] 94-94: Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state.
[error] 762-762: Unchecked zero values passed as parameters may be in error or an indication of potentially insecure code design.
[error] 1421-1421: Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert.
[error] 121-121: Using a payable fallback (including receive) with no explicit functionality may indicate incomplete contract logic.
[error] 787-787: External calls in a loop may lead to denial of service if those calls revert.
[error] 777-777: External calls in a loop may lead to denial of service if those calls revert.
[error] 762-762: External calls in a loop may lead to denial of service if those calls revert.
[error] 513-513: External calls in a loop may lead to denial of service if those calls revert.
[error] 492-492: External calls in a loop may lead to denial of service if those calls revert.
[error] 477-477: External calls in a loop may lead to denial of service if those calls revert.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (13)
deployments/arbitrum.diamond.staging.json (1)
179-179: No functional change
The removal of the trailing newline has no impact on the JSON content or deployments.deployments/arbitrum.staging.json (1)
56-57: Add LiFiDEXAggregator deployment address
The new"LiFiDEXAggregator": "0x4E992E3A49c61FDE728926A35BAd715303a8223f"entry correctly specifies the upgraded aggregator contract for version 1.10.0. The comma after the previous"ChainflipFacet"is required for valid JSON.src/Interfaces/IHyperswapV3Factory.sol (3)
1-3: SPDX and pragma are properly declared
The license identifier and Solidity version (^0.8.17) align with the rest of the codebase.
4-7: NatSpec header is clear
The title, author, and version tags provide adequate documentation for this new interface.
8-19: Interface signature looks correct
ThegetPoolfunction is well-documented and matches typical Uniswap‐style factory interfaces. Ensure that this parameter ordering and return type match the on-chain HyperswapV3 factory implementation.src/Interfaces/IHyperswapV3QuoterV2.sol (2)
1-3: SPDX and pragma are properly declared
The license identifier and Solidity version (^0.8.17) are consistent with project standards.
4-7: NatSpec header is clear
The title, author, and version tags give concise metadata for the quoter interface.deployments/xdc.staging.json (1)
1-3: Add XDC staging deployment entry
The newLiFiDEXAggregatoraddress for the XDC network is correctly specified. Please confirm that this minimal JSON file is compatible with the deployment tooling and that other required addresses are inherited or merged appropriately.src/Periphery/LiFiDEXAggregator.sol (2)
42-42: Version update looks good.The version bump to 1.10.0 appropriately reflects the addition of new callback functions for HyperswapV3 and LaminarV3 integrations.
726-732:❓ Verification inconclusive
Verify HyperswapV3 callback security model.
The new
hyperswapV3SwapCallbackfunction delegates directly touniswapV3SwapCallback, relying on thelastCalledPoolvalidation. Please ensure that HyperswapV3 pools are compatible with this security model and that the callback validation is sufficient.Consider adding a comment documenting that this callback is specifically for HyperswapV3 pools and explaining the security model.
🏁 Script executed:
#!/bin/bash # Search for HyperswapV3 interface definitions to understand the expected callback behavior fd -e sol -x grep -l "IHyperswapV3" {} \; | head -10 | xargs -I {} grep -A 10 -B 2 "SwapCallback\|hyperswapV3SwapCallback" {} # Look for any HyperswapV3-specific validation or requirements rg -A 5 -B 5 "HyperswapV3" --type solLength of output: 314
I wasn’t able to locate any HyperswapV3 definitions or usages with the initial search. Let’s broaden our grep to cover all Solidity files and import lines:
🏁 Script executed:
#!/bin/bash # 1. Find any references to “hyperswapV3” in .sol files rg --color=never -g '*.sol' -n 'hyperswapV3' # 2. Look for an IHyperswapV3 interface definition rg --color=never -g '*.sol' -n 'interface IHyperswapV3' # 3. Check imports for any Hyperswap-related files rg --color=never -g '*.sol' -n 'import .*Hyperswap' # 4. Confirm how lastCalledPool is set/used in the aggregator rg --color=never -g '*.sol' -n 'lastCalledPool' # 5. Inspect the uniswapV3SwapCallback implementation rg --color=never -g '*.sol' -n 'function uniswapV3SwapCallback'Length of output: 1617
I still can’t see the callback implementations and doc comments. Let’s dump the surrounding code for both callback functions:
#!/bin/bash # Show 10 lines before & after uniswapV3SwapCallback rg -n -C10 "function uniswapV3SwapCallback" -g '*.sol' # Show 10 lines before & after hyperswapV3SwapCallback rg -n -C10 "function hyperswapV3SwapCallback" -g '*.sol'
Let’s pull the full implementations around both callbacks for clarity:
#!/bin/bash # Dump uniswapV3SwapCallback implementation sed -n '530,630p' src/Periphery/LiFiDEXAggregator.sol # Dump hyperswapV3SwapCallback implementation sed -n '720,780p' src/Periphery/LiFiDEXAggregator.sol
Verify HyperswapV3 callback security model
File:
src/Periphery/LiFiDEXAggregator.sol
Lines: 726–732+ /// @dev Callback entrypoint for HyperswapV3 pools. + /// Delegates to `uniswapV3SwapCallback` and relies on the `lastCalledPool` check for authenticity. function hyperswapV3SwapCallback( int256 amount0Delta, int256 amount1Delta, bytes calldata data ) external { uniswapV3SwapCallback(amount0Delta, amount1Delta, data); }
- Ensure the HyperswapV3 swap entrypoint in this contract sets
lastCalledPoolbefore calling into the pool, exactly as the UniswapV3 flow.- Confirm that HyperswapV3 pools invoke this function with the same callback signature and cannot bypass the
lastCalledPoolcheck.- Add the above NatSpec comment so future readers understand that this callback reuses UniswapV3’s security model but is dedicated to HyperswapV3 pools.
test/solidity/Periphery/LiFiDEXAggregator.t.sol (3)
108-119: Good refactoring to reduce code duplication.The new
_initializeDexAggregatorhelper function effectively consolidates the repeated initialization logic across different test setups.
2136-2143: Multi-hop swaps are also not supported for LaminarV3.LaminarV3 has the same limitation as HyperswapV3 regarding multi-hop swaps. This consistency suggests it might be a broader V3-style DEX integration pattern.
The single-hop test coverage is comprehensive and follows good testing practices.
1847-2144: Tests were actually added despite PR description.The PR description states that "tests covering the new functionality have not been added," but comprehensive test contracts
LiFiDexAggregatorHyperswapV3TestandLiFiDexAggregatorLaminarV3Testwere actually implemented. Please update the PR description to reflect the actual changes.The test coverage includes:
- Single-hop swaps from user funds
- Single-hop swaps from aggregator funds
- Appropriate handling of the multi-hop limitation
Likely an incorrect or invalid review comment.
Which Jira task belongs to this PR?
LF-13902
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)