LF-11761 - Implement glacis [GlacisFacet v1.0.0,IGlacisAirlift v1.0.0] #945
Conversation
|
Warning Rate limit exceeded@mirooon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces the Glacis Facet, a new smart contract and configuration system designed for cross-chain token bridging. It adds support for bridging tokens across the Arbitrum, Optimism, and Base networks using the Glacis airlift protocol. Key changes include the introduction of a JSON configuration file, deployment scripts, contract implementations, interface definitions, and comprehensive test coverage for the bridging functionality. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
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: 7
🧹 Nitpick comments (6)
src/Interfaces/IGlacisAirlift.sol (2)
32-35: Clarify parameter descriptions inaddSelectorsToTokenThe parameter descriptions for
diamondSelectorsandfacetSelectorsare identical in the comments. Update the descriptions to accurately reflect each parameter's purpose to avoid confusion.
79-86: MarkquoteSendfunction asviewThe
quoteSendfunction appears to only read state without modifying it. Marking it asviewcan optimize gas usage and clarify its intended use.src/Facets/GlacisFacet.sol (1)
105-105: Simplify address to bytes32 conversionThe conversion
bytes32(uint256(uint160(_bridgeData.receiver)))can be simplified tobytes32(uint256(uint160(address(_bridgeData.receiver))))or evenbytes32(uint256(uint160(_bridgeData.receiver)))depending on the context. Ensure that the conversion is clear and concise.🧰 Tools
🪛 GitHub Actions: Audit Verifier
[error] Could not find a logged audit for contract GlacisFacet in version 1.0.0. This check will not pass until the audit log contains a completed audit for this file.
script/deploy/facets/UpdateGlacisFacet.s.sol (1)
6-13: Align contract name with file name for clarityThe contract is named
DeployScript, but the file name isUpdateGlacisFacet.s.sol. For consistency and maintainability, consider renaming the contract toUpdateGlacisFacetScriptto match the file name.test/solidity/Facets/GlacisFacet.t.sol (1)
221-227: Consider parameterizing liquidity amounts.The hardcoded liquidity amounts should be parameterized to test different scenarios and market conditions.
+ uint256 constant DAI_LIQUIDITY = 100_000 * 10 ** 18; + uint256 constant WORMHOLE_LIQUIDITY = 100_000 * 10 ** 18; + addLiquidity( ADDRESS_DAI, ADDRESS_WORMHOLE_TOKEN, - 100_000 * 10 ** ERC20(ADDRESS_DAI).decimals(), - 100_000 * 10 ** wormhole.decimals() + DAI_LIQUIDITY, + WORMHOLE_LIQUIDITY );docs/GlacisFacet.md (1)
37-37: Fix language and formatting issues.
- Line 37: Add hyphen to "swap-specific"
- Line 49: Add comma after "In the following"
Also applies to: 49-49
🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: 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 (13)
config/glacis.json(1 hunks)deployments/_deployments_log_file.json(2 hunks)deployments/arbitrum.diamond.staging.json(2 hunks)deployments/arbitrum.staging.json(1 hunks)docs/GlacisFacet.md(1 hunks)script/demoScripts/demoGlacisAirlift.ts(1 hunks)script/deploy/facets/DeployGlacisFacet.s.sol(1 hunks)script/deploy/facets/UpdateGlacisFacet.s.sol(1 hunks)src/Facets/GlacisFacet.sol(1 hunks)src/Interfaces/IGlacisAirlift.sol(1 hunks)test/solidity/Facets/GlacisFacet.t.sol(1 hunks)test/solidity/utils/Interfaces.sol(1 hunks)test/solidity/utils/TestBase.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/glacis.json
🧰 Additional context used
🪛 LanguageTool
docs/GlacisFacet.md
[uncategorized] ~37-~37: 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] ~49-~49: 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)
🪛 GitHub Actions: Audit Verifier
src/Facets/GlacisFacet.sol
[error] Could not find a logged audit for contract GlacisFacet in version 1.0.0. This check will not pass until the audit log contains a completed audit for this file.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (10)
src/Facets/GlacisFacet.sol (1)
16-112: Ensure the newGlacisFacetcontract has undergone a preliminary auditThe pipeline indicates that
GlacisFacethas not yet passed an audit. It's important to have the contract audited to ensure security and compliance before deployment.Run the following script to check for logged audits of
GlacisFacet:✅ Verification successful
The review comment is correct - GlacisFacet has not been audited
The project's audit log shows comprehensive tracking of all audited contracts, and GlacisFacet is not listed among them. While the project maintains regular security reviews with the latest audit completed on January 10, 2025, this new facet requires a security audit before deployment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search audit logs for entries related to GlacisFacet version 1.0.0 # Search for GlacisFacet in audit logs rg 'GlacisFacet' audits/ # Verify if there is an entry for version 1.0.0 rg 'GlacisFacet.*version.*1\.0\.0' audits/Length of output: 165
Script:
#!/bin/bash # Search for audit and security related files echo "=== Searching for audit and security files ===" fd -t f -i "audit|security" --max-depth 3 echo -e "\n=== Checking README and documentation for audit mentions ===" # Search in markdown files fd -e md | xargs rg -i "audit|security|glacis" echo -e "\n=== Checking docs directory ===" # Check if docs directory exists and search within it fd -t d "^docs$" --max-depth 2Length of output: 2752
Script:
#!/bin/bash echo "=== Checking audit log for GlacisFacet ===" cat audit/auditLog.json echo -e "\n=== Checking git history for recent GlacisFacet changes ===" # Check when GlacisFacet was added/modified git log --oneline --no-merges -- src/Facets/GlacisFacet.solLength of output: 9275
🧰 Tools
🪛 GitHub Actions: Audit Verifier
[error] Could not find a logged audit for contract GlacisFacet in version 1.0.0. This check will not pass until the audit log contains a completed audit for this file.
test/solidity/utils/Interfaces.sol (1)
61-70: LGTM! Interface matches Uniswap V2 Router specification.The
addLiquidityfunction signature correctly matches the Uniswap V2 Router interface specification.test/solidity/Facets/GlacisFacet.t.sol (1)
147-175: Complete the TODO for fuzzed test implementation.The fuzzed test implementation needs completion. Consider:
- Adding more edge cases
- Testing with different token decimals
- Testing with various chain IDs
Would you like me to help implement a more comprehensive fuzzed test suite?
deployments/arbitrum.staging.json (1)
53-54: LGTM!The new facet addresses have been added correctly.
docs/GlacisFacet.md (2)
3-11: Complete the "How it works" section.The explanation of how the Glacis Facet works is incomplete. Please provide a detailed explanation of:
- The purpose of the facet
- The interaction flow between components
- Any important considerations or limitations
15-18: Enhance method documentation.The method descriptions should include:
- Parameter descriptions
- Return values
- Error conditions
- Usage examples with code snippets
deployments/arbitrum.diamond.staging.json (2)
141-142: LGTM!The new facets have been added with correct format and version numbers.
Also applies to: 148-150
160-160: LGTM!The periphery addresses have been updated correctly.
Also applies to: 162-162
deployments/_deployments_log_file.json (2)
26996-26996: Review the optimizer runs value.The optimizer runs value of 1000000 is unusually high. Standard values typically range between 200-10000. High values can:
- Increase deployment gas costs
- Lead to potential optimization bugs
- Result in longer compilation times
Consider reducing this value unless there's a specific reason for such aggressive optimization.
26998-26998: Verify the constructor argument address.Please verify that
0xe0a049955e18cffd09c826c2c2e965439b6ab272is the correct Glacis protocol address on Arbitrum.✅ Verification successful
Constructor argument address is verified correct
The address
0xe0a049955e18cffd09c826c2c2e965439b6ab272is confirmed as the correct Glacis Airlift contract address on Arbitrum, as evidenced by the official configuration inconfig/glacis.jsonand its consistent usage throughout the codebase's implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the constructor argument address matches Glacis protocol # Search for references to this address in the codebase echo "Searching for address references in the codebase..." rg -i "0xe0a049955e18cffd09c826c2c2e965439b6ab272" # Search for Glacis protocol address constants echo "Searching for Glacis protocol address constants..." ast-grep --pattern 'address constant GLACIS_ADDRESS = $_'Length of output: 520
Script:
#!/bin/bash # Search for documentation files mentioning Glacis echo "Searching for documentation about Glacis..." fd -e md -e txt | xargs rg -i "glacis.*address|address.*glacis" # Search for any deployment or configuration files mentioning Glacis echo "Searching for deployment/config files..." fd -e json -e yaml -e toml | xargs rg -i "glacis" # Search for interface definitions to understand the contract structure echo "Searching for Glacis interface definitions..." ast-grep --pattern 'interface IGlacisAirlift'Length of output: 1069
Script:
#!/bin/bash # Search for contract implementations and imports echo "Searching for Glacis-related imports and implementations..." rg -A 5 "import.*Glacis|contract.*Glacis" # Search for any references to Airlift in the codebase echo "Searching for Airlift references..." rg -i "airlift" -A 5 # Look for any deployment scripts specifically related to Glacis echo "Searching for Glacis deployment scripts..." fd deploy | xargs rg -i "glacis"Length of output: 15948
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
script/demoScripts/demoGlacisAirlift.ts (3)
15-16: Add environment variable validation.Add checks for required environment variables with descriptive error messages.
- const RPC_URL = process.env.ETH_NODE_URI_ARBITRUM - const PRIVATE_KEY = process.env.PRIVATE_KEY + const RPC_URL = process.env.ETH_NODE_URI_ARBITRUM + if (!RPC_URL) { + throw new Error('ETH_NODE_URI_ARBITRUM environment variable is not set') + } + const PRIVATE_KEY = process.env.PRIVATE_KEY + if (!PRIVATE_KEY) { + throw new Error('PRIVATE_KEY environment variable is not set') + }
69-69: Update integrator name.The integrator name "ACME Devs" should be replaced with a more specific identifier for tracking and monitoring purposes.
- integrator: 'ACME Devs', + integrator: 'LiFi Demo Script',
79-81: Use typed contract interface instead of raw ABI array.Replace the raw ABI array with a proper interface definition for better type safety and maintainability.
- const airlift = new Contract(config.arbitrum.airlift, [ - 'function quoteSend(address token, uint256 amount, bytes32 receiver, uint256 destinationChainId, address refundAddress, uint256 msgValue) external returns ((uint256, uint256), uint256, uint256, ((uint256, uint256), uint256, uint256))', - ]) + const airlift = IGlacisAirlift__factory.connect(config.arbitrum.airlift, provider)This requires adding the import:
import { IGlacisAirlift__factory } from '../../typechain'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
script/demoScripts/demoGlacisAirlift.ts(1 hunks)src/Interfaces/IGlacisAirlift.sol(1 hunks)test/solidity/Facets/GlacisFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/solidity/Facets/GlacisFacet.t.sol
- src/Interfaces/IGlacisAirlift.sol
🔇 Additional comments (1)
script/demoScripts/demoGlacisAirlift.ts (1)
8-8: Verify staging deployment configuration usage.The script is using staging deployments. Ensure this is intentional and document any limitations or differences compared to production deployments.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/solidity/Facets/GlacisFacet.t.sol (2)
219-225: Ensure liquidity addition is successful before proceeding with swapsIn the
testBase_CanSwapAndBridgeTokenstest, liquidity is added to the DEX pair forADDRESS_DAIandADDRESS_WORMHOLE_TOKEN. Consider adding assertions to verify that the liquidity addition was successful before performing swaps. This can help in diagnosing issues related to insufficient liquidity during testing.
66-74: Review function approvals for swap functions involving native tokensSince the facet does not support bridging of native assets, approving Uniswap functions like
swapTokensForExactETHandswapETHForExactTokensmay not be necessary for this test suite. Removing these approvals can streamline the setup and focus the tests on supported functionalities.
📜 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/demoGlacisAirlift.ts(1 hunks)test/solidity/Facets/GlacisFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deployments/_deployments_log_file.json
- script/demoScripts/demoGlacisAirlift.ts
🧰 Additional context used
📓 Learnings (1)
test/solidity/Facets/GlacisFacet.t.sol (1)
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: The GlacisFacet test suite inherits from TestBaseFacet which already covers various failure scenarios including invalid receiver address, invalid amounts, same chain bridging, and insufficient funds, making additional failure scenario tests redundant.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (3)
test/solidity/Facets/GlacisFacet.t.sol (3)
14-20: Approval functions in test contract are appropriateThe functions
addDexandsetFunctionApprovalBySignatureinTestGlacisFaceteffectively set up the testing environment by allowing the addition of DEX addresses and approval of function signatures. This enhances the flexibility and robustness of the tests.
37-103: Initialization in setUp function is comprehensive and well-structuredThe
setUpfunction thoroughly initializes the test environment, including setting up test contracts, configuring function selectors, approving DEX functions, and preparing valid test data withvalidGlacisData. This ensures that the tests run in a controlled and predictable environment.
262-283: Revert test for insufficient funds is correctly implementedThe
testBase_Revert_CallerHasInsufficientFundsfunction effectively tests the scenario where the caller lacks sufficient tokens to perform the bridge. The use ofvm.expectRevertwith the appropriate error selector ensures that the contract behaves as expected in this failure case.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/solidity/Facets/GlacisFacet.t.sol (1)
79-86: Add assertions to verify successful liquidity additionAfter adding liquidity for the DAI-{SOURCE TOKEN} pair, consider adding assertions to confirm that the liquidity has been successfully added to the DEX. This ensures that the swap operations in subsequent tests have the necessary liquidity and can prevent false positives due to inadequate setup.
Example:
addLiquidity( ADDRESS_DAI, ADDRESS_SRC_TOKEN, 100_000 * 10 ** ERC20(ADDRESS_DAI).decimals(), 100_000 * 10 ** srcToken.decimals() ); + // Assert that liquidity has been added + uint256 pairBalance = ERC20(ADDRESS_DAI).balanceOf(ADDRESS_UNISWAP_PAIR); + require(pairBalance > 0, "Liquidity addition failed");Note: Replace
ADDRESS_UNISWAP_PAIRwith the actual pair address or retrieve it dynamically.src/Interfaces/IGlacisAirlift.sol (1)
65-78: Add missing parameter description formsgValueIn the
quoteSendfunction, the parametermsgValuelacks a Natspec comment description. Please add a description formsgValueto enhance code documentation and clarity.docs/GlacisFacet.md (2)
1-16: Documentation clearly explains the Glacis Facet integration.The integration details and limitations are well documented. However, there's a minor grammatical improvement needed in line 7: "They are custom tokens focused" should be "It focuses on custom tokens" for better clarity.
18-51: Method and parameter documentation is comprehensive.The documentation for public methods, GlacisData struct, and parameters is clear and well-structured.
Consider using a swap-specific instead of "swap specific" in line 42 for better readability.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~42-~42: 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 (9)
deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(2 hunks)deployments/arbitrum.staging.json(1 hunks)docs/GlacisFacet.md(1 hunks)script/demoScripts/demoGlacisAirlift.ts(1 hunks)src/Facets/GlacisFacet.sol(1 hunks)src/Interfaces/IGlacisAirlift.sol(1 hunks)test/solidity/Facets/GlacisFacet.t.sol(1 hunks)test/solidity/utils/TestBase.sol(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- deployments/arbitrum.staging.json
- deployments/_deployments_log_file.json
- script/demoScripts/demoGlacisAirlift.ts
- deployments/arbitrum.diamond.staging.json
🧰 Additional context used
🪛 LanguageTool
docs/GlacisFacet.md
[uncategorized] ~42-~42: 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] ~54-~54: 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)
[uncategorized] ~95-~95: 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: SC Core Dev Approval Check
src/Facets/GlacisFacet.sol
[error] Contract GlacisFacet has not undergone a required audit for version 1.0.0
test/solidity/Facets/GlacisFacet.t.sol
[warning] 219-225: Missing assertions to verify successful liquidity addition before performing swaps
🪛 GitHub Actions: Audit Verifier
src/Facets/GlacisFacet.sol
[error] Missing audit log entry for contract GlacisFacet version 1.0.0. An audit log entry must be added before the check can pass.
🪛 Gitleaks (8.21.2)
test/solidity/Facets/GlacisFacet.t.sol
366-366: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
380-380: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (5)
src/Facets/GlacisFacet.sol (2)
99-104: Confirm token allowance and transfer to the 'airlift' contractEnsure that the contract has the necessary allowance to transfer
_bridgeData.sendingAssetIdtokens to theairliftcontract. SinceSafeERC20.safeTransferis used, the transfer will fail without sufficient allowance. Verify that the allowance is properly set prior to this transfer.🧰 Tools
🪛 GitHub Actions: SC Core Dev Approval Check
[error] Contract GlacisFacet has not undergone a required audit for version 1.0.0
🪛 GitHub Actions: Audit Verifier
[error] Missing audit log entry for contract GlacisFacet version 1.0.0. An audit log entry must be added before the check can pass.
109-109: Verify the correctness of address to bytes32 conversionThe conversion
bytes32(uint256(uint160(_bridgeData.receiver)))is used to cast anaddresstobytes32. Confirm that this conversion aligns with the expected format of thereceiverparameter inairlift.send. Incorrect casting may lead to funds being sent to an unintended address.🧰 Tools
🪛 GitHub Actions: SC Core Dev Approval Check
[error] Contract GlacisFacet has not undergone a required audit for version 1.0.0
🪛 GitHub Actions: Audit Verifier
[error] Missing audit log entry for contract GlacisFacet version 1.0.0. An audit log entry must be added before the check can pass.
test/solidity/utils/TestBase.sol (3)
171-181: BASE network addresses follow consistent pattern.The added constant addresses for the BASE network maintain the same structure as other networks.
250-259: BASE network overwrite logic is properly implemented.The address overwrite logic for BASE network follows the same pattern as other networks and includes all necessary address updates.
467-491: Test utility function implementation is correct.The
addLiquidityfunction correctly implements the liquidity addition process for testing purposes. As per previous feedback, production-level validations are not required in test utility functions.
| /// @author LI.FI (https://li.fi/) | ||
| /// @notice Integration of the Glacis airlift (wrapper for native token bridging standards) | ||
| /// @custom:version 1.0.0 | ||
| contract GlacisFacet is ILiFi, ReentrancyGuard, SwapperV2, Validatable { |
There was a problem hiding this comment.
💡 Codebase verification
GlacisFacet requires audit completion before deployment
The review comment is accurate. After examining the audit logs, there is no audit record for the GlacisFacet contract at any version. All other facets in the system have undergone security audits, as evidenced by the comprehensive audit history in audit/auditLog.json. An audit needs to be completed and logged before proceeding with the deployment.
🔗 Analysis chain
Ensure the 'GlacisFacet' contract undergoes the required audit
As indicated in the pipeline failures, the GlacisFacet contract has not undergone the required audit for version 1.0.0. Please complete the audit process and add the necessary audit log entry to pass the pipeline checks.
You can verify the presence of an audit log entry using the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for an audit log entry for 'GlacisFacet' version 1.0.0
# Search for audit entries mentioning 'GlacisFacet' at version 1.0.0
grep -r "GlacisFacet" audits/ | grep "version 1.0.0" || echo "Audit log entry NOT found for GlacisFacet version 1.0.0."
Length of output: 216
Script:
#!/bin/bash
# Search for any audit-related files and documentation
echo "=== Searching for audit-related files ==="
fd -i audit
echo -e "\n=== Checking for README files that might contain audit info ==="
fd -i readme -e md -X cat {}
echo -e "\n=== Checking when GlacisFacet.sol was added ==="
git log --diff-filter=A --format="%ad %h %s" --date=short -- src/Facets/GlacisFacet.sol
Length of output: 12191
Script:
#!/bin/bash
echo "=== Checking audit log for GlacisFacet entries ==="
cat audit/auditLog.json | jq -r '.[] | select(.contract | contains("GlacisFacet"))'
echo -e "\n=== Checking full audit log structure ==="
cat audit/auditLog.json | jq '.'
Length of output: 9451
🧰 Tools
🪛 GitHub Actions: SC Core Dev Approval Check
[error] Contract GlacisFacet has not undergone a required audit for version 1.0.0
🪛 GitHub Actions: Audit Verifier
[error] Missing audit log entry for contract GlacisFacet version 1.0.0. An audit log entry must be added before the check can pass.
src/Facets/GlacisFacet.sol
Outdated
| payable(msg.sender) | ||
| ); |
There was a problem hiding this comment.
Please check that we are handling the native fee right, e.g. when the user starts with native token, we have to ensure enough native stays in the contract to pay the bridge fee. e.g. check StargateFacetV2
There was a problem hiding this comment.
remember our discussion about the parameter _nativeReserve....if you dont know how to do it, let's do it together
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
src/Facets/GlacisFacet.sol (3)
12-16: Consider clarifying usage in the docstring.While the docstring references wrapper functionality for native token bridging standards, it might be beneficial to further highlight the reliance on the
IGlacisAirliftinterface and any assumptions regarding deposit of tokens prior to calling this function.🧰 Tools
🪛 GitHub Actions: Audit Verifier
[error] Missing audit log entry for contract GlacisFacet version 1.0.0. An audit log entry must be added before the check can pass.
40-60: Validate zero-value edge cases.The function deposits assets and then calls
_startBridge. Consider adding checks handling zero_bridgeData.minAmount. This prevents unintended bridging calls with zero tokens.🧰 Tools
🪛 GitHub Actions: Audit Verifier
[error] Missing audit log entry for contract GlacisFacet version 1.0.0. An audit log entry must be added before the check can pass.
90-115: Document bridging steps for clarity.Inline comments are helpful, but adding a bit more context for each step (particularly the
SafeERC20.safeTransfertoairliftand the subsequentairlift.send) will make this logic friendlier for reviewers and auditors.🧰 Tools
🪛 GitHub Actions: Audit Verifier
[error] Missing audit log entry for contract GlacisFacet version 1.0.0. An audit log entry must be added before the check can pass.
src/Interfaces/IGlacisAirlift.sol (2)
34-49: Ensure correct refund flow on bridging.The
sendfunction references arefundAddress. Confirm that any leftover funds after bridging properly route to the designated address. This helps prevent lock-ups or lost tokens.
65-80: Review return data usage.The
quoteSendfunction returns a complex structQuoteSendInfo. Ensure consuming contracts interpret and use theairliftFeeInfofields correctly (e.g. log, user display, fee calculations) to avoid confusion.docs/GlacisFacet.md (2)
37-37: Use proper hyphenation for clarity.Replace “swap specific library” with “swap-specific library” to improve readability.
- Swapping is performed by a swap specific library + Swapping is performed by a swap-specific library🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: 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)
49-49: Consider adding a comma after “In the following”.A missing comma can slightly reduce clarity. Add a comma to improve flow.
- In the following some sample calls are shown... + In the following, some sample calls are shown...🧰 Tools
🪛 LanguageTool
[uncategorized] ~49-~49: 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)
test/solidity/Facets/GlacisFacet.t.sol (3)
32-32: Consider adding a detailed comment explaining the payableAmount value.The
payableAmountvalue of 1 ether is used for fee estimation in thequoteSendfunction. While there's a brief explanation in the comments below, it would be helpful to document why this specific value was chosen and any implications of using a different value.
128-130: Add explicit revert tests for native token operations.Instead of leaving these test functions empty with comments, consider adding explicit tests that verify the contract correctly rejects native token operations.
function testBase_CanBridgeNativeTokens() public virtual override { - // facet does not support bridging of native assets + bridgeData.sendingAssetId = address(0); + + vm.startPrank(USER_SENDER); + vm.expectRevert("GlacisFacet: Native asset not supported"); + initiateBridgeTxWithFacet(false); + vm.stopPrank(); }Also applies to: 193-195
368-369: Add comments explaining the network context for hardcoded addresses.Consider adding detailed comments that explain:
- The network context (Arbitrum, Base)
- Why these specific tokens were chosen for testing
- Links to block explorers for reference
- ADDRESS_SRC_TOKEN = 0xB0fFa8000886e57F86dd5264b9582b2Ad87b2b91; // address of W token on Arbitrum network + // W token on Arbitrum network + // Reference: https://arbiscan.io/token/0xB0fFa8000886e57F86dd5264b9582b2Ad87b2b91 + ADDRESS_SRC_TOKEN = 0xB0fFa8000886e57F86dd5264b9582b2Ad87b2b91;Also applies to: 382-383
🧰 Tools
🪛 Gitleaks (8.21.2)
368-368: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config/glacis.json(1 hunks)docs/GlacisFacet.md(1 hunks)src/Facets/GlacisFacet.sol(1 hunks)src/Interfaces/IGlacisAirlift.sol(1 hunks)test/solidity/Facets/GlacisFacet.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/glacis.json
🧰 Additional context used
🪛 LanguageTool
docs/GlacisFacet.md
[uncategorized] ~37-~37: 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] ~49-~49: 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)
🪛 GitHub Actions: Audit Verifier
src/Facets/GlacisFacet.sol
[error] Missing audit log entry for contract GlacisFacet version 1.0.0. An audit log entry must be added before the check can pass.
🪛 Gitleaks (8.21.2)
test/solidity/Facets/GlacisFacet.t.sol
368-368: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
382-382: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (5)
src/Facets/GlacisFacet.sol (1)
62-86: Review swap error handling.For the swap path, ensure you handle scenarios where the swap might fail or return less than expected. Consider clarifying whether partial fills are allowed or if the function reverts on insufficient output after the swap.
🧰 Tools
🪛 GitHub Actions: Audit Verifier
[error] Missing audit log entry for contract GlacisFacet version 1.0.0. An audit log entry must be added before the check can pass.
src/Interfaces/IGlacisAirlift.sol (2)
5-21: Minimize interface surface if possible.Review whether all fields in these structs (
QuoteSendInfo,AirliftFeeInfo,Fee) are absolutely necessary. Reducing interface bloat can streamline the codebase and make audits more manageable.
23-33: Validate external calls.The
addSelectorsToTokenmethod can introduce complexity if unverified or untrusted facets get whitelisted. Consider strict contract verification or access control checks prior to registering new facets for tokens.docs/GlacisFacet.md (1)
5-5: Validate description accuracy.Double-check that the references to GMP (General Message Passing) align with the actual bridging logic. The text emphasizes GMP, but Glacis primarily focuses on token bridging. Align phrasing with the actual capabilities of Glacis.
test/solidity/Facets/GlacisFacet.t.sol (1)
1-386: Overall implementation looks good!The test suite is well-structured with comprehensive coverage of the GlacisFacet functionality. It includes:
- Proper setup and configuration
- Thorough test cases for both success and failure scenarios
- Network-specific test implementations
🧰 Tools
🪛 Gitleaks (8.21.2)
368-368: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
382-382: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/solidity/Facets/GlacisFacet.t.sol (2)
10-21: Add contract documentation.Consider adding NatSpec documentation to describe the purpose and functionality of the
TestGlacisFacetcontract and its methods.// Stub GlacisFacet Contract +/// @title TestGlacisFacet +/// @notice Test helper contract that extends GlacisFacet with additional methods for testing +/// @dev Provides methods to manage allowed contracts and function selectors contract TestGlacisFacet is GlacisFacet { constructor(IGlacisAirlift _airlift) GlacisFacet(_airlift) {} + /// @notice Adds a DEX contract to the allowed list + /// @param _dex Address of the DEX to allow function addDex(address _dex) external { LibAllowList.addAllowedContract(_dex); } + /// @notice Adds a function selector to the allowed list + /// @param _signature Function selector to allow function setFunctionApprovalBySignature(bytes4 _signature) external { LibAllowList.addAllowedSelector(_signature); } }
368-394: Document the hardcoded addresses.The contract addresses are valid blockchain addresses, not API keys. However, consider adding more detailed comments explaining:
- The purpose of each contract address
- Why these specific networks were chosen
- The significance of the block numbers used for forking
contract GlacisFacetWormholeTest is GlacisFacetTestBase { function setUp() public virtual override { + // Arbitrum network configuration + // Block number chosen to ensure stable state for testing customRpcUrlForForking = "ETH_NODE_URI_ARBITRUM"; customBlockNumberForForking = 298468086; + // Glacis Airlift contract on Arbitrum airliftContract = IGlacisAirlift( 0xE0A049955E18CFfd09C826C2c2e965439B6Ab272 ); + // W token on Arbitrum, chosen for its liquidity and compatibility ADDRESS_SRC_TOKEN = 0xB0fFa8000886e57F86dd5264b9582b2Ad87b2b91; + // Optimism chain ID for cross-chain testing destinationChainId = 10; super.setUp(); } } contract GlacisFacetLINKTest is GlacisFacetTestBase { function setUp() public virtual override { + // Base network configuration + // Block number chosen to ensure stable state for testing customRpcUrlForForking = "ETH_NODE_URI_BASE"; customBlockNumberForForking = 25427676; + // Glacis Airlift contract on Base airliftContract = IGlacisAirlift( 0x56E20A6260644CC9F0B7d79a8C8E1e3Fabc15CEA ); + // LINK token on Base, chosen for its liquidity and compatibility ADDRESS_SRC_TOKEN = 0x88Fb150BDc53A65fe94Dea0c9BA0a6dAf8C6e196; + // Mode chain ID for cross-chain testing destinationChainId = 34443; super.setUp(); } }🧰 Tools
🪛 Gitleaks (8.21.2)
376-376: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
390-390: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Actions: SC Core Dev Approval Check
[warning] 380-380: Detected a Generic API Key, potentially exposing access to various services and sensitive operations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/solidity/Facets/GlacisFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (1)
test/solidity/Facets/GlacisFacet.t.sol (1)
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:0-0
Timestamp: 2025-01-28T11:29:09.566Z
Learning: Empty test methods with explanatory comments are acceptable in test classes when the feature being tested (e.g., native token bridging) is not supported by the implementation.
🪛 Gitleaks (8.21.2)
test/solidity/Facets/GlacisFacet.t.sol
376-376: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
390-390: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Actions: SC Core Dev Approval Check
test/solidity/Facets/GlacisFacet.t.sol
[warning] 219-225: Missing assertions to verify successful liquidity addition before performing swaps
[warning] 366-366: Detected a Generic API Key, potentially exposing access to various services and sensitive operations
[warning] 380-380: Detected a Generic API Key, potentially exposing access to various services and sensitive operations
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: generate-tag
- GitHub Check: analyze
🔇 Additional comments (2)
test/solidity/Facets/GlacisFacet.t.sol (2)
136-138: Empty test methods are appropriate here.The empty test methods with explanatory comments are acceptable since the facet does not support bridging of native assets, as indicated in the retrieved learnings.
Also applies to: 201-203
83-88: Add assertions to verify successful liquidity addition.The liquidity addition is crucial for the test setup, but there are no assertions to verify it was successful. Consider adding balance checks before and after the liquidity addition.
+ uint256 daiBalanceBefore = ERC20(ADDRESS_DAI).balanceOf(address(this)); + uint256 srcTokenBalanceBefore = srcToken.balanceOf(address(this)); + addLiquidity( ADDRESS_DAI, ADDRESS_SRC_TOKEN, 100_000 * 10 ** ERC20(ADDRESS_DAI).decimals(), 100_000 * 10 ** srcToken.decimals() ); + + uint256 daiBalanceAfter = ERC20(ADDRESS_DAI).balanceOf(address(this)); + uint256 srcTokenBalanceAfter = srcToken.balanceOf(address(this)); + + require( + daiBalanceBefore > daiBalanceAfter && + srcTokenBalanceBefore > srcTokenBalanceAfter, + "Liquidity addition failed" + );
Which Jira task belongs to this PR?
LF-11761
Why did I implement it this way?
The addLiquidity function was added to TaskBase because, in the case of Glacis, they do not provide routes for stablecoins like USDT or USDC. As a result, I had to work with more custom tokens that lack liquidity on Uniswap V2 and Sushiswap V2. This made it challenging to simulate the swapping process without having sufficient liquidity. To address this, I introduced an abstracted addLiquidity method in the parent TaskBase test suite. This abstraction allows for simulating liquidity addition upfront, enabling proper testing of swap functionality in such scenarios.
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)