Deprecate unused interfaces #1022
Conversation
…stargateV1-and-Amarok
WalkthroughThis update revises several configuration and deployment scripts to transition from the older Stargate configuration files to a new v2 format. In 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: 0
🔭 Outside diff range comments (2)
src/Facets/CalldataVerificationFacet.sol (2)
6-6:⚠️ Potential issueRemove duplicate import of
AcrossFacetV3.The
AcrossFacetV3import is duplicated on lines 6 and 8.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 validation block for AcrossV3.
The validation block for
AcrossFacetV3(lines 354-412) is duplicated. The first block handles bothstartBridgeTokensViaAcrossV3andswapAndStartBridgeTokensViaAcrossV3, making the second block redundant.// 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 (1)
src/Facets/CalldataVerificationFacet.sol (1)
269-416: Consider refactoringvalidateDestinationCalldatafor better maintainability.The function has grown complex with multiple similar validation blocks. Consider refactoring it to reduce code duplication and improve maintainability.
Here's a suggested approach:
+ struct ValidationData { + bytes4 selector; + bytes callData; + bytes dstCalldata; + bytes callTo; + } + + function _validateStargateV2(ValidationData memory data) internal pure returns (bool) { + if (data.selector == StargateFacetV2.startBridgeTokensViaStargate.selector) { + (, StargateFacetV2.StargateData memory stargateDataV2) = abi.decode( + data.callData.slice(4, data.callData.length - 4), + (ILiFi.BridgeData, StargateFacetV2.StargateData) + ); + return + keccak256(data.dstCalldata) == keccak256(stargateDataV2.sendParams.composeMsg) && + _compareBytesToBytes32CallTo(data.callTo, stargateDataV2.sendParams.to); + } + if (data.selector == StargateFacetV2.swapAndStartBridgeTokensViaStargate.selector) { + (, , StargateFacetV2.StargateData memory stargateDataV2) = abi.decode( + data.callData.slice(4, data.callData.length - 4), + (ILiFi.BridgeData, LibSwap.SwapData[], StargateFacetV2.StargateData) + ); + return + keccak256(data.dstCalldata) == keccak256(stargateDataV2.sendParams.composeMsg) && + _compareBytesToBytes32CallTo(data.callTo, stargateDataV2.sendParams.to); + } + return false; + } + function validateDestinationCalldata( bytes calldata data, bytes calldata callTo, bytes calldata dstCalldata ) external pure returns (bool isValid) { - bytes memory callData = data; + ValidationData memory validationData; + validationData.callData = data; + validationData.dstCalldata = dstCalldata; + validationData.callTo = callTo; // Handle standardizedCall if (bytes4(data[:4]) == StandardizedCallFacet.standardizedCall.selector) { - callData = abi.decode(data[4:], (bytes)); + validationData.callData = abi.decode(data[4:], (bytes)); } - bytes4 selector = abi.decode(callData, (bytes4)); + validationData.selector = abi.decode(validationData.callData, (bytes4)); // Validate based on selector - if (selector == StargateFacetV2.startBridgeTokensViaStargate.selector || - selector == StargateFacetV2.swapAndStartBridgeTokensViaStargate.selector) { - return _validateStargateV2(callData, callTo, dstCalldata); - } + return _validateStargateV2(validationData); }This refactoring:
- Introduces a
ValidationDatastruct to group related data- Extracts validation logic into separate functions per bridge
- Reduces code duplication
- Makes it easier to add/remove bridge validations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
archive/config/stargateV1.json(1 hunks)archive/scripts/demoScripts/demoDeBridge.ts(1 hunks)archive/scripts/demoScripts/demoSynapseBridge.ts(1 hunks)config/stargate.json(1 hunks)script/deploy/healthCheck.ts(0 hunks)src/Facets/CalldataVerificationFacet.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 (4)
- archive/scripts/demoScripts/demoSynapseBridge.ts
- archive/config/stargateV1.json
- archive/scripts/demoScripts/demoDeBridge.ts
- config/stargate.json
🧰 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 (4)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: generate-tag
🔇 Additional comments (1)
src/Facets/CalldataVerificationFacet.sol (1)
18-18: LGTM! Version update to 1.3.0.The version update aligns with the removal of deprecated interfaces.
Test Coverage ReportLine Coverage: 78.75% (2079 / 2640 lines) |
🤖 GitHub Action: Security Alerts Review 🔍🚨 Unresolved Security Alerts Found! 🚨 🔴 View Alert - File: 🟢 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: |
…unused-interfaces
lifi-action-bot
changed the title
…unused-interfaces
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/stargateV2.json (3)
2-51: ValidateendpointV2Mapping Values
TheendpointV2object provides deployment addresses for multiple chains. Notice that several chains share the same address (for example, mainnet, arbitrum, aurora, base, blast, bsc, celo, fantom, etc. are assigned0x1a44076050125825900e736c501f859c50fE728c). Please verify that reusing the same address across these chains is intentional and documented.
52-81: ReviewtokenMessagingConfiguration Block
ThetokenMessagingsection now includes several critical deployment addresses along with reference links to documentation. Some entries (e.g.,"fantom": "----FantomIsNotSupportedByStargateV2----") use explicit placeholder strings to denote lack of support. Verify that all downstream logic consuming these values is robust against placeholder strings and won’t inadvertently treat them as valid addresses.
82-82: Placeholder forzksyncintokenMessaging
The value for the"zksync"key is now set to"---comingSoon---", clearly marking it as a placeholder. Ensure that any code referring to this key properly checks for and handles this non-address value to avoid runtime errors or misconfigurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/stargateV2.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
config/stargateV2.json (1)
Learnt from: ezynda3
PR: lifinance/contracts#875
File: config/stargate.json:34-34
Timestamp: 2024-11-26T07:14:24.221Z
Learning: For the Lisk chain, an entry in the `endpointIds` array in `config/stargate.json` is not required.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
🔇 Additional comments (1)
config/stargateV2.json (1)
1-84: Overall Configuration Restructuring Confirmation
This file reflects a significant restructuring by removing the deprecated interfaces (such as the oldendpointIdsarray) and organizing configuration into two clear objects:endpointV2andtokenMessaging. Ensure that all downstream scripts and contracts have been updated to reference these new keys.
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)