LF-14567 Add KatanaV3 to LDA [IKatanaV3AggregateRouter v1.0.0,IKatanaV3Governance v1.0.0,IKatanaV3Pool v1.0.0,LibAsset v2.1.2,LiFiDEXAggregator v1.12.0] #1283
Conversation
## Walkthrough
This change introduces KatanaV3 pool support into the DEX aggregator by adding new interface definitions, updating the aggregator contract to handle KatanaV3 swaps, and extending the test suite to cover this functionality. Additional deployment metadata and contract address updates for the Ronin network are included. Minor visibility and formatting adjustments are made in utility libraries and tests. An audit entry for LiFiDEXAggregator version 1.12.0 and LibAsset version 2.1.2 is added. The version control workflow logic is refined to improve detection of relevant version bumps.
## Changes
| Cohort / File(s) | Change Summary |
|----------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| **KatanaV3 Interface Additions**<br>`src/Interfaces/KatanaV3/IKatanaV3AggregateRouter.sol`, `src/Interfaces/KatanaV3/IKatanaV3Governance.sol`, `src/Interfaces/KatanaV3/IKatanaV3Pool.sol` | Adds new Solidity interfaces for KatanaV3: an aggregate router with an `execute` function, governance interface with `getRouter()`, and pool interface declaring governance, tokens, fee, and swap functions. These interfaces define interaction points with KatanaV3 contracts. |
| **DEX Aggregator KatanaV3 Support**<br>`src/Periphery/LiFiDEXAggregator.sol` | Extends the LiFiDEXAggregator contract to support KatanaV3 pools by adding new pool type constants, swap command bytes, and a private swap function `swapKatanaV3`. The main swap logic is updated to handle KatanaV3 pools by routing calls accordingly. Token transfers and swap path encoding follow UniswapV3 conventions. The contract version annotation is bumped from 1.11.0 to 1.12.0. |
| **KatanaV3 Aggregator Tests**<br>`test/solidity/Periphery/LiFiDEXAggregator.t.sol` | Adds a new test contract `LiFiDexAggregatorKatanaV3Test` extending the existing aggregator tests. It sets up a Ronin fork and tests single-pool swaps from user and aggregator funds, verifies reverts on invalid pool or recipient addresses, and disables multi-hop tests due to KatanaV3 incompatibility with zero-amount hops. The `PoolType` enum is extended with `KatanaV3` = 10. Tests assert correct token transfers and revert reasons. |
| **Deployment Metadata and Address Updates**<br>`deployments/_deployments_log_file.json`, `deployments/ronin.diamond.json`, `deployments/ronin.json`, `config/dexs.json` | Updates Ronin network deployment metadata by removing older "ronin" production entries and adding a new version 1.12.0 deployment with detailed constructor args and empty ZK_SOLC_VERSION fields. Updates contract addresses for `LiFiDEXAggregator` and `GasZipPeriphery` in deployment and config files to new values. No other networks or environments are affected. |
| **Asset Library Visibility and Version**<br>`src/Libraries/LibAsset.sol` | Updates the version annotation from 2.1.1 to 2.1.2 and changes the visibility of `transferERC20` function from `private` to `internal` without modifying its logic. |
| **Test Import Formatting**<br>`test/solidity/Facets/ChainflipFacet.t.sol` | Splits a combined import statement for error types into two separate import lines for improved clarity and formatting. No functional or usage changes. |
| **Audit Log Update**<br>`audit/auditLog.json` | Adds a new audit entry "audit20250728" documenting an audit by Sujith Somraaj on July 28, 2025. Associates the LiFiDEXAggregator version 1.12.0 and LibAsset version 2.1.2 with this audit entry, extending audit history and version mapping. |
| **CI Workflow Logic Refinement**<br>`.github/workflows/versionControlAndAuditCheck.yml` | Refines the GitHub Actions version control workflow by improving how code changes and version updates are detected. Uses explicit base branch diffs, filters out non-relevant changes (comments, pragma, license), fetches old version from base branch content, and consolidates logic for version bump verification and audit marking. Removes legacy commented code and enhances logging clarity. |
## Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~15–20 minutes
## Possibly related PRs
- lifinance/contracts#1218: Adds support for Izumi and SyncSwap pools in the LiFiDEXAggregator contract, related by extending aggregator swap support for new pool types.
- lifinance/contracts#1225: Adds initial Ronin network configuration and deployment entries; related by handling Ronin deployment metadata and addresses.
- lifinance/contracts#1053: Improves version control workflow logic for detecting relevant code changes and enforcing version bumps, related to `.github/workflows/versionControlAndAuditCheck.yml`.
## Suggested labels
`AuditNotRequired`Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lifi-action-bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/Interfaces/KatanaV3/IKatanaV3Governance.sol (1)
1-9: Interface looks good – minor NatSpec nitpickConsider naming the return parameter for clarity:
- function getRouter() external view returns (address); + /// @return router Address of the Katana V3 Router + function getRouter() external view returns (address router);Purely cosmetic; feel free to ignore.
script/deploy/safe/deploy-safe.ts (1)
51-53: Place__dirnameshim directly after imports for clarityThe ESM shim is perfect functionally, but locating the declaration right after the import block (before any other statements) keeps all environment-setup code in one place and avoids the “magic constant in the middle” smell.
-import { dirname, join } from 'path' -import { fileURLToPath } from 'url' +import { dirname, join } from 'path' +import { fileURLToPath } from 'url' + +// --------------------------------------------------------------------------- +// ESM equivalents of __filename / __dirname +// --------------------------------------------------------------------------- +const __filename = fileURLToPath(import.meta.url) +const __dirname = dirname(__filename)Relocating these two lines prevents accidental re-definitions later and keeps top-level context clear.
Also applies to: 80-83
src/Interfaces/KatanaV3/IKatanaV3Pool.sol (1)
4-19: Optional: add per-function NatSpec for clearer docsThe interface is minimal but public; brief
@noticeonswap,token0, etc., would help integrators.src/Periphery/LiFiDEXAggregator.sol (2)
87-87: Remove unused error declarationThe
KatanaV3SwapUnexpectederror is declared but never used in the implementation. Since KatanaV3 doesn't use the callback pattern withlastCalledPoolvalidation (unlike UniswapV3 or Algebra), this error can be removed.-error KatanaV3SwapUnexpected();
917-917: Document slippage protection strategyThe
amountOutMinis set to 0, which provides no slippage protection at this level. While the comment mentions handling at a higher level, this should be more prominently documented.- uint256(0), // amountOutMin (0, as we handle slippage at higher level) + uint256(0), // amountOutMin: Set to 0 - slippage protection MUST be handled by processRouteInternal's amountOutMin parameter
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/dexs.json(1 hunks)deployments/_deployments_log_file.json(2 hunks)deployments/ronin.diamond.json(1 hunks)deployments/ronin.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- deployments/ronin.json
- config/dexs.json
🚧 Files skipped from review as they are similar to previous changes (1)
- deployments/_deployments_log_file.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 0xDEnYO
PR: lifinance/contracts#1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: In the LiFi contracts, token pair validation is handled by third-party contracts, and route discovery including liquidity checks are performed off-chain. The contract only receives pre-validated routes, making token pair and liquidity validation tests unnecessary at the contract level.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1237
File: conventions.md:56-60
Timestamp: 2025-07-03T01:44:43.968Z
Learning: Always consult the conventions.md file for the latest rules and conventions when reviewing PRs or code changes in the lifinance/contracts repository. Make suggestions when code changes do not match the documented conventions in this file.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/blast.json:27-27
Timestamp: 2024-10-04T09:06:19.927Z
Learning: Audit references for older contracts deployed over 2 months ago, including `LiFiDEXAggregator.sol`, will be added early next year (estimated).
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/blast.json:27-27
Timestamp: 2024-10-09T03:47:21.269Z
Learning: Audit references for older contracts deployed over 2 months ago, including `LiFiDEXAggregator.sol`, will be added early next year (estimated).
Learnt from: mirooon
PR: lifinance/contracts#1283
File: src/Periphery/LiFiDEXAggregator.sol:877-881
Timestamp: 2025-07-17T11:24:43.164Z
Learning: KatanaV3 pools (being UniV3-style implementations) have a requirement that amountSpecified != 0, which means they cannot work with the processOnePool command that passes amountSpecified = 0. This makes KatanaV3 incompatible with multi-hop swaps where a second hop would use INTERNAL_INPUT_SOURCE.
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/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `deployments/*.diamond.json` and `deployments/*.json` files, the `LiFiDEXAggregator` contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/worldchain.diamond.json:40-43
Timestamp: 2024-11-26T01:16:27.721Z
Learning: Version inconsistencies of 'GenericSwapFacetV3' across deployment configurations are acceptable if they are only due to differences in Solidity pragma directives.
Learnt from: 0xDEnYO
PR: lifinance/contracts#832
File: src/Periphery/LiFiDEXAggregator.sol:578-697
Timestamp: 2024-12-04T02:05:41.355Z
Learning: Unit tests are not required for newly added swap callback functions in `LiFiDEXAggregator.sol`.
Learnt from: ezynda3
PR: lifinance/contracts#937
File: config/dexs.json:95-99
Timestamp: 2025-01-21T11:04:46.116Z
Learning: In the dexs.json configuration, utility contracts (like FeeCollector, LiFuelFeeCollector, TokenWrapper) can be treated as DEX contracts since they are handled similarly in the system's logic.
Learnt from: ezynda3
PR: lifinance/contracts#861
File: config/dexs.json:748-752
Timestamp: 2024-11-21T08:39:29.530Z
Learning: In the 'worldchain' network, the addresses `0x50D5a8aCFAe13Dceb217E9a071F6c6Bd5bDB4155`, `0x8f023b4193a6b18C227B4a755f8e28B3D30Ef9a1`, and `0x603a538477d44064eA5A5d8C345b4Ff6fca1142a` are used as DEXs and should be included in `config/dexs.json`.
deployments/ronin.diamond.json (19)
Learnt from: 0xDEnYO
PR: #1109
File: deployments/worldchain.diamond.json:84-85
Timestamp: 2025-04-21T03:15:12.236Z
Learning: In deployment JSON files that contain "diamond" in their filename (located in the deployments folder), periphery contracts may have empty string values for their addresses. This indicates that the contract is not deployed on that particular chain and should not be flagged as an issue during code reviews.
Learnt from: 0xDEnYO
PR: #1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs involving address updates like the RelayFacet to Worldchain, verify the actual presence of entries in files before reporting issues. The RelayFacet exists in the diamond log file and the PR diff already contains the necessary address change.
Learnt from: 0xDEnYO
PR: #819
File: deployments/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the deployments/*.diamond.json and deployments/*.json files, the LiFiDEXAggregator contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.
Learnt from: 0xDEnYO
PR: #782
File: deployments/base.diamond.json:148-148
Timestamp: 2024-11-25T09:05:43.045Z
Learning: In deployment configuration files (e.g., deployments/base.diamond.json), empty addresses for contracts like Permit2Proxy may be placeholders and will be updated after approvals are released from the multisig safe.
Learnt from: 0xDEnYO
PR: #1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs updating contract addresses (like RelayFacet on Worldchain), verify the presence of entries in all relevant files (worldchain.json, worldchain.diamond.json, _deployments_log_file.json) before reporting inconsistencies. The RelayFacet entry exists in all required deployment files with the correct address.
Learnt from: 0xDEnYO
PR: #782
File: deployments/cronos.diamond.json:67-67
Timestamp: 2024-11-26T01:17:13.212Z
Learning: In the deployments/cronos.diamond.json file, the Permit2Proxy address is intentionally left empty because Permit2Proxy is not deployed on the Cronos network.
Learnt from: ezynda3
PR: #806
File: deployments/_deployments_log_file.json:5780-5793
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The owner address 0x11f11121df7256c40339393b0fb045321022ce44 and the DiamondCutFacet address 0xd5cf40a2a18b633cfd6a1ae16d1771596498cf83 in the LiFiDiamond deployment on xlayer are correct and should not be flagged as issues, even if they are not referenced in the Solidity files.
Learnt from: 0xDEnYO
PR: #1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: 0xDEnYO
PR: #807
File: deployments/bsc.diamond.staging.json:100-101
Timestamp: 2024-10-24T06:17:25.211Z
Learning: In deployments/bsc.diamond.staging.json, the trailing comma after the last property in the Periphery object is acceptable and should not be flagged as a JSON formatting error.
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.
Learnt from: 0xDEnYO
PR: #1140
File: config/permit2Proxy.json:3-3
Timestamp: 2025-07-15T05:05:28.134Z
Learning: In config/permit2Proxy.json, the addresses represent the Permit2 contract addresses on each chain, NOT the Permit2Proxy contract addresses. These Permit2 addresses are used as constructor arguments when deploying the Permit2Proxy contract. The Permit2Proxy acts as a wrapper/proxy that interacts with the underlying Permit2 contract on each network.
Learnt from: 0xDEnYO
PR: #992
File: script/deploy/_targetState.json:25-32
Timestamp: 2025-02-13T03:04:19.306Z
Learning: Duplicate contract names (e.g., ERC20Proxy, Executor, etc.) in _targetState.json are expected when they appear across different network configurations, as they represent the same contract type deployed on multiple networks.
Learnt from: ezynda3
PR: #924
File: script/deploy/zksync/DeployExecutor.s.sol:48-48
Timestamp: 2025-01-28T14:26:29.054Z
Learning: The Executor contract constructor accepts two parameters:
- _erc20Proxy: The address of the ERC20Proxy contract
- _owner: The owner address (passed to WithdrawablePeriphery)
Learnt from: 0xDEnYO
PR: #1112
File: deployments/soneium.diamond.json:81-81
Timestamp: 2025-04-22T09:04:44.244Z
Learning: In the lifinance/contracts repository, it's normal and expected for periphery contracts to have empty address values in deployment files when they are not deployed on a particular network. This applies to all periphery contracts including ReceiverChainflip, ReceiverStargateV2, and others. PRs should not flag empty periphery contract addresses as issues.
Learnt from: 0xDEnYO
PR: #807
File: src/Periphery/GasZipPeriphery.sol:4-14
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In GasZipPeriphery.sol, LibUtil and Validatable are used, so ensure not to suggest their removal in future reviews.
Learnt from: ezynda3
PR: #937
File: config/dexs.json:95-99
Timestamp: 2025-01-21T11:04:46.116Z
Learning: In the dexs.json configuration, utility contracts (like FeeCollector, LiFuelFeeCollector, TokenWrapper) can be treated as DEX contracts since they are handled similarly in the system's logic.
Learnt from: 0xDEnYO
PR: #807
File: src/Periphery/GasZipPeriphery.sol:57-62
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the GasZipPeriphery contract, it's acceptable to let low-level calls like liFiDEXAggregator.call fail without explicit error handling, as failing the entire transaction is sufficient and saves gas.
Learnt from: 0xDEnYO
PR: #782
File: src/Periphery/Permit2Proxy.sol:75-108
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the Permit2Proxy contract (src/Periphery/Permit2Proxy.sol), reentrancy protection is not necessary for functions like callDiamondWithEIP2612Signature when calling our own trusted diamond contract (LIFI_DIAMOND).
Learnt from: 0xDEnYO
PR: #819
File: deployments/base.diamond.json:123-123
Timestamp: 2024-10-04T09:01:56.514Z
Learning: In the lifinance/contracts repository, it's acceptable to retain references to the old LiFuelFeeCollector address (0xc4f7A34b8d283f66925eF0f5CCdFC2AF3030DeaE) in deployment files when updating them is not necessary.
🪛 Gitleaks (8.27.2)
deployments/ronin.diamond.json
72-72: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Which Jira task belongs to this PR?
LF-14567
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)