Harmonize custom version tags [CalldataVerificationFacet v1.3.0,IAllBridge v1.0.0,ICBridge v1.0.0,ICircleBridgeProxy v1.0.0,IConnextHandler v1.0.0,IDiamondCut v2.0.0,IDiamondLoupe v1.0.0,IDlnSource v1.0.0,IERC165 v1.0.0,IERC173 v1.0.0,IERC20Proxy v1.0.0,IExecutor v1.0.0,IGasZip v1.0.0,IGatewayRouter v1.0.0,IHopBridge v1.0.0,IL1StandardBridge v1.0.0,ILiFi v1.0.0,IMayan v1.0.0,IOmniBridge v1.0.0,IRootChainManager v1.0.0,ISquidMulticall v1.0.0,ISquidRouter v1.0.0,IStargate v1.0.0,ISymbiosisMetaRouter v1.0.0,IThorSwap v1.0.0,IXDaiBridge v1.0.0,IXDaiBridgeL2 v1.0.0] #1021
Conversation
…stargateV1-and-Amarok
WalkthroughA new JSON configuration file is introduced for Stargate endpoints, and import paths in demo scripts have been updated to reflect a new directory structure. Legacy configuration data in the existing Stargate config has been removed. Several Solidity interface files now include enhanced documentation (titles, authors, reordered version comments) and updated licenses in selected files. Additionally, validations for Amarok and legacy Stargate functionalities were removed from the calldata verification facet, and corresponding tests were deleted, while a health check script was adjusted. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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
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: 1
🔭 Outside diff range comments (2)
src/Facets/CalldataVerificationFacet.sol (2)
6-6:⚠️ Potential issueRemove duplicate import of AcrossFacetV3.
The AcrossFacetV3 import is duplicated.
import { ILiFi } from "../Interfaces/ILiFi.sol"; import { LibSwap } from "../Libraries/LibSwap.sol"; import { AcrossFacetV3 } from "./AcrossFacetV3.sol"; import { StargateFacetV2 } from "./StargateFacetV2.sol"; -import { AcrossFacetV3 } from "./AcrossFacetV3.sol"; import { CelerIMFacetBase, CelerIM } from "lifi/Helpers/CelerIMFacetBase.sol";Also applies to: 8-8
354-412:⚠️ Potential issueRemove duplicate AcrossV3 validation logic.
The AcrossV3 validation logic is duplicated. The entire block from lines 383-412 is identical to the block from lines 354-381.
// Case: AcrossV3 if (selector == AcrossFacetV3.startBridgeTokensViaAcrossV3.selector) { (, AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( callData.slice(4, callData.length - 4), (ILiFi.BridgeData, AcrossFacetV3.AcrossV3Data) ); return keccak256(dstCalldata) == keccak256(acrossV3Data.message) && keccak256(callTo) == keccak256(abi.encode(acrossV3Data.receiverAddress)); } if ( selector == AcrossFacetV3.swapAndStartBridgeTokensViaAcrossV3.selector ) { (, , AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( callData.slice(4, callData.length - 4), ( ILiFi.BridgeData, LibSwap.SwapData[], AcrossFacetV3.AcrossV3Data ) ); return keccak256(dstCalldata) == keccak256(acrossV3Data.message) && keccak256(callTo) == keccak256(abi.encode(acrossV3Data.receiverAddress)); } - // --------------------------------------- - // Case: AcrossV3 - if (selector == AcrossFacetV3.startBridgeTokensViaAcrossV3.selector) { - (, AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( - callData.slice(4, callData.length - 4), - (ILiFi.BridgeData, AcrossFacetV3.AcrossV3Data) - ); - - return - keccak256(dstCalldata) == keccak256(acrossV3Data.message) && - keccak256(callTo) == - keccak256(abi.encode(acrossV3Data.receiverAddress)); - } - if ( - selector == - AcrossFacetV3.swapAndStartBridgeTokensViaAcrossV3.selector - ) { - (, , AcrossFacetV3.AcrossV3Data memory acrossV3Data) = abi.decode( - callData.slice(4, callData.length - 4), - ( - ILiFi.BridgeData, - LibSwap.SwapData[], - AcrossFacetV3.AcrossV3Data - ) - ); - return - keccak256(dstCalldata) == keccak256(acrossV3Data.message) && - keccak256(callTo) == - keccak256(abi.encode(acrossV3Data.receiverAddress)); - }
🧹 Nitpick comments (5)
src/Interfaces/IL1StandardBridge.sol (1)
4-7: Standardize the version tag format.While the documentation additions are great, consider using the standard NatSpec
@versiontag instead of@custom:versionto align with common practices.Apply this diff:
/// @title Interface for L1StandardBridge /// @author LI.FI (https://li.fi) -/// @custom:version 1.0.0 +/// @version 1.0.0archive/config/stargateV1.json (3)
134-185: Composers Addresses Review
The"composers"object maps various network identifiers to their respective addresses. The mixture of valid addresses and placeholder zero addresses appears intentional. As a good-to-have refinement, consider ensuring that any non-placeholder address is correctly checksummed (if required by your standards) and documented so that future maintainers can validate them more easily.
186-231: Routers Addresses Consistency
The"routers"object is comprehensive and lists router addresses for many networks. Verify that each address is correct according to the latest deployment details and consider standardizing the casing/format of these hexadecimal addresses for consistency. Additionally, a good-to-have improvement might be to include a comment or supplementary documentation (outside the JSON file, since JSON does not support comments) indicating the source or audit status of these addresses.
1-231: Overall Configuration Enhancement Suggestion
Since the PR objective mentions "Harmonize custom version tags," consider including a configuration version field at the root level. This can help track changes and ensure compatibility in future upgrades. For example, you might insert the following line after the opening brace:{ + "configVersion": "1.0",This addition would facilitate smoother version management and documentation alignment across different configuration files.
src/Facets/CalldataVerificationFacet.sol (1)
269-416: Consider refactoring validateDestinationCalldata for better maintainability.The
validateDestinationCalldatafunction is quite long and contains repetitive validation patterns for different bridges. Consider refactoring it to use a map of validators or a switch pattern to improve maintainability and reduce the likelihood of errors like the duplicate AcrossV3 validation.Here's a suggested approach:
// Define a validator type type Validator = (bytes memory callData, bytes calldata callTo, bytes calldata dstCalldata) => bool; // Create a mapping of selectors to validators mapping(bytes4 => Validator) private validators; // Initialize validators in the constructor constructor() { validators[StargateFacetV2.startBridgeTokensViaStargate.selector] = validateStargateV2; validators[StargateFacetV2.swapAndStartBridgeTokensViaStargate.selector] = validateStargateV2WithSwap; // ... add other validators } // Example validator function function validateStargateV2(bytes memory callData, bytes calldata callTo, bytes calldata dstCalldata) private pure returns (bool) { (, StargateFacetV2.StargateData memory stargateDataV2) = abi.decode( callData.slice(4, callData.length - 4), (ILiFi.BridgeData, StargateFacetV2.StargateData) ); return keccak256(dstCalldata) == keccak256(stargateDataV2.sendParams.composeMsg) && _compareBytesToBytes32CallTo(callTo, stargateDataV2.sendParams.to); } // Main validation function function validateDestinationCalldata( bytes calldata data, bytes calldata callTo, bytes calldata dstCalldata ) external pure returns (bool isValid) { bytes memory callData = data; if (bytes4(data[:4]) == StandardizedCallFacet.standardizedCall.selector) { callData = abi.decode(data[4:], (bytes)); } bytes4 selector = abi.decode(callData, (bytes4)); // Get and execute the validator Validator validator = validators[selector]; if (validator != null) { return validator(callData, callTo, dstCalldata); } return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
archive/config/stargateV1.json(1 hunks)archive/scripts/demoScripts/demoDeBridge.ts(1 hunks)archive/scripts/demoScripts/demoSynapseBridge.ts(1 hunks)archive/src/Interfaces/IDeBridgeGate.sol(1 hunks)archive/src/Interfaces/IHyphenRouter.sol(1 hunks)archive/src/Interfaces/IMultichainRouter.sol(1 hunks)archive/src/Interfaces/IMultichainToken.sol(1 hunks)archive/src/Interfaces/IStargateRouter.sol(1 hunks)archive/src/Interfaces/ISynapseRouter.sol(1 hunks)archive/src/Interfaces/ITeleportGateway.sol(1 hunks)archive/src/Interfaces/ITokenMessenger.sol(1 hunks)archive/src/Interfaces/ITransactionManager.sol(1 hunks)config/stargate.json(1 hunks)script/deploy/healthCheck.ts(0 hunks)src/Facets/CalldataVerificationFacet.sol(1 hunks)src/Interfaces/IAcrossSpokePool.sol(1 hunks)src/Interfaces/IAllBridge.sol(1 hunks)src/Interfaces/ICBridge.sol(1 hunks)src/Interfaces/ICircleBridgeProxy.sol(1 hunks)src/Interfaces/IConnextHandler.sol(1 hunks)src/Interfaces/IDiamondCut.sol(1 hunks)src/Interfaces/IDiamondLoupe.sol(1 hunks)src/Interfaces/IDlnSource.sol(1 hunks)src/Interfaces/IERC165.sol(1 hunks)src/Interfaces/IERC173.sol(1 hunks)src/Interfaces/IERC20Proxy.sol(1 hunks)src/Interfaces/IExecutor.sol(1 hunks)src/Interfaces/IGasZip.sol(1 hunks)src/Interfaces/IGatewayRouter.sol(1 hunks)src/Interfaces/IHopBridge.sol(1 hunks)src/Interfaces/IL1StandardBridge.sol(1 hunks)src/Interfaces/ILiFi.sol(1 hunks)src/Interfaces/IMayan.sol(1 hunks)src/Interfaces/IOmniBridge.sol(1 hunks)src/Interfaces/IRootChainManager.sol(1 hunks)src/Interfaces/ISquidMulticall.sol(1 hunks)src/Interfaces/ISquidRouter.sol(1 hunks)src/Interfaces/IStargate.sol(1 hunks)src/Interfaces/ISymbiosisMetaRouter.sol(1 hunks)src/Interfaces/IThorSwap.sol(1 hunks)src/Interfaces/IXDaiBridge.sol(1 hunks)src/Interfaces/IXDaiBridgeL2.sol(1 hunks)test/solidity/Facets/CalldataVerificationFacet.t.sol(0 hunks)
💤 Files with no reviewable changes (2)
- script/deploy/healthCheck.ts
- test/solidity/Facets/CalldataVerificationFacet.t.sol
✅ Files skipped from review due to trivial changes (38)
- archive/scripts/demoScripts/demoDeBridge.ts
- archive/src/Interfaces/IHyphenRouter.sol
- src/Interfaces/ISquidMulticall.sol
- archive/src/Interfaces/IDeBridgeGate.sol
- archive/src/Interfaces/IMultichainToken.sol
- src/Interfaces/IAcrossSpokePool.sol
- src/Interfaces/IERC20Proxy.sol
- src/Interfaces/IGasZip.sol
- src/Interfaces/IThorSwap.sol
- src/Interfaces/IXDaiBridgeL2.sol
- src/Interfaces/IXDaiBridge.sol
- src/Interfaces/IExecutor.sol
- src/Interfaces/IERC173.sol
- src/Interfaces/ISymbiosisMetaRouter.sol
- archive/src/Interfaces/ISynapseRouter.sol
- src/Interfaces/ILiFi.sol
- src/Interfaces/IConnextHandler.sol
- archive/src/Interfaces/ITransactionManager.sol
- src/Interfaces/IMayan.sol
- src/Interfaces/IAllBridge.sol
- archive/src/Interfaces/ITokenMessenger.sol
- src/Interfaces/ICircleBridgeProxy.sol
- src/Interfaces/IStargate.sol
- src/Interfaces/IDlnSource.sol
- archive/src/Interfaces/IMultichainRouter.sol
- src/Interfaces/IOmniBridge.sol
- src/Interfaces/IRootChainManager.sol
- archive/scripts/demoScripts/demoSynapseBridge.ts
- config/stargate.json
- src/Interfaces/IDiamondCut.sol
- src/Interfaces/ICBridge.sol
- src/Interfaces/IHopBridge.sol
- src/Interfaces/IGatewayRouter.sol
- archive/src/Interfaces/ITeleportGateway.sol
- src/Interfaces/IERC165.sol
- src/Interfaces/ISquidRouter.sol
- src/Interfaces/IDiamondLoupe.sol
- archive/src/Interfaces/IStargateRouter.sol
🧰 Additional context used
🧠 Learnings (1)
src/Facets/CalldataVerificationFacet.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#1003
File: src/Facets/CalldataVerificationFacet.sol:16-16
Timestamp: 2025-02-19T08:30:20.501Z
Learning: The project does not actively maintain a changelog, and suggestions about adding changelog entries should be avoided.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (3)
src/Interfaces/IL1StandardBridge.sol (1)
1-1: LGTM! Good choice of license.The change from BUSL-1.1 to MIT license is appropriate for an interface file, making it more accessible for integration by other projects.
archive/config/stargateV1.json (2)
1-71: Endpoint IDs Configuration Check
The"endpointIds"array is structured correctly with each entry providing a numeric"chainId"and its corresponding"endpointId". Ensure that all endpoint IDs are accurate and truly reflect the deployed environment details. As this configuration is critical for network routing, double-check that these IDs match the intended endpoints and that any future additions follow the same consistent format.
72-133: Chains Mapping Consistency
The"chains"array maps"chainId"to"lzChainId"appropriately. However, note the following potential discrepancies:
- The
"endpointIds"array includes chain IDs such as167000,30, and122which are missing from this"chains"array.- Conversely, the
"chains"array includes achainIdof5000that does not appear in"endpointIds".Please verify whether these differences are intentional or if they might lead to configuration mismatches during cross-chain operations.
Test Coverage ReportLine Coverage: 78.75% (2079 / 2640 lines) |
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/LF-12494
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)