Implement ChainflipFacet [ChainflipFacet v1.0.0,IChainflip v1.0.0,ReceiverChainflip v1.0.0] #984
Conversation
WalkthroughThis pull request introduces support for the Chainflip protocol by adding a configuration file, new documentation, demo scripts, deployment and update scripts, a smart contract implementation for the Chainflip facet, a related interface, and comprehensive tests. The changes define vault addresses per network, document public methods and data structures, and provide scripts to deploy and update the facet. Additionally, the implementation of the ChainflipFacet contract enables bridging and swapping tokens according to the Chainflip protocol requirements. Changes
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
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 (
|
![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/Facets/ChainflipFacet.sol (2)
13-13: Remove debug import in production.It seems you're importing the
consoleutility (line 13) for debugging. Consider removing or wrapping it with conditional compilation for production to avoid unnecessary overhead or inadvertent log leakage.-import { console } from "forge-std/console.sol";
100-159: Future-proof chain ID mappings.The chain ID checks and consistent reverts for unsupported chains are well-defined. To accommodate future expansions, consider a more flexible mechanism (whitelisting, mapping, or dynamic registry) to reduce the need to modify contract code for additional chains.
script/deploy/facets/UpdateChainflipFacet.s.sol (1)
1-1: License compatibility check.Currently the file is marked as
UNLICENSED, while other contracts useMIT. This is typically fine for a deployment script, but confirm it aligns with your project’s licensing policy.test/solidity/Facets/ChainflipFacet.t.sol (1)
74-78: Add test cases for invalid ChainflipData parameters.The validChainflipData setup uses hardcoded values. Consider adding test cases for invalid parameters.
+ function testRevert_InvalidChainflipData() public { + ChainflipFacet.ChainflipData memory invalidData = ChainflipFacet.ChainflipData({ + dstToken: 0, + cfParameters: "" + }); + vm.expectRevert("Invalid destination token"); + chainflipFacet.startBridgeTokensViaChainflip(bridgeData, invalidData); + }docs/ChainflipFacet.md (1)
35-35: Fix typo in swap documentation.The sentence has a grammatical error.
-Swapping is performed by a swap specific library that expects an array of calldata to can be run on various DEXs +Swapping is performed by a swap-specific library that expects an array of calldata that can be run on various DEXs🧰 Tools
🪛 LanguageTool
[uncategorized] ~35-~35: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...(SPECIFIC_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
config/chainflip.json(1 hunks)docs/ChainflipFacet.md(1 hunks)script/demoScripts/demoChainflip.ts(1 hunks)script/deploy/facets/DeployChainflipFacet.s.sol(1 hunks)script/deploy/facets/UpdateChainflipFacet.s.sol(1 hunks)src/Facets/ChainflipFacet.sol(1 hunks)src/Interfaces/IChainflip.sol(1 hunks)test/solidity/Facets/ChainflipFacet.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/chainflip.json
🧰 Additional context used
🪛 LanguageTool
docs/ChainflipFacet.md
[uncategorized] ~35-~35: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...
(SPECIFIC_HYPHEN)
[uncategorized] ~47-~47: Possible missing comma found.
Context: ...alls to interact with the Facet In the following some sample calls are shown that allow ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~88-~88: Possible missing comma found.
Context: ...er from 30 USDT on Avalanche to USDC on Binance you can execute the following request: ...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 GitHub Actions: VersionControlAndAuditVerification
src/Interfaces/IChainflip.sol
[error] 1-1: File does not contain a version tag. Every version-controlled contract needs to have a custom:version tag in its code.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (3)
src/Facets/ChainflipFacet.sol (2)
52-72: Implementation of direct bridging logic looks consistent.The logic for depositing assets and then calling
_startBridgeappears sound. Good use of existing modifiers such asnonReentrant,refundExcessNative, and the validation checks.
74-98: Implementation of swap-then-bridge flow is clear.The approach of performing swaps first with
_depositAndSwapbefore calling_startBridgeis well-structured. The finalminAmountassignment ensures correct bridging amounts. Nicely done.script/deploy/facets/DeployChainflipFacet.s.sol (1)
24-30: Verify configuration path handling.The path construction assumes a specific directory structure. Consider adding validation to ensure the config file exists.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the chainflip config file exists if [ ! -f "config/chainflip.json" ]; then echo "Error: chainflip.json configuration file not found" exit 1 fi # Validate JSON structure jq -e '.mainnet.chainflipVault, .arbitrum.chainflipVault' config/chainflip.json > /dev/nullLength of output: 120
Configuration path handling verified.
The shell script confirms that the expected configuration file (config/chainflip.json) exists and contains valid JSON entries for bothmainnet.chainflipVaultandarbitrum.chainflipVault. No issues were detected with the configuration path, so additional validation in this area appears unnecessary.
Test Coverage ReportLine Coverage: 78.64% (2257 / 2870 lines) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
script/demoScripts/demoChainflip.ts (2)
40-41: Consider using environment variables for token addresses.The USDC token address is hardcoded. For better flexibility and to prevent accidental deployments with incorrect addresses, consider using environment variables.
-const SRC_TOKEN_ADDRESS = - '0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48' as `0x${string}` +if (!process.env.SRC_TOKEN_ADDRESS) { + throw new Error('SRC_TOKEN_ADDRESS environment variable is required') +} +const SRC_TOKEN_ADDRESS = process.env.SRC_TOKEN_ADDRESS as `0x${string}`
92-101: Consider adding error handling for bridge failures.The bridge transaction could fail for various reasons (e.g., insufficient liquidity, network issues). Consider adding specific error handling and retry logic.
await executeTransaction( () => lifiDiamondContract.write.startBridgeTokensViaChainflip([ bridgeData, chainflipData, ]), 'Starting bridge tokens via Chainflip', publicClient, true + // Add retry options + { + maxRetries: 3, + retryDelay: 1000, + onError: (error) => { + console.error('Bridge failed:', error) + // Add specific error handling based on error type + } + } )src/Facets/ChainflipFacet.sol (1)
31-39: Consider using an enum or mapping for chain IDs.The current implementation uses constants for chain IDs. Consider using an enum or mapping for better maintainability and to prevent magic numbers.
- uint256 private constant CHAIN_ID_ETHEREUM = 1; - uint256 private constant CHAIN_ID_ARBITRUM = 42161; - uint256 private constant CHAIN_ID_SOLANA = 1151111081099710; - uint256 private constant CHAIN_ID_BITCOIN = 20000000000001; - - uint32 private constant CHAINFLIP_ID_ETHEREUM = 1; - uint32 private constant CHAINFLIP_ID_ARBITRUM = 4; - uint32 private constant CHAINFLIP_ID_SOLANA = 5; - uint32 private constant CHAINFLIP_ID_BITCOIN = 3; + enum ChainId { + ETHEREUM = 1, + ARBITRUM = 42161, + SOLANA = 1151111081099710, + BITCOIN = 20000000000001 + } + + mapping(ChainId => uint32) private chainflipIds = { + ChainId.ETHEREUM: 1, + ChainId.ARBITRUM: 4, + ChainId.SOLANA: 5, + ChainId.BITCOIN: 3 + };test/solidity/Facets/ChainflipFacet.t.sol (2)
81-85: Add test cases for different destination token IDs.The test suite uses a fixed destination token ID of 6. Consider adding test cases for different token IDs to ensure the contract handles various scenarios correctly.
+ function test_CanBridgeTokensWithDifferentDestinationTokens() public { + uint32[] memory tokenIds = new uint32[](3); + tokenIds[0] = 6; // USDC + tokenIds[1] = 1; // Example token + tokenIds[2] = 2; // Example token + + for (uint256 i = 0; i < tokenIds.length; i++) { + validChainflipData.dstToken = tokenIds[i]; + vm.startPrank(USER_SENDER); + usdc.approve(_facetTestContractAddress, bridgeData.minAmount); + initiateBridgeTxWithFacet(false); + vm.stopPrank(); + } + }
219-236: Add more edge cases for address validation.The test only checks for empty addresses. Consider adding tests for invalid address formats and other edge cases.
+ function testRevert_WhenUsingInvalidSolanaAddress() public { + bridgeData.receiver = LibAsset.NON_EVM_ADDRESS; + bridgeData.destinationChainId = CHAIN_ID_SOLANA; + validChainflipData = ChainflipFacet.ChainflipData({ + dstToken: 6, + nonEvmAddress: bytes32("invalid_solana_address"), + cfParameters: "" + }); + + vm.startPrank(USER_SENDER); + usdc.approve(_facetTestContractAddress, bridgeData.minAmount); + vm.expectRevert("Invalid Solana address format"); + initiateBridgeTxWithFacet(false); + vm.stopPrank(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
script/demoScripts/demoChainflip.ts(1 hunks)script/deploy/facets/UpdateChainflipFacet.s.sol(1 hunks)src/Facets/ChainflipFacet.sol(1 hunks)test/solidity/Facets/ChainflipFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/deploy/facets/UpdateChainflipFacet.s.sol
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
🔇 Additional comments (3)
script/demoScripts/demoChainflip.ts (1)
84-89: Verify the destination token ID.The destination token ID is hardcoded to
6. Ensure this is the correct token ID for the destination chain.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any references to token IDs in the codebase to verify the correct value rg -A 5 "dstToken: 6" .Length of output: 2304
Destination Token ID Verification Confirmed
The codebase consistently sets
dstTokento6in both the demo script and the test cases, implying that this value is intended for the destination chain. No discrepancies were found.src/Facets/ChainflipFacet.sol (1)
153-181: Add reentrancy protection for token transfers.While the contract inherits from
ReentrancyGuard, ensure that thenonReentrantmodifier is applied to all external methods that handle token transfers.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for external methods that handle token transfers ast-grep --pattern 'function $name($_) external $_ { $$$ transfer($_) $$$ }'Length of output: 84
Action Required: Manually Verify NonReentrant Usage on External Token Transfer Methods
The initial automated search for a direct call to thetransferfunction did not produce any output. This suggests that token transfers may be performed indirectly (e.g., via the helperLibAsset.maxApproveERC20or through external vault calls likexSwapNativeandxSwapToken) rather than via explicit in-linetransfercalls. However, the review comment raises a valid concern about ensuring that all externally callable methods that trigger token transfers are properly protected with thenonReentrantmodifier.To ensure that the contract is secure:
- Manually inspect the external functions in
src/Facets/ChainflipFacet.solthat perform token operations (such as those invokingLibAsset.maxApproveERC20,xSwapNative, orxSwapToken).- Confirm that each such function is indeed decorated with the
nonReentrantmodifier.- If any token transfer–handling function lacks this protection, add the modifier to mitigate potential reentrancy issues.
Since our automated search did not yield definitive evidence (likely due to abstraction of token transfers), please manually review the relevant functions to ensure robust reentrancy protection.
test/solidity/Facets/ChainflipFacet.t.sol (1)
149-183: Add validation for Solana address format.The test uses a hardcoded Solana address. Consider adding validation to ensure the address follows the correct format.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for Solana address validation in the codebase rg -A 5 "Solana.*address.*validation" .Length of output: 41
Issue: Missing Validation for Solana Address Format
Our search confirms that there is no dedicated Solana address format validation logic anywhere in the codebase. The test in
test/solidity/Facets/ChainflipFacet.t.solis using a hardcoded Solana address without an accompanying check to ensure that the provided address conforms to expected Solana formatting guidelines. It is recommended that you add validation logic—either within the facet or before initiating the bridge transaction—to enforce the correct format for Solana addresses.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Periphery/ReceiverChainflip.sol (1)
82-111: Catch block discards revert reason.The
try ... catch {}without capturing the revert reason helps automatically recover tokens but obscures debugging. If clarity is desired, considercatch Error(string memory reason)to log or emit the reason for easier troubleshooting.Example:
} catch (bytes memory reason) { + // Optionally, decode or log the revert reason assetId.safeTransfer(receiver, amount); emit LiFiTransferRecovered( _transactionId, assetId, receiver, amount, block.timestamp ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Periphery/ReceiverChainflip.sol(1 hunks)test/solidity/Periphery/ReceiverChainflip.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: generate-tag
🔇 Additional comments (7)
src/Periphery/ReceiverChainflip.sol (3)
19-21: Validate non-zero addresses for immutables.Consider adding a check in the constructor to ensure that both
_executorand_chainflipVaultare non-zero addresses to prevent misconfiguration or accidental deployment with a null address.Possible improvement example:
constructor( address _owner, address _executor, address _chainflipVault ) WithdrawablePeriphery(_owner) { + require(_executor != address(0), "Executor address cannot be zero"); + require(_chainflipVault != address(0), "Chainflip vault address cannot be zero"); executor = IExecutor(_executor); chainflipVault = _chainflipVault; }
22-29: Modifier ensures restricted access.The
onlyChainflipVaultmodifier is concise and securely enforces access restrictions tocfReceive. This is a good approach to limit privileged calls solely to the designated Chainflip vault.
113-115: Empty fallback is acceptable.The fallback function is correctly defined to receive native tokens. No further changes are required here.
test/solidity/Periphery/ReceiverChainflip.t.sol (4)
25-48: Initialization covers essential paths.The
setUp()function covers adequate setup steps, including reading configuration JSON files, labeling addresses for clarity, and deploying required contracts. This is well-structured and thorough.
50-59: Constructor test is simple and effective.
test_contractIsSetUpCorrectly()verifies key contract invariants (executor address, vault address). Spelling out these checks is a solid way to confirm proper deployment parameters.
112-139: Ensures privileged function is restricted.
test_revert_OnlyChainflipVaultCanCallCfReceive()properly checks that unauthorized entities cannot invokecfReceive. This adds robust coverage for permission logic.
173-216: Recovery flow verified.
test_willReturnFundsToUserIfDstCallFails()demonstrates correct handling when a destination swap fails, ensuring user funds are returned. This thoroughly verifies error-handling and token recovery.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
docs/ChainflipFacet.md (6)
13-19: Clarify Public Methods Documentation
The public methods are clearly listed along with concise descriptions. To further improve clarity, consider adding parameter-specific details or linking to corresponding implementation sections so that readers can easily cross-reference the information.
32-32: Enhance Guidance on Non-EVM Destinations
The note on setting thereceiverinBridgeDatafor non-EVM destinations is useful. It might help to include a brief example or clarification on edge cases to further support developers using this configuration.
43-49: Improve Terminology: Use "swap-specific"
On line 47, the phrase "swap specific library" is used. For clarity and consistency with common terminology, consider using a hyphen to form "swap-specific library."-Swapping is performed by a swap specific library that expects an array of calldata... +Swapping is performed by a swap-specific library that expects an array of calldata...🧰 Tools
🪛 LanguageTool
[uncategorized] ~47-~47: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...(SPECIFIC_HYPHEN)
86-88: Enhance In-Text Explanation for the /quote Endpoint
The link to detailed documentation is helpful. To aid quick understanding, consider including a short inline summary of the key steps involved in using the/quoteendpoint.
90-97: Punctuation Improvement in "Cross Only" Section
For improved readability, add a comma after "Arbitrum" in line 92.-To get a transaction for a transfer from USDC on Ethereum to USDC on Arbitrum you can execute the following request: +To get a transaction for a transfer from USDC on Ethereum to USDC on Arbitrum, you can execute the following request:🧰 Tools
🪛 LanguageTool
[uncategorized] ~92-~92: Possible missing comma found.
Context: ...ansfer from USDC on Ethereum to USDC on Arbitrum you can execute the following request: ...(AI_HYDRA_LEO_MISSING_COMMA)
98-104: Punctuation Improvement in "Swap & Cross" Section
Similarly, in line 100, consider inserting a comma after "Arbitrum" to enhance clarity.-To get a transaction for a transfer from USDT on Ethereum to USDC on Arbitrum you can execute the following request: +To get a transaction for a transfer from USDT on Ethereum to USDC on Arbitrum, you can execute the following request:🧰 Tools
🪛 LanguageTool
[uncategorized] ~100-~100: Possible missing comma found.
Context: ...ansfer from USDT on Ethereum to USDC on Arbitrum you can execute the following request: ...(AI_HYDRA_LEO_MISSING_COMMA)
script/deploy/facets/DeployReceiverChainflip.s.sol (1)
24-59: Configuration parsing is appropriate—consider validating missing or zero addresses.
The code properly reads from multiple JSON config files and returns addresses viaabi.encode. Ensure the final constructor argument order matches theReceiverChainflipcontract requirements. It may be wise to guard against zero addresses if configurations are missing.function getConstructorArgs() internal override returns (bytes memory) { ... + require(refundWalletAddress != address(0), "Invalid refund wallet"); + require(chainflipVault != address(0), "Invalid vault address"); + require(executor != address(0), "Invalid executor address"); return abi.encode(refundWalletAddress, executor, chainflipVault); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/ChainflipFacet.md(1 hunks)script/deploy/facets/DeployReceiverChainflip.s.sol(1 hunks)script/deploy/resources/deployRequirements.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
script/deploy/facets/DeployReceiverChainflip.s.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/DeployReceiverStargateV2.s.sol:19-21
Timestamp: 2025-01-28T14:27:50.689Z
Learning: In LiFi's deployment scripts, the `deploy` method in `DeployScriptBase` handles the concatenation of constructor arguments with the contract's creation code, so child contracts don't need to concatenate the arguments explicitly.
🪛 LanguageTool
docs/ChainflipFacet.md
[uncategorized] ~47-~47: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...
(SPECIFIC_HYPHEN)
[uncategorized] ~92-~92: Possible missing comma found.
Context: ...ansfer from USDC on Ethereum to USDC on Arbitrum you can execute the following request: ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~100-~100: Possible missing comma found.
Context: ...ansfer from USDT on Ethereum to USDC on Arbitrum you can execute the following request: ...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: generate-tag
🔇 Additional comments (14)
script/deploy/resources/deployRequirements.json (2)
572-581: ChainflipFacet Configuration AdditionThe new
"ChainflipFacet"entry follows the same structure as the existing entries. The use of"configData"with the_chainflipVaultkey, along with the provided"configFileName","keyInConfigFile", and"allowToDeployWithZeroAddress": "false", is consistent with the project's configuration conventions.
Please verify that the"chainflip.json"file is properly set up and that the placeholder<NETWORK>is used consistently with other facets.
582-600: ReceiverChainflip Configuration AdditionThe
"ReceiverChainflip"configuration is well-formed. It correctly defines its"configData"with both the_owner(using"global.json"and".refundWallet") and_chainflipVaultkeys (mirroring the settings of"ChainflipFacet"). In addition, the inclusion of the"contractAddresses"section for"Executor"with"allowToDeployWithZeroAddress": "false"aligns with the structure seen in other receiver configurations.
Ensure that these settings correctly map to the deployed contracts and that external calls to these addresses are properly validated per security best practices.docs/ChainflipFacet.md (7)
1-6: Expand the "How it works" Section for Greater Clarity
The introduction is clear but could benefit from additional technical details. Consider elaborating on how the ChainflipFacet interacts with the ChainflipVault using DELEGATECALL and outlining the token transfer flow in more depth.
7-11: Effective Use of Mermaid Diagram
The Mermaid diagram visually explains the relationship between LiFiDiamond, ChainflipFacet, and Chainflip Vault well. As a suggestion, you might add labels or a brief legend if the diagram expands in complexity in the future.
20-30: Accurate and Informative ChainflipData Struct Documentation
TheChainflipDatastruct is well documented and clearly explains the purpose of each parameter. Ensure that any future changes to the contract implementation are also reflected here to maintain consistency.
34-42: Clear Overview of Supported Chains
The section on supported chains is informative. For added clarity, consider linking to external references or documentation that verifies these chain IDs.
51-55: Solid Explanation of LiFi Data Usage
The explanation regarding theBridgeDataparameter and its role for analytics is clear and sufficient.
57-62: Clear Introduction to Sample Calls
The introductory text for sample calls sets the context well. It might be helpful to mention any prerequisites or assumptions briefly for developers new to the API.
63-84: Well-Structured JavaScript Example for Quote Result
The JavaScript snippet illustrating thequoteResultobject is both clear and illustrative. Please ensure that if any field names change in the future, the example is updated accordingly.script/deploy/facets/DeployReceiverChainflip.s.sol (5)
1-3: License and compiler version look fine.
These lines indicate a non-open source license and a modern Solidity compiler version, which are acceptable choices.
4-7: Imports are concise and well-structured.
All imports are relevant and appear to be aligned with the contract's deployment logic. No issues found.
8-10: Contract declaration and library usage look good.
ThestdJsonlibrary is correctly applied to handle JSON parsing. No concerns here.
11-12: Constructor usage is consistent with the base class.
Passing"ReceiverChainflip"to the parent constructor follows the known pattern forDeployScriptBase.
13-22: Deploy logic appears correct but verify constructor argument handling.
Callingdeploy(type(ReceiverChainflip).creationCode)without manual argument concatenation is correct becauseDeployScriptBasehandles appending the constructor arguments internally. Confirm that theReceiverChainflipconstructor satisfies this approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/solidity/Facets/ChainflipFacet.t.sol (3)
34-86: Add error handling for config file operations.The setup process could be more robust with proper error handling for file operations and JSON parsing.
Consider adding checks:
string memory path = string.concat( vm.projectRoot(), "/config/chainflip.json" ); +require(vm.isFile(path), "Config file not found"); string memory json = vm.readFile(path); +require(bytes(json).length > 0, "Config file is empty"); CHAINFLIP_VAULT = json.readAddress(".mainnet.chainflipVault"); +require(CHAINFLIP_VAULT != address(0), "Invalid vault address in config");
120-147: Add documentation for the override methods.While the implementation is correct, adding documentation would improve maintainability.
Consider adding NatSpec documentation:
+/// @notice Initiates a bridge transaction using the ChainflipFacet +/// @param isNative Boolean indicating if the transaction involves native tokens +/// @dev This is an override of the TestBaseFacet method function initiateBridgeTxWithFacet(bool isNative) internal override {
149-217: Consider adding more edge cases to the bridging tests.While the current tests cover the basic functionality, consider adding tests for:
- Invalid destination token IDs
- Malformed non-EVM addresses
- Maximum and minimum amount boundaries
script/demoScripts/demoChainflip.ts (1)
86-91: Consider making ChainflipData values configurable.The current implementation uses hardcoded values which could limit the script's flexibility:
dstTokenis hardcoded to 3 (USDC)nonEvmAddressis hardcoded to zero addresscfParametersis hardcoded to empty bytesConsider making these values configurable through environment variables or command-line arguments for better reusability:
+const DST_TOKEN = process.env.DST_TOKEN || 3 +const NON_EVM_ADDRESS = process.env.NON_EVM_ADDRESS || '0x0000000000000000000000000000000000000000000000000000000000000000' +const CF_PARAMETERS = process.env.CF_PARAMETERS || '0x' const chainflipData: ChainflipFacet.ChainflipDataStruct = { - dstToken: 3, // USDC - nonEvmAddress: - '0x0000000000000000000000000000000000000000000000000000000000000000', - cfParameters: '0x', // Empty parameters as per implementation + dstToken: DST_TOKEN, + nonEvmAddress: NON_EVM_ADDRESS, + cfParameters: CF_PARAMETERS, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deployments/_deployments_log_file.json(1 hunks)script/demoScripts/demoChainflip.ts(1 hunks)script/demoScripts/utils/demoScriptHelpers.ts(2 hunks)test/solidity/Facets/ChainflipFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deployments/_deployments_log_file.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
🔇 Additional comments (9)
test/solidity/Facets/ChainflipFacet.t.sol (3)
1-10: LGTM! Well-structured imports and declarations.The file uses an appropriate Solidity version and imports all necessary dependencies.
11-22: LGTM! Well-designed test stub contract.The TestChainflipFacet provides appropriate helper methods for testing while maintaining the core functionality of ChainflipFacet.
219-236: LGTM! Good negative test case.The test properly verifies the contract's behavior with an empty non-EVM address.
script/demoScripts/demoChainflip.ts (4)
1-28: LGTM!The imports and setup are well-organized with proper type safety using
Narrow.
94-135: LGTM!The swap implementation is well-structured with proper slippage handling and error checking.
137-146: LGTM!The direct bridging implementation is clean and straightforward.
150-155: LGTM!The error handling is appropriate with proper process exit codes.
script/demoScripts/utils/demoScriptHelpers.ts (2)
183-195: LGTM!The refactored minAmountOut calculation is more readable and maintainable. The 1% slippage tolerance is a reasonable default.
312-312: Verify the increased slippage tolerance.The slippage tolerance has been increased from 5% to 10%. While this provides more flexibility in volatile markets, it also increases the risk of unfavorable trades.
Please confirm if this increase is intentional and aligns with the project's risk tolerance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
script/demoScripts/demoChainflip.ts (4)
28-34: Use constants for chain IDs and validate token addresses.Consider these improvements:
- Define chain IDs as constants or enums for better maintainability
- Add validation for token addresses
+const MAINNET_CHAIN_ID = 1 +const ARBITRUM_CHAIN_ID = 42161 async function main() { const withSwap = process.argv.includes('--with-swap') const srcTokenAddress = withSwap ? ADDRESS_USDT_ARB : ADDRESS_USDC_ARB + if (srcTokenAddress === undefined) { + throw new Error('Source token address is undefined') + } // === Set up environment === const srcChain: SupportedChain = 'arbitrum' - const destinationChainId = 1 // Mainnet + const destinationChainId = MAINNET_CHAIN_ID
78-83: Improve ChainflipData configuration.
- Define constants for Chainflip token designators
- Document the purpose of cfParameters
- Use constants for zero address values
+// Chainflip token designators +const CF_TOKEN_DESIGNATORS = { + USDC_ETH: 3, +} as const +// Constants +const ZERO_BYTES32 = '0x' + '0'.repeat(64) const chainflipData: ChainflipFacet.ChainflipDataStruct = { - dstToken: 3, // Chainflip designator for USDC on ETH + dstToken: CF_TOKEN_DESIGNATORS.USDC_ETH, nonEvmAddress: - '0x0000000000000000000000000000000000000000000000000000000000000000', + ZERO_BYTES32, + // cfParameters is left empty as per Chainflip's implementation + // It may be used for future protocol upgrades cfParameters: '0x', // Empty parameters as per implementation }
85-137: Enhance error handling and transaction monitoring.Consider these improvements:
- Add try-catch blocks for better error handling
- Enhance transaction status logging
- Extract swap data generation to a separate function
+async function generateSwapData( + amount: bigint, + lifiDiamondAddress: string +): Promise<any> { + return getUniswapDataERC20toExactERC20( + ADDRESS_UNISWAP_ARB, + 42161, + ADDRESS_USDT_ARB, + ADDRESS_USDC_ARB, + amount, + lifiDiamondAddress, + true + ) +} if (withSwap) { + try { - const swapData = await getUniswapDataERC20toExactERC20( - ADDRESS_UNISWAP_ARB, - 42161, - ADDRESS_USDT_ARB, - ADDRESS_USDC_ARB, - amount, - lifiDiamondAddress, - true - ) + const swapData = await generateSwapData(amount, lifiDiamondAddress) // ... rest of the code + } catch (error) { + console.error('Failed to generate swap data:', error) + throw error + } }
139-144: Improve error handling in the main execution.Enhance error handling to provide more context and structured error information.
main() .then(() => process.exit(0)) .catch((error) => { - console.error(error) + console.error('Script execution failed:') + if (error instanceof Error) { + console.error('Error message:', error.message) + console.error('Stack trace:', error.stack) + } else { + console.error('Unknown error:', error) + } process.exit(1) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deployments/_deployments_log_file.json(1 hunks)script/demoScripts/demoChainflip.ts(1 hunks)script/demoScripts/utils/demoScriptHelpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- script/demoScripts/utils/demoScriptHelpers.ts
- deployments/_deployments_log_file.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: protect_audit_labels
🔇 Additional comments (1)
script/demoScripts/demoChainflip.ts (1)
1-27: LGTM!The imports and setup are well-organized with proper type safety using Narrow.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
lifi-action-bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Interfaces/IChainflip.sol(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Enforce Min Test Coverage
src/Interfaces/IChainflip.sol
[error] 12-12: Expected '(' but got identifier.
[warning] 12-12: --ir-minimum enables viaIR with minimum optimization, which can result in inaccurate source mappings.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (1)
src/Interfaces/IChainflip.sol (1)
9-27: Well-structured interface design!The interface is well-designed with:
- Clear parameter naming
- Appropriate use of the
payablemodifier for native token swaps- Consistent parameter ordering between methods
🧰 Tools
🪛 GitHub Actions: Enforce Min Test Coverage
[error] 12-12: Expected '(' but got identifier.
[warning] 12-12:
--ir-minimumenables viaIR with minimum optimization, which can result in inaccurate source mappings.
There was a problem hiding this comment.
nitpick / optional:
If there is only one value I would suggest to simplify/revert the structure, e.g.
{
"chainflipVault": {
"mainnet": "...",
"arbitrum": "...",
"optimism": "..."
}
}
src/Interfaces/IChainflip.sol
Outdated
|
|
||
| /// @custom:version 1.0.0 | ||
| interface IChainflipVault { | ||
| function xSwapNative( |
There was a problem hiding this comment.
how did you test the custom receiver without a deployment?
docs/ChainflipFacet.md
Outdated
| ```solidity | ||
| struct ChainflipData { | ||
| uint32 dstToken; // Token identifier on the destination chain | ||
| bytes32 nonEvmAddress; // Destination address for non-EVM chains (Solana, Bitcoin) |
There was a problem hiding this comment.
would suggest to rename this to nonEVMReceiver (like in MayanFacet) and move it to first position (as described in the new facet checklist)
|
|
||
| The facet supports the following chains with their respective IDs: | ||
|
|
||
| - Ethereum (1) |
There was a problem hiding this comment.
Could this change in the future? If yes, could we link to some resource instead that shows which networks are supported so our docs do not get outdated easily?
| }); | ||
| } | ||
|
|
||
| // All facet test files inherit from `utils/TestBaseFacet.sol` and require the following method overrides: |
There was a problem hiding this comment.
please add test cases for destination calls, too
| assertEq(receiver.chainflipVault() == chainflipVault, true); | ||
| } | ||
|
|
||
| function test_OwnerCanPullERC20Token() public { |
There was a problem hiding this comment.
Hmm...not sure if we need to test this in every contract that inherits from WithdrawablePeriphery as this is already tested there....maybe only if some functions are overwritten or what do you think?
There was a problem hiding this comment.
Makes sense. Will remove.
| payload, | ||
| ADDRESS_USDC, | ||
| defaultUSDCAmount | ||
| ); |
There was a problem hiding this comment.
please also check here if funds were transferred / balances changed as expected
same below
There was a problem hiding this comment.
could you please add a demo for a destination call in there, too?
…nto LF-12192-chainflip
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Facets/ChainflipFacet.sol (1)
159-181: Consider specifying a gas budget or clarifying zero value usage.In both the native-token (lines 161-172) and ERC20 (lines 193-203 or 204-212) flows, you're passing
0as the gas budget parameter. If different budgets will eventually be required, consider exposing a parameter or providing a comment to explain that zero is intentional.Also applies to: 193-213
test/solidity/Facets/ChainflipFacet.t.sol (1)
1-301: Optional additional test scenarios.You may want to add coverage for:
- Bridging with an extremely large
dstToken, testing boundary conditions.- Bridging with no
_bridgeData.minAmount(zero) to confirm expected behavior.- Attempting a native token transfer while passing a non-zero allowance for an ERC20.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Facets/ChainflipFacet.sol(1 hunks)src/Interfaces/IChainflip.sol(1 hunks)test/solidity/Facets/ChainflipFacet.t.sol(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Enforce Min Test Coverage
src/Interfaces/IChainflip.sol
[error] 12-12: Expected '(' but got identifier.
[warning] 12-12: --ir-minimum enables viaIR with minimum optimization, which can result in inaccurate source mappings.
🪛 GitHub Actions: Types Bindings
src/Interfaces/IChainflip.sol
[error] 12-12: Expected '(' but got identifier.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (8)
src/Interfaces/IChainflip.sol (3)
6-8: Remove duplicate SPDX license identifier and pragma statements.These lines replicate the license and pragma already declared at lines 1-3, causing unnecessary duplication. Removing them will keep the header clean and help avoid pipeline warnings or failures.
Apply this diff to remove the duplicates:
-// SPDX-License-Identifier: MIT -pragma solidity ^0.8.17;
11-12: Fix duplicate function declaration forxSwapNative.Line 11 and line 12 define the same function
xSwapNative, resulting in a syntax error (the pipeline logs show “Expected '(' but got identifier” at line 12). Removing the second declaration resolves the error.Apply this diff to remove the unintentional duplication:
function xSwapNative( -function xSwapNative( uint32 dstChain,🧰 Tools
🪛 GitHub Actions: Enforce Min Test Coverage
[error] 12-12: Expected '(' but got identifier.
[warning] 12-12:
--ir-minimumenables viaIR with minimum optimization, which can result in inaccurate source mappings.🪛 GitHub Actions: Types Bindings
[error] 12-12: Expected '(' but got identifier.
9-9: License and version tag.The custom version tag on line 9 is helpful for tracking contract versions and meets compliance with your pipeline checks. No issues here.
src/Facets/ChainflipFacet.sol (2)
119-129: Acknowledge if-else logic for chain ID mapping.Your if-else approach for mapping
_bridgeData.destinationChainIdtodstChainis valid, given Solidity’s current limitations on constant or immutable mappings. This pattern ensures clarity on which chains are supported.
153-157: Validation of destination call vs. Chainflip parameters is good.The check
_bridgeData.hasDestinationCall != (_chainflipData.cfParameters.length > 0)helps prevent mismatches between user intent and actual bridging data. Nice to see thorough validation here.test/solidity/Facets/ChainflipFacet.t.sol (3)
35-48: Config-based vault address lookup is well-structured.Reading the
chainflipVaultfromconfig/chainflip.jsonis clean and flexible. Ensure the config file is consistently maintained in different environments (test, staging, production) or handle missing/invalid file reads gracefully.Would you like me to generate a script to confirm the file’s presence in all relevant deployment contexts?
257-299: Test coverage for destination calls is thorough.Your
test_CanBridgeTokensWithDestinationCall()covers bridging tokens with a message (cfParameters) for the Chainflip bridging logic. This is excellent for end-to-end coverage of cross-chain messaging.
224-236: Good coverage for unsupported destination chain.The test verifies the revert condition for polygon chain ID (137), ensuring your
ChainflipFacetproperly rejects unsupported destinations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/ChainflipFacet.md (4)
24-30: Consider renamingnonEvmAddresstononEVMReceiver.For consistency with other facets (like MayanFacet), consider renaming the parameter and moving it to the first position in the struct.
struct ChainflipData { + bytes32 nonEVMReceiver; // Destination address for non-EVM chains (Solana, Bitcoin) uint32 dstToken; // Token identifier on the destination chain - bytes32 nonEvmAddress; // Destination address for non-EVM chains (Solana, Bitcoin) bytes cfParameters; // Additional parameters for future features }
36-43: Consider relying on external documentation for supported chains.To prevent the documentation from becoming outdated, consider removing the explicit chain list and relying solely on the external reference link, which is already provided.
-The facet supports the following chains with their respective IDs: - -- Ethereum (1) -- Arbitrum (42161) -- Solana (1151111081099710) -- Bitcoin (20000000000001) - -[Reference](https://docs.chainflip.io/swapping/integrations/advanced/vault-swaps#supported-chains) +For the latest list of supported chains and their respective IDs, please refer to the [Chainflip documentation](https://docs.chainflip.io/swapping/integrations/advanced/vault-swaps#supported-chains).
61-61: Fix grammatical issues in the text.Add missing commas for better readability:
-In the following some sample calls are shown that allow you to retrieve +In the following, some sample calls are shown that allow you to retrieve-To get a transaction for a transfer from USDT on Ethereum to USDC on Arbitrum you can execute +To get a transaction for a transfer from USDT on Ethereum to USDC on Arbitrum, you can executeAlso applies to: 102-102
🧰 Tools
🪛 LanguageTool
[uncategorized] ~61-~61: Possible missing comma found.
Context: ...alls to interact with the Facet In the following some sample calls are shown that allow ...(AI_HYDRA_LEO_MISSING_COMMA)
90-90: Add a security note about wallet addresses.Consider adding a security reminder about not sharing private keys or using test wallets for examples.
-**Hint**: Don't forget to replace `{YOUR_WALLET_ADDRESS}` with your real wallet address in the examples. +**Hint**: Don't forget to replace `{YOUR_WALLET_ADDRESS}` with your real wallet address in the examples. For testing purposes, always use a test wallet and never share your private keys.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/ChainflipFacet.md(1 hunks)src/Interfaces/IChainflip.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Interfaces/IChainflip.sol
🧰 Additional context used
🧠 Learnings (1)
docs/ChainflipFacet.md (1)
Learnt from: ezynda3
PR: lifinance/contracts#984
File: docs/ChainflipFacet.md:0-0
Timestamp: 2025-02-13T08:57:00.095Z
Learning: The ChainflipData struct in ChainflipFacet has three fields:
1. uint32 dstToken - The destination token identifier in Chainflip
2. bytes32 nonEvmAddress - The non-EVM destination address
3. bytes cfParameters - Additional parameters for Chainflip protocol
🪛 LanguageTool
docs/ChainflipFacet.md
[uncategorized] ~49-~49: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...
(SPECIFIC_HYPHEN)
[uncategorized] ~61-~61: Possible missing comma found.
Context: ...alls to interact with the Facet In the following some sample calls are shown that allow ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~102-~102: Possible missing comma found.
Context: ...ansfer from USDT on Ethereum to USDC on Arbitrum you can execute the following request: ...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
🔇 Additional comments (1)
docs/ChainflipFacet.md (1)
1-12: LGTM! Clear introduction with helpful visualization.The introduction effectively explains the facet's purpose and includes a clear mermaid diagram showing the interaction flow.
script/demoScripts/demoChainflip.ts
Outdated
| const amount = parseUnits('10', Number(srcTokenDecimals)) | ||
|
|
||
| console.info( | ||
| `\nBridge ${amount} ${srcTokenName} (${srcTokenSymbol}) from ${srcChain} --> Arbitrum` |
src/Facets/ChainflipFacet.sol
Outdated
| error EmptyNonEvmAddress(); | ||
| address public immutable chainflipVault; |
There was a problem hiding this comment.
nitpick:blank line between these two lines
src/Facets/ChainflipFacet.sol
Outdated
|
|
||
| /// @dev Parameters specific to Chainflip bridge | ||
| /// @param dstToken Token to be received on the destination chain (uint32) | ||
| /// @param cfParameters Additional metadata for future features (currently unused) |
There was a problem hiding this comment.
missing description for nonEvmAddress param
src/Facets/ChainflipFacet.sol
Outdated
|
|
||
| /// @dev Parameters specific to Chainflip bridge | ||
| /// @param dstToken Token to be received on the destination chain (uint32) | ||
| /// @param cfParameters Additional metadata for future features (currently unused) |
There was a problem hiding this comment.
I think that part for future features (currently unused) could be removed and added to Third party integration notion page with explanation that this param is unused and can be used in the future. Wdyt?
| /// @notice Constructor for the contract. | ||
| /// @param _chainflipVault Address of the Chainflip vault contract | ||
| constructor(address _chainflipVault) { | ||
| chainflipVault = _chainflipVault; |
There was a problem hiding this comment.
In my opinion would be better to replace this line:
address public immutable chainflipVault;
to this
IChainflipVault public immutable chainflipVault;
where constructor would also accept IChainflipVault _chainflipVault param and adjust the rest of the code accordingly
| vm.expectRevert("ChainflipFacet: Unsupported destination chain"); | ||
| initiateBridgeTxWithFacet(false); |
There was a problem hiding this comment.
nitpick: blank lines between these two lines
| vm.label(address(erc20Proxy), "ERC20Proxy"); | ||
| } | ||
|
|
||
| function test_contractIsSetUpCorrectly() public { |
There was a problem hiding this comment.
nitpick: to keep test naming consistent start with capital character test_ContractIsSetUpCorrectly. similar cases below
| ); | ||
| } | ||
|
|
||
| function test_revert_WithdrawTokenNonOwner() public { |
| @@ -530,5 +533,18 @@ abstract contract TestBase is Test, DiamondTest, ILiFi { | |||
| uint256 targetBlock = block.number + numBlocks; | |||
| vm.roll(targetBlock); | |||
| } | |||
|
|
|||
| function getConfigAddressFromPath( | |||
| { | ||
| "chainflipVault": { | ||
| "mainnet": "0xF5e10380213880111522dd0efD3dbb45b9f62Bcc", | ||
| "arbitrum": "0x79001a5e762f3bEFC8e5871b42F6734e00498920" | ||
| } | ||
| } |
There was a problem hiding this comment.
instead of using the following json structure. I propose organizing our config files using a structure that maps networks to their elements like:
{
"mainnet": {
"chainflipVault": "0xF5e10380213880111522dd0efD3dbb45b9f62Bcc"
},
"arbitrum": {
"chainflipVault": "0x79001a5e762f3bEFC8e5871b42F6734e00498920"
}
}
This approach will align with our template demo script which has the following code:
// If you need to retrieve a specific address from your config file
// based on the chain and element name, use this helper function.
//
// First, ensure you import the relevant config file:
// import config from '../../config/{{camelCase name}}.json'
//
// Then, retrieve the address:
// const EXAMPLE_ADDRESS = getConfigElement(config, srcChain, 'example');
https://github.com/lifinance/contracts/blob/main/templates/facetDemoScript.template.hbs#L37-L45
getConfigElement function first checks for the specified network and element after the network.
https://github.com/lifinance/contracts/blob/main/script/demoScripts/utils/demoScriptHelpers.ts#L606-L618
I understand that more complex structures exist in our repository but in most cases the format described above will be sufficient. It’s better to keep it clear from the beginning.
always open for discussion
There was a problem hiding this comment.
@0xDEnYO I reverted it as you suggested but this approach makes sense in that it keeps everything in a format that our helpers expect even if it requires a bit more repetition.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/solidity/utils/TestBase.sol (1)
537-548: LGTM! Well-structured configuration reader.The
getConfigAddressFromPathfunction provides a clean way to read network-specific addresses from JSON configuration files. The implementation:
- Safely constructs the file path using
string.concat- Uses foundry's
vm.projectRoot()for reliable path resolution- Leverages
vm.readFilefor file I/O during tests- Returns parsed address using the JSON path
Consider adding error handling for:
- Non-existent configuration files
- Invalid JSON paths
- Invalid address formats
test/solidity/Facets/ChainflipFacet.t.sol (1)
35-85: Remove console log before merging.The setup function is well-structured, but the console log should be removed.
Apply this diff:
- console.log("Chainflip Vault Address:", CHAINFLIP_VAULT);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
config/chainflip.json(1 hunks)script/demoScripts/demoChainflip.ts(1 hunks)script/deploy/facets/DeployChainflipFacet.s.sol(1 hunks)script/deploy/facets/DeployReceiverChainflip.s.sol(1 hunks)script/deploy/resources/deployRequirements.json(1 hunks)src/Facets/ChainflipFacet.sol(1 hunks)src/Periphery/ReceiverChainflip.sol(1 hunks)test/solidity/Facets/ChainflipFacet.t.sol(1 hunks)test/solidity/Periphery/ReceiverChainflip.t.sol(1 hunks)test/solidity/utils/TestBase.sol(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- script/deploy/facets/DeployChainflipFacet.s.sol
- script/deploy/facets/DeployReceiverChainflip.s.sol
- config/chainflip.json
🧰 Additional context used
🧠 Learnings (1)
src/Periphery/ReceiverChainflip.sol (1)
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Periphery/ReceiverChainflip.sol:49-71
Timestamp: 2025-02-13T08:56:27.487Z
Learning: The executor contract in ReceiverChainflip and other periphery contracts is a trusted component of the system, and therefore does not require additional reentrancy protection.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: generate-tag
🔇 Additional comments (24)
test/solidity/utils/TestBase.sol (1)
20-22: LGTM! Adding JSON parsing capabilities.The addition of
stdJsonfrom forge-std enables reading and parsing JSON configuration files during testing, which is a good practice for managing network-specific addresses and other configuration data.script/demoScripts/demoChainflip.ts (3)
1-27: LGTM!The imports and configuration setup are well-organized and properly typed.
28-61: LGTM!The main function setup is comprehensive, with proper handling of command-line arguments and environment initialization.
85-136: Add destination call example.The script demonstrates basic bridging and swap+bridge scenarios but lacks an example of destination calls. Consider adding a demo for destination calls to showcase this feature.
src/Periphery/ReceiverChainflip.sol (4)
1-18: LGTM!The imports and contract declaration are well-organized with appropriate inheritance.
19-35: LGTM!The storage variables and access control modifier are properly implemented.
61-83: LGTM!The
cfReceivefunction is well-implemented with proper access control and message handling.
101-140: LGTM!The
_swapAndCompleteBridgeTokensfunction handles both token types correctly with proper error handling and approval management.src/Facets/ChainflipFacet.sol (3)
47-51: Add validation for ChainflipData struct.Consider adding validation for the
ChainflipDatastruct fields to prevent potential issues:
dstToken: Validate supported token IDsnonEvmAddress: Validate non-zero value for non-EVM chainscfParameters: Validate length constraints
66-107: LGTM!The bridging functions are well-implemented with proper access control and validation.
115-227: LGTM!The internal methods are well-implemented with proper error handling and chain ID mapping.
test/solidity/Periphery/ReceiverChainflip.t.sol (4)
1-86: LGTM!The setup and basic tests are comprehensive, properly verifying contract initialization and access control.
88-132: LGTM!The ERC20 swap tests are thorough, verifying both successful execution and balance changes.
134-224: LGTM!The failure handling and native token tests provide good coverage of edge cases and native token functionality.
227-296: LGTM!The helper functions are well-implemented, providing comprehensive test data generation for both token types.
test/solidity/Facets/ChainflipFacet.t.sol (7)
11-23: LGTM!The stub contract is well-structured with appropriate helper functions for testing.
116-148: LGTM!The test case thoroughly validates the bridging functionality to Solana with appropriate balance assertions.
150-180: LGTM!The test case thoroughly validates the bridging functionality to Bitcoin with appropriate balance assertions.
182-216: LGTM!The test case thoroughly validates the bridging functionality to Ethereum with appropriate balance assertions and chain-specific setup.
218-231: LGTM!The test case correctly validates the error handling for unsupported chains.
233-251: LGTM!The test case correctly validates the error handling for empty non-EVM addresses.
253-295: LGTM!The test case thoroughly validates the bridging functionality with destination call and appropriate balance assertions.
script/deploy/resources/deployRequirements.json (2)
573-581: LGTM!The ChainflipFacet configuration follows the established pattern and correctly references the chainflip.json file.
582-599: LGTM!The ReceiverChainflip configuration follows the established pattern and correctly references the required configuration files.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
src/Interfaces/IChainflip.sol (2)
6-8: Move@custom:versiontag above pragma statement.Based on the project's standard practice mentioned in past review comments, the version tag should be placed above the pragma statement.
Apply this diff to align with the project's conventions:
// SPDX-License-Identifier: MIT +/// @custom:version 1.0.0 pragma solidity ^0.8.17; import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; /// @title Interface for Chainflip Vault contract -/// @custom:version 1.0.0 interface IChainflipVault {🧰 Tools
🪛 GitHub Actions: SC Core Dev Approval Check
[warning] 6-8: Remove duplicate SPDX license identifier and pragma statements.
9-71:❓ Verification inconclusive
Enhance NatSpec documentation for better clarity.
The NatSpec comments could be improved by:
- Adding descriptions for
nonEvmAddressparameter inxSwapNativeandxSwapTokenfunctions.- Clarifying the format and constraints for
cfParameters.
🌐 Web query:
What is the format and constraints for cfParameters in Chainflip protocol?💡 Result:
The Chainflip protocol's
cfParametersencompass various configuration settings and constraints that govern swap execution, liquidity provisioning, and validator operations. Key components include:DCA (Dollar Cost Averaging) Parameters
- Chunk count: Set by brokers but subject to protocol limits[1]
- Minimum chunk size: Protocol-defined floor per asset (e.g., 1k USDC)[1]
- Automatic adjustment: Protocol reduces chunks if
(total_amount / chunk_size) < requested_chunks[1]Swap Execution Parameters
type ExecuteSwapParams = { srcChain: Chain; srcAsset: Asset; destChain: Chain; destAsset: Asset; destAddress: string; amount: string; // Base units (e.g., wei) ccmParams?: { gasBudget: string; // Base units message: `0x${string}`; // Hex-encoded <10kB }; };Constraints:
- ERC-20 token allowances must cover swap amounts[4]
- Message payloads limited to <10kB[4]
- Gas budgets specified in source asset base units[4]
Cryptographic Constraints
- FROST threshold signatures require parameter validation:
- Generated values must lie in [1,q) range for EdDSA/Schnorr operations[7][10]
- Strict bounds checking during secret sharing ceremonies[10]
Liquidity Provisioning Parameters
- Requires State Chain account with staked FLIP[10]
- Deterministic address generation rules:
- Bitcoin: HD wallet addresses[10]
- EVM chains: CREATE2 addresses[10]
Cross-Chain Messaging (CCM)
Receiver interface requirements:
// EVM function cfReceive( uint32 srcChain, bytes calldata srcAddress, bytes calldata message, address token, uint256 amount ) external payable; // Solana pub fn cf_receive_token( source_chain: u32, source_address: Vec<u8>, message: Vec<u8>, amount: u64 ) -> Result<()>;Parameters must match these function signatures exactly[4].
These parameters are enforced through protocol-level validation in both on-chain components (State Chain) and off-chain validator operations[6][7]. Developers can query current parameter values via the
cf_environmentRPC method[1].Citations:
- 1: https://docs.chainflip.io/swapping/swapping-basics
- 2: https://docs.chainflip.io/concepts/system-overview
- 3: https://chainflip.io/faq
- 4: https://docs.chainflip.io/swapping/integrations/javascript-sdk/swap-assets/execute-swap
- 5: https://chorus.one/articles/network-101-a-concise-guide-to-chainflip-and-how-it-simplifies-cross-chain-swaps
- 6: https://docs.chainflip.io/validators/mainnet/validator-setup/important-notes
- 7: https://resources.cryptocompare.com/asset-management/10393/1701792654639.pdf
- 8: https://docs.chainflip.io/concepts/welcome
- 9: https://docs.chainflip.io/lp/introduction
- 10: https://www.auxilioventures.com/wp-content/uploads/2022/08/Chainflip-Whitepaper-May-2020.pdf
Enhance NatSpec Documentation for cfParameters and Non-EVM Address Handling
cfParameters: Update the NatSpec for both
xSwapNativeandxSwapTokento clearly explain that thecfParametersargument encapsulates multiple configuration settings. This includes parameters for dollar cost averaging (e.g., chunk count, minimum chunk size), swap execution details (such as gas budget in source-asset base units and a hex-encoded message limited to less than 10kB), cryptographic constraints (e.g., bounds for threshold signature validation), liquidity provisioning rules, and cross-chain messaging requirements. Referencing the Chainflip protocol documents can provide developers with the necessary context on these expected formats and constraints.Non-EVM Address Clarification: The current NatSpec does not mention a
nonEvmAddressparameter—even though the review requested descriptions for it. If thedstAddressparameter is intended to also support non-EVM chain address formats, please clarify this in the documentation by indicating:
- The expected format for non-EVM addresses.
- Any differences or additional validations required when dealing with addresses on non-EVM chains.
This enhanced documentation will ensure that developers correctly interpret the parameters and adhere to protocol-level validations.
🧰 Tools
🪛 GitHub Actions: SC Core Dev Approval Check
[error] 12-12: Expected '(' but got identifier.
[error] 11-12: Duplicate function declaration for 'xSwapNative'.
[warning] 12-12: --ir-minimum enables viaIR with minimum optimization, which can result in inaccurate source mappings.
[error] 11-12: Duplicate function declaration for 'xSwapNative'.
script/demoScripts/demoChainflip.ts (1)
77-77: Consider using constants for chain IDs.The hardcoded chain ID could be replaced with a constant for better maintainability.
Apply this diff to use the constant:
- bridgeData.destinationChainId = 42161; // Arbitrum chain ID + bridgeData.destinationChainId = CHAIN_ID_ARBITRUM;test/solidity/Facets/ChainflipFacet.t.sol (2)
45-45: Remove debug logging in production code.Console logging should be removed before deployment.
- console.log("Chainflip Vault Address:", CHAINFLIP_VAULT);
118-307: Add missing test scenarios.The test suite could be enhanced with additional scenarios:
- Test for maximum message length
- Test for various gas amounts in destination calls
- Test for token ID validation
Would you like me to generate the additional test cases?
docs/ChainflipFacet.md (6)
13-19: Enhance method documentation with parameter descriptions.Consider adding parameter descriptions to make the documentation more comprehensive:
- `function startBridgeTokensViaChainflip(BridgeData calldata _bridgeData, ChainflipData calldata _chainflipData)` - - Bridges tokens using Chainflip without performing any swaps + - Bridges tokens using Chainflip without performing any swaps + - Parameters: + - `_bridgeData`: Contains bridge configuration and analytics data + - `_chainflipData`: Contains Chainflip-specific parameters for the transfer - `swapAndStartBridgeTokensViaChainflip(BridgeData memory _bridgeData, LibSwap.SwapData[] calldata _swapData, ChainflipData memory _chainflipData)` - - Performs swap(s) before bridging tokens using Chainflip + - Performs swap(s) before bridging tokens using Chainflip + - Parameters: + - `_bridgeData`: Contains bridge configuration and analytics data + - `_swapData`: Array of swap operations to perform before bridging + - `_chainflipData`: Contains Chainflip-specific parameters for the transfer
20-35: Add usage examples for ChainflipData parameters.Consider adding examples to illustrate how to properly format the parameters:
} + +### Examples + +```javascript +const chainflipData = { + // For Bitcoin destination + nonEVMReceiver: "0x1234...5678", // Bitcoin address encoded as bytes32 + dstToken: 20000000000001, // Bitcoin chain ID + message: "0x", // Empty for Bitcoin + gasAmount: 0, // Not used for Bitcoin + cfParameters: "0x" // Empty if no additional parameters +}; + +// For Solana destination +const chainflipData = { + nonEVMReceiver: "0x9abc...def0", // Solana address encoded as bytes32 + dstToken: 1151111081099710, // Solana chain ID + message: "0x...", // Optional message data + gasAmount: 1000000, // Gas budget for Solana + cfParameters: "0x..." // Additional parameters if needed +}; +```
36-46: Make chain support documentation more maintainable.Instead of listing specific chains, consider referencing Chainflip's documentation directly:
## Supported Chains -The facet supports the following chains with their respective IDs: +The facet supports various EVM and non-EVM chains. For the most up-to-date list of supported chains and their respective IDs, please refer to: -Ethereum (1) -Arbitrum (42161) -Solana (1151111081099710) -Bitcoin (20000000000001) - -[Reference](https://docs.chainflip.io/swapping/integrations/advanced/vault-swaps#supported-chains) +- [Chainflip Supported Chains Documentation](https://docs.chainflip.io/swapping/integrations/advanced/vault-swaps#supported-chains) + +This ensures you always have access to the latest information about supported chains and their IDs.
47-60: Fix grammatical issues in the documentation.Minor grammatical improvements needed:
-Swapping is performed by a swap specific library that expects an array of calldata +Swapping is performed by a swap-specific library that expects an array of calldata🧰 Tools
🪛 LanguageTool
[uncategorized] ~51-~51: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda...(SPECIFIC_HYPHEN)
61-93: Improve readability of the API documentation.Add proper punctuation and clarify the explanation:
-In the following some sample calls are shown that allow you to retrieve a populated transaction +In the following section, sample calls are shown that allow you to retrieve a populated transaction🧰 Tools
🪛 LanguageTool
[uncategorized] ~63-~63: A comma might be missing here.
Context: ...alls to interact with the Facet In the following some sample calls are shown that allow ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
94-108: Add example responses for API calls.Consider adding example responses to help users understand what to expect:
curl 'https://li.quest/v1/quote?fromChain=ETH&fromAmount=1000000&fromToken=USDT&toChain=ARB&toToken=USDC&slippage=0.03&allowBridges=chainflip&fromAddress={YOUR_WALLET_ADDRESS}'
+Example response:
+```json
+{
- "id": "0x...",
- "type": "lifi",
- "tool": "chainflip",
- "action": {
- "fromChainId": 1,
- "toChainId": 42161,
- "fromToken": "USDT",
- "toToken": "USDC",
- "fromAmount": "1000000",
- "slippage": "0.03"
- },
- "estimate": {
- "toAmount": "998500",
- "executionDuration": 300
- }
+}
+```</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ed037ce0ceb55e43a8393466eee8c5df6e816b0d and 6d22b49c290a632969ae30a9ef75a5c7e6f27391. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `docs/ChainflipFacet.md` (1 hunks) * `script/demoScripts/demoChainflip.ts` (1 hunks) * `src/Facets/ChainflipFacet.sol` (1 hunks) * `src/Interfaces/IChainflip.sol` (1 hunks) * `test/solidity/Facets/ChainflipFacet.t.sol` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (1)</summary> <details> <summary>docs/ChainflipFacet.md (1)</summary>Learnt from: ezynda3
PR: #984
File: docs/ChainflipFacet.md:0-0
Timestamp: 2025-02-13T08:57:00.095Z
Learning: The ChainflipData struct in ChainflipFacet has three fields:
- uint32 dstToken - The destination token identifier in Chainflip
- bytes32 nonEvmAddress - The non-EVM destination address
- bytes cfParameters - Additional parameters for Chainflip protocol
</details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/ChainflipFacet.md</summary> [uncategorized] ~51-~51: When ‘swap-specific’ is used as a modifier, it is usually spelled with a hyphen. Context: ... parameter. Swapping is performed by a swap specific library that expects an array of callda... (SPECIFIC_HYPHEN) --- [uncategorized] ~63-~63: A comma might be missing here. Context: ...alls to interact with the Facet In the following some sample calls are shown that allow ... (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA) </details> </details> <details> <summary>🪛 GitHub Actions: SC Core Dev Approval Check</summary> <details> <summary>test/solidity/Facets/ChainflipFacet.t.sol</summary> [warning] 1-301: Optional additional test scenarios recommended. </details> <details> <summary>src/Interfaces/IChainflip.sol</summary> [error] 12-12: Expected '(' but got identifier. --- [error] 11-12: Duplicate function declaration for 'xSwapNative'. --- [warning] 12-12: --ir-minimum enables viaIR with minimum optimization, which can result in inaccurate source mappings. --- [warning] 6-8: Remove duplicate SPDX license identifier and pragma statements. --- [error] 11-12: Duplicate function declaration for 'xSwapNative'. </details> <details> <summary>src/Facets/ChainflipFacet.sol</summary> [warning] 159-181: Consider specifying a gas budget or clarifying zero value usage. --- [warning] 47-51: Add validation for ChainflipData struct. </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (4)</summary> * GitHub Check: audit-verification * GitHub Check: run-unit-tests * GitHub Check: enforce-min-test-coverage * GitHub Check: generate-tag </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>src/Interfaces/IChainflip.sol (1)</summary> `1-4`: **LGTM!** The license, pragma, and import statements are correctly defined. </details> <details> <summary>script/demoScripts/demoChainflip.ts (2)</summary> `1-27`: **LGTM!** The imports and setup code are well-structured and correctly implemented. --- `82-82`: <details> <summary>✅ Verification successful</summary> **Verify the destination token ID.** The hardcoded token ID `7` should be verified for correctness. --- 🏁 Script executed: ```shell #!/bin/bash # Search for token ID definitions or mappings rg -A 5 "dstToken.*7" .Length of output: 982
Token ID Verification: Confirm USDC on Arbitrum Mapping
The test files indicate that the destination token ID is consistently hardcoded as
7—with comments (e.g., intest/solidity/Facets/ChainflipFacet.t.sol) specifying it represents USDC on Arbitrum. Please ensure that this mapping is intentional for the production flow. If this value is correct, consider linking it to a centralized configuration or adding documentation to prevent future ambiguity.
src/Facets/ChainflipFacet.sol (1)
220-234: Consider using a mapping for chain ID conversion.
While the if-else chain works, it could be refactored for better readability.
Based on past review comments and learnings, using if-else with constants is the preferred approach in Solidity since mappings cannot be constant or immutable.
docs/ChainflipFacet.md (1)
3-11: LGTM! Clear and effective explanation.
The overview and diagram effectively communicate how the Chainflip Facet integrates with the LiFiDiamond contract and interacts with the Chainflip Vault.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
script/demoScripts/demoChainflip.ts (1)
77-84: Consider documenting the Chainflip parameters.The
chainflipDatastruct contains hardcoded values that might not be immediately clear to users.Add comments explaining:
- Why
nonEVMReceiveris set to zero bytes32- What the
dstTokenvalue 3 represents (USDC on ETH)- The purpose of empty
messageandcfParameterssrc/Facets/ChainflipFacet.sol (1)
149-154: Consider improving error message for destination call mismatch.The current error message
InformationMismatchis generic and might not help users understand the specific issue.- revert InformationMismatch(); + revert("ChainflipFacet: hasDestinationCall must match message presence");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(2 hunks)deployments/arbitrum.staging.json(1 hunks)script/demoScripts/demoChainflip.ts(1 hunks)src/Facets/ChainflipFacet.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- deployments/_deployments_log_file.json
- deployments/arbitrum.staging.json
- deployments/arbitrum.diamond.staging.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
🔇 Additional comments (4)
script/demoScripts/demoChainflip.ts (2)
1-17: LGTM! Clean and well-organized imports.The imports are logically grouped and include all necessary dependencies for the demo script.
140-145: LGTM! Proper error handling.The script includes appropriate error handling and process exit codes.
src/Facets/ChainflipFacet.sol (2)
50-56: LGTM! Well-documented struct.The
ChainflipDatastruct is well-documented with clear parameter descriptions.
220-234: LGTM! Clear chain ID conversion logic.The chain ID conversion is implemented using if-else statements as recommended, with clear error handling.
| const swapData = await getUniswapDataERC20toExactERC20( | ||
| ADDRESS_UNISWAP_ARB, // Uniswap router address on Arbitrum | ||
| 42161, // Arbitrum chain id | ||
| ADDRESS_USDT_ARB, // Swap from USDT | ||
| ADDRESS_USDC_ARB, // Swap to USDC | ||
| amount, // The exact output amount we want in USDC | ||
| lifiDiamondAddress, // Receiver for the swapped tokens (the diamond) | ||
| true // requiresDeposit flag | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding slippage protection for the swap.
The swap data is generated without explicit slippage parameters, which could lead to unfavorable rates.
🏁 Script executed:
#!/bin/bash
# Check if getUniswapDataERC20toExactERC20 includes slippage protection
rg -A 5 "getUniswapDataERC20toExactERC20" ./script/demoScripts/utils/Length of output: 540
Action Required: Add explicit slippage protection to the swap
It appears that the function getUniswapDataERC20toExactERC20—as currently implemented in script/demoScripts/utils/demoScriptHelpers.ts—does not include any parameter or logic for specifying a slippage tolerance. Given that the swap data is generated without explicit slippage parameters, this might expose the swap to unfavorable rate fluctuations.
- Location:
script/demoScripts/demoChainflip.ts(lines 89–97) - Verification: The search output for
getUniswapDataERC20toExactERC20confirmed that the function signature accepts the uniswap router address, chain ID, sending and receiving asset IDs, and the exact output amount—but no slippage parameter is present.
Please consider enhancing the function to incorporate slippage protection, either by adding a new parameter or integrating a mechanism that adjusts the swap to guard against price volatility.
| if (_bridgeData.sendingAssetId == address(0)) { | ||
| if (_bridgeData.hasDestinationCall) { | ||
| IChainflipVault(chainflipVault).xCallNative{ | ||
| value: _bridgeData.minAmount | ||
| }( | ||
| dstChain, | ||
| encodedDstAddress, | ||
| _chainflipData.dstToken, | ||
| _chainflipData.message, | ||
| _chainflipData.gasAmount, | ||
| _chainflipData.cfParameters | ||
| ); | ||
| } else { | ||
| IChainflipVault(chainflipVault).xSwapNative{ | ||
| value: _bridgeData.minAmount | ||
| }( | ||
| dstChain, | ||
| encodedDstAddress, | ||
| _chainflipData.dstToken, | ||
| _chainflipData.cfParameters | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding gas budget validation for cross-chain calls.
The contract forwards native value without validating the gas budget, which could lead to failed transactions on the destination chain.
🌐 Web query:
What is the recommended gas budget for Chainflip cross-chain calls?
💡 Result:
The recommended gas budget for Chainflip cross-chain calls depends on the specific blockchain involved and transaction type. Based on available data:
For Ethereum deposits:
- Typical gas usage is ~31k gas[5], costing significantly less than standard swaps (about 1/3 of Uniswap v3 trade costs)[1].
- Batching reduces costs further for destination-chain transfers[5].
General guidelines:
- Destination-chain variability:
- Gas budgets should account for real-time network conditions on the target chain (e.g., Solana/NEAR typically cost $2k-$10k daily)[4].
- Use Symbiosis-like logic where calldata includes stablecoin-equivalent gas estimates for S-chain and destination-chain steps[2].
Key optimization factors:
- Chainflip's app-chain architecture minimizes external computation costs by handling logic internally[1][9].
- Batching combines multiple egress transactions into single broadcasts to reduce per-transaction overhead[5].
- Protocol automatically deducts estimated gas fees from swap outputs during broadcast[5].
For developers:
- Use Symbiosis-style simulations to verify calldata executability before setting budgets[2].
- Refer to Chainflip's SDK/API tools for real-time fee estimations[2][5].
While exact figures vary by chain, Ethereum-based swaps generally require budgeting ~31k gas plus destination-chain fees adjusted through batching optimizations[1][5].
Citations:
- 1: https://blog.chainflip.io/chainflip-jit-amm-pricing/
- 2: https://docs.symbiosis.finance/main-concepts/symbiosis-and-fees
- 3: https://github.com/iliasoroka1/chainflip_agg_contracts
- 4: https://flipsidecrypto.xyz/rmas/gas-guzzlers-cross-chain-comparison-0ylyOE
- 5: https://docs.chainflip.io/concepts/swaps-amm/native-swap-flow
- 6: https://chainflip.io/whitepaper.pdf
- 7: https://docs.chainflip.io/concepts/token-economics/incentive-design-emission-and-burning
- 8: https://www.auxilioventures.com/wp-content/uploads/2022/08/Chainflip-Whitepaper-May-2020.pdf
- 9: https://cryptonary.com/research/smart-money-bets-on-chainflip-is-this-the-next-cross-chain-star/
- 10: https://learn.bybit.com/defi/what-is-chainflip-flip/
Gas Budget Validation for Cross-Chain Calls
In src/Facets/ChainflipFacet.sol (lines 157–179), the contract currently forwards native value without verifying that the provided gas budget (i.e., _chainflipData.gasAmount) is sufficient for execution on the destination chain. Based on Chainflip guidance, for Ethereum-based swaps a typical gas budget is around ~31k gas (plus additional destination-chain fees when applicable), though exact requirements depend on real-time network conditions.
-
Action Required:
Add a check to validate that_chainflipData.gasAmountmeets or exceeds an expected threshold (for example, at least ~31k gas for Ethereum) before initiating a cross-chain call. Consider using Chainflip’s SDK/API or Symbiosis-style simulation logic to determine the appropriate gas amount dynamically. -
Rationale:
Without this validation, insufficient gas could lead to failed transactions on the destination chain.
Which Jira task belongs to this PR?
LF-12192
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)