Upgrade glacis facet [GlacisFacet v1.2.0, IGlacisAirlift v1.1.0] #1457
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Extend GlacisData struct with outputToken field for multibridge routing - Update IGlacisAirlift interface from v1.0.0 to v1.1.0 - Add support for 6-param send() and 7-param quoteSend() - Update tests to fork from Optimism at block 143581698 - Add documentation for breaking changes and new features BREAKING CHANGE: GlacisData struct now requires 4th field (outputToken)
Contributor
WalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (51)📓 Common learnings📚 Learning: 2025-07-04T08:59:08.108ZApplied to files:
📚 Learning: 2025-02-12T09:44:10.963ZApplied to files:
📚 Learning: 2025-08-07T10:20:01.383ZApplied to files:
📚 Learning: 2025-04-21T03:17:53.443ZApplied to files:
📚 Learning: 2025-09-09T10:39:26.383ZApplied to files:
📚 Learning: 2025-04-21T03:17:53.443ZApplied to files:
📚 Learning: 2025-04-21T03:15:12.236ZApplied to files:
📚 Learning: 2024-10-24T06:17:25.211ZApplied to files:
📚 Learning: 2025-09-22T00:52:26.172ZApplied to files:
📚 Learning: 2024-09-27T07:10:15.586ZApplied to files:
📚 Learning: 2025-08-29T10:02:09.041ZApplied to files:
📚 Learning: 2025-09-25T07:47:30.735ZApplied to files:
📚 Learning: 2025-11-25T01:36:33.521ZApplied to files:
📚 Learning: 2025-11-25T01:36:33.521ZApplied to files:
📚 Learning: 2025-02-11T10:35:03.536ZApplied to files:
📚 Learning: 2024-11-25T09:05:03.917ZApplied to files:
📚 Learning: 2025-07-16T01:03:08.106ZApplied to files:
📚 Learning: 2024-10-31T09:09:38.568ZApplied to files:
📚 Learning: 2025-10-10T10:56:04.861ZApplied to files:
📚 Learning: 2025-01-22T12:36:12.699ZApplied to files:
📚 Learning: 2025-09-16T01:39:54.099ZApplied to files:
📚 Learning: 2024-11-26T01:16:27.721ZApplied to files:
📚 Learning: 2025-11-25T01:36:33.521ZApplied to files:
📚 Learning: 2025-02-13T08:57:00.095ZApplied to files:
📚 Learning: 2025-08-27T23:36:40.773ZApplied to files:
📚 Learning: 2025-08-29T11:12:02.184ZApplied to files:
📚 Learning: 2025-08-07T10:20:01.383ZApplied to files:
📚 Learning: 2024-11-25T09:05:43.045ZApplied to files:
📚 Learning: 2024-12-04T01:59:34.045ZApplied to files:
📚 Learning: 2025-01-28T14:30:06.911ZApplied to files:
📚 Learning: 2025-06-13T08:30:26.220ZApplied to files:
📚 Learning: 2025-10-02T18:10:09.934ZApplied to files:
📚 Learning: 2025-11-25T01:36:33.521ZApplied to files:
📚 Learning: 2025-10-13T11:13:48.847ZApplied to files:
📚 Learning: 2025-02-12T09:44:12.961ZApplied to files:
📚 Learning: 2024-10-09T03:47:21.269ZApplied to files:
📚 Learning: 2024-10-09T03:47:21.269ZApplied to files:
📚 Learning: 2025-08-26T12:11:21.073ZApplied to files:
📚 Learning: 2025-07-15T05:05:28.134ZApplied to files:
📚 Learning: 2025-02-11T10:33:52.791ZApplied to files:
📚 Learning: 2025-05-28T17:33:33.959ZApplied to files:
📚 Learning: 2025-05-28T17:33:10.529ZApplied to files:
📚 Learning: 2025-09-15T12:33:51.069ZApplied to files:
📚 Learning: 2024-09-30T03:52:27.281ZApplied to files:
📚 Learning: 2025-11-25T01:36:33.521ZApplied to files:
📚 Learning: 2025-01-21T11:07:36.236ZApplied to files:
📚 Learning: 2024-10-09T03:47:21.269ZApplied to files:
📚 Learning: 2025-01-21T11:04:46.116ZApplied to files:
📚 Learning: 2025-05-27T16:19:09.643ZApplied to files:
📚 Learning: 2024-11-21T08:39:29.530ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lifi-action-bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/solidity/Facets/GlacisFacet.t.sol (1)
402-409: Optional: Consider extracting the outputToken conversion logic.The pattern
outputToken == address(0) ? bytes32(0) : bytes32(uint256(uint160(outputToken)))is repeated in many test functions. While this is acceptable for tests, you could optionally extract it to a helper function in the test base to reduce duplication:function getOutputTokenBytes32() internal view returns (bytes32) { return outputToken == address(0) ? bytes32(0) : bytes32(uint256(uint160(outputToken))); }Then use:
outputToken: getOutputTokenBytes32()Also applies to: 456-463, 481-488, 512-519, 539-546, 570-577, 603-610, 632-639, 670-677, 708-715, 740-747
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
docs/GlacisFacet.md(3 hunks)script/demoScripts/demoGlacis.ts(5 hunks)src/Facets/GlacisFacet.sol(3 hunks)src/Interfaces/IGlacisAirlift.sol(4 hunks)test/solidity/Facets/GlacisFacet.t.sol(16 hunks)
🧰 Additional context used
🧠 Learnings (39)
📓 Common learnings
Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: src/Facets/GlacisFacet.sol:1-3
Timestamp: 2025-01-28T11:28:16.225Z
Learning: The GlacisFacet contract audit will be conducted and added to the audit log in later stages of development. This is acceptable during the initial development phase.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1357
File: deployments/lens.diamond.json:48-51
Timestamp: 2025-09-09T10:39:26.383Z
Learning: In the lifinance/contracts repository, when deployment JSON files show address changes (like AcrossFacetV3 address updates in deployments/*.diamond.json), the corresponding _deployments_log_file.json updates may be handled in separate PRs rather than the same PR that updates the diamond configuration files.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T06:25:01.232Z
Learning: Contracts like `OpBNBBridgeFacet` may be in development and only available in feature branches, resulting in missing source files in the main branch. Before flagging such missing contracts, check for open PRs that might include them.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1283
File: deployments/ronin.diamond.json:65-68
Timestamp: 2025-08-07T10:20:01.383Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) and deployment log files have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs involving address updates like the RelayFacet to Worldchain, verify the actual presence of entries in files before reporting issues. The RelayFacet exists in the diamond log file and the PR diff already contains the necessary address change.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1334
File: deployments/mainnet.json:54-54
Timestamp: 2025-08-26T02:20:52.515Z
Learning: For deployment PRs in the lifinance/contracts repository, carefully verify the specific scope mentioned in the PR title and description before suggesting updates to other networks. Not all deployments are cross-network updates - some are targeted to specific chains only.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/UniV2StyleFacet.sol:0-0
Timestamp: 2025-08-27T13:47:28.646Z
Learning: In src/Periphery/Lda/Facets/UniV2StyleFacet.sol and similar LDA facets, mirooon prefers to rely on backend validation for pool addresses rather than adding contract code-size checks in the smart contract, as pool validation occurs during payload generation and transactions would fail anyway if sent to invalid addresses.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/_targetState.json:49-49
Timestamp: 2024-11-25T09:05:03.917Z
Learning: The `RelayFacet` contract, when missing from the source code but referenced in deployment configurations, should be treated the same way as `OpBNBBridgeFacet` and can be ignored in code reviews.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs updating contract addresses (like RelayFacet on Worldchain), verify the presence of entries in all relevant files (worldchain.json, worldchain.diamond.json, _deployments_log_file.json) before reporting inconsistencies. The RelayFacet entry exists in all required deployment files with the correct address.
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.
📚 Learning: 2024-10-31T09:09:38.568Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 843
File: src/Facets/RelayFacet.sol:165-170
Timestamp: 2024-10-31T09:09:38.568Z
Learning: In the `RelayFacet` contract (`src/Facets/RelayFacet.sol`), within the `_startBridge` function, low-level `call` is intentionally used to transfer tokens so that extra bytes can be added to the calldata, as required for integrating with Relay Protocol.
Applied to files:
docs/GlacisFacet.mdsrc/Facets/GlacisFacet.solsrc/Interfaces/IGlacisAirlift.solscript/demoScripts/demoGlacis.tstest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-09-22T00:52:26.172Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1388
File: deployments/hyperevm.diamond.json:72-75
Timestamp: 2025-09-22T00:52:26.172Z
Learning: In diamond configuration files (deployments/*.diamond.json), it's acceptable to have multiple versions of the same facet (e.g., GlacisFacet v1.0.0 and v1.1.0) deployed at different contract addresses. This is intentional design for version coexistence, gradual migration, or backward compatibility purposes.
Applied to files:
docs/GlacisFacet.md
📚 Learning: 2025-01-28T11:28:16.225Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: src/Facets/GlacisFacet.sol:1-3
Timestamp: 2025-01-28T11:28:16.225Z
Learning: The GlacisFacet contract audit will be conducted and added to the audit log in later stages of development. This is acceptable during the initial development phase.
Applied to files:
docs/GlacisFacet.mdsrc/Facets/GlacisFacet.soltest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-08-27T23:36:40.773Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
Applied to files:
docs/GlacisFacet.mdsrc/Facets/GlacisFacet.solsrc/Interfaces/IGlacisAirlift.soltest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-10-10T10:56:04.861Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearFacet.sol:4-13
Timestamp: 2025-10-10T10:56:04.861Z
Learning: LibAllowList is only required for facets that make arbitrary external calls to DEX aggregators (e.g., GenericSwapFacetV3). Bridge facets that call specific protocol contracts (like EverclearFacet calling IEverclearFeeAdapter) do not need to import LibAllowList.
Applied to files:
docs/GlacisFacet.mdsrc/Facets/GlacisFacet.solsrc/Interfaces/IGlacisAirlift.sol
📚 Learning: 2025-01-22T12:36:12.699Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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.
Applied to files:
docs/GlacisFacet.mdsrc/Facets/GlacisFacet.soltest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2024-11-25T09:04:55.880Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/_targetState.json:143-143
Timestamp: 2024-11-25T09:04:55.880Z
Learning: Errors about the missing `OpBNBBridgeFacet` contract are expected when it is referenced in the target state but not yet merged into the main branch.
Applied to files:
docs/GlacisFacet.mdsrc/Interfaces/IGlacisAirlift.soltest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
Applied to files:
docs/GlacisFacet.mdsrc/Facets/GlacisFacet.solsrc/Interfaces/IGlacisAirlift.soltest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-08-27T13:07:58.254Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CurveFacet.sol:0-0
Timestamp: 2025-08-27T13:07:58.254Z
Learning: The CurveFacet in src/Periphery/Lda/Facets/CurveFacet.sol has fundamental issues: it imports a non-existent ICurveLegacy interface, and attempts to send ETH to non-payable exchange functions in both ICurve and ICurveV2 interfaces. All current Curve interfaces are non-payable and cannot accept native ETH.
Applied to files:
docs/GlacisFacet.mdsrc/Facets/GlacisFacet.sol
📚 Learning: 2025-09-16T07:56:45.093Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1324
File: test/solidity/Facets/EcoFacet.t.sol:135-141
Timestamp: 2025-09-16T07:56:45.093Z
Learning: In EcoFacet test setup, the isNative parameter in swap-and-bridge flows refers to the bridge token type (output), not the swap input type. The ERC20 branch (isNative=false) can still swap from native ETH to ERC20 tokens, requiring msg.value = swapData[0].fromAmount for the ETH input to the swap.
Applied to files:
docs/GlacisFacet.mdtest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-02-13T08:57:00.095Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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
Applied to files:
docs/GlacisFacet.mdsrc/Facets/GlacisFacet.solsrc/Interfaces/IGlacisAirlift.solscript/demoScripts/demoGlacis.tstest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-07-16T01:04:11.083Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1275
File: src/Facets/AllBridgeFacet.sol:175-175
Timestamp: 2025-07-16T01:04:11.083Z
Learning: In AllBridgeFacet.sol, zero fee validation is not required at the contract level because the backend system ensures that calldata and msg.value are properly aligned before transactions reach the contract.
Applied to files:
docs/GlacisFacet.mdsrc/Interfaces/IGlacisAirlift.sol
📚 Learning: 2025-08-29T10:02:09.041Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1299
File: src/Facets/AcrossFacetPackedV4.sol:118-136
Timestamp: 2025-08-29T10:02:09.041Z
Learning: In AcrossFacetPackedV4.sol, the team explicitly chooses to omit calldata length validation in gas-optimized packed functions like startBridgeTokensViaAcrossV4NativePacked to save gas, accepting the trade-off of potential out-of-bounds reverts for better gas efficiency.
Applied to files:
docs/GlacisFacet.mdsrc/Interfaces/IGlacisAirlift.solscript/demoScripts/demoGlacis.tstest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-08-29T11:07:57.743Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/CurveFacet.sol:87-91
Timestamp: 2025-08-29T11:07:57.743Z
Learning: In src/Periphery/LDA/Facets/CurveFacet.sol, modern Curve pools (isV2=true, representing V2/NG pools) should reject native tokenIn by adding an early revert check when LibAsset.isNativeAsset(tokenIn) is true, since ICurveV2 exchange functions are non-payable and cannot accept native ETH.
Applied to files:
src/Facets/GlacisFacet.soltest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-06-13T08:30:26.220Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1207
File: deployments/_deployments_log_file.json:34037-34080
Timestamp: 2025-06-13T08:30:26.220Z
Learning: LiFiDiamond contains generic withdrawal logic, so individual facet contracts (e.g., PioneerFacet) do not need their own Ether-withdraw functions.
Applied to files:
src/Facets/GlacisFacet.sol
📚 Learning: 2025-08-27T13:07:58.254Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CurveFacet.sol:0-0
Timestamp: 2025-08-27T13:07:58.254Z
Learning: The CurveFacet implementation is fundamentally broken: ICurveLegacy interface doesn't exist in the codebase but is imported and used, and the code attempts to send ETH to non-payable exchange functions. The entire native asset handling logic needs to be redesigned.
Applied to files:
src/Facets/GlacisFacet.sol
📚 Learning: 2025-10-02T18:10:09.934Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1406
File: src/Facets/UnitFacet.sol:75-81
Timestamp: 2025-10-02T18:10:09.934Z
Learning: In UnitFacet.sol and similar facet contracts in src/Facets/, the LiFiTransferStarted event emission should ALWAYS be at the end of the _startBridge internal function, after all validations and asset transfers have been completed successfully.
Applied to files:
src/Facets/GlacisFacet.sol
📚 Learning: 2025-04-28T07:46:24.084Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.
Applied to files:
src/Facets/GlacisFacet.sol
📚 Learning: 2025-02-11T10:35:03.536Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 988
File: script/tasks/solidity/AddTokenApprovalsToCBridgeFacetPacked.s.sol:17-21
Timestamp: 2025-02-11T10:35:03.536Z
Learning: The CBridgeFacetPacked and cBridge addresses in AddTokenApprovalsToCBridgeFacetPacked.s.sol must not be zero addresses as they are required for token approvals to function properly.
Applied to files:
src/Interfaces/IGlacisAirlift.soltest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2024-11-22T08:49:35.205Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-11-22T08:49:35.205Z
Learning: In `DeBridgeDlnFacet.sol`, direct access to the `deBridgeChainId` mapping is acceptable, and using the `getDeBridgeChainId` function is not necessary in these instances.
Applied to files:
src/Interfaces/IGlacisAirlift.sol
📚 Learning: 2025-07-17T04:21:55.549Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.
Applied to files:
script/demoScripts/demoGlacis.ts
📚 Learning: 2025-09-25T00:12:58.536Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.
Applied to files:
script/demoScripts/demoGlacis.tstest/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-02-21T09:05:22.118Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 984
File: test/solidity/Facets/ChainflipFacet.t.sol:269-269
Timestamp: 2025-02-21T09:05:22.118Z
Learning: Fixed gas amounts in test files (e.g., ChainflipFacet.t.sol) don't require extensive documentation or configuration as they are only used for testing purposes.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-08-29T11:12:02.184Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs involving address updates like the RelayFacet to Worldchain, verify the actual presence of entries in files before reporting issues. The RelayFacet exists in the diamond log file and the PR diff already contains the necessary address change.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-01-28T11:29:09.566Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 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.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-08-29T11:53:38.549Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2024-10-22T03:24:24.705Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: test/solidity/Facets/GasZipFacet.t.sol:337-339
Timestamp: 2024-10-22T03:24:24.705Z
Learning: In `GasZipFacet` tests, when expecting a failure due to insufficient balance for sending value, the test should expect a generic revert without specifying a specific error.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-01-22T12:38:37.557Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: test/solidity/utils/TestBase.sol:446-470
Timestamp: 2025-01-22T12:38:37.557Z
Learning: Test utility functions in TestBase.sol don't require production-level validations like slippage checks and input validation since they operate in a controlled test environment.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-08-20T09:56:24.259Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-09-19T14:10:55.064Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1324
File: src/Facets/EcoFacet.sol:68-71
Timestamp: 2025-09-19T14:10:55.064Z
Learning: In src/Facets/EcoFacet.sol, the team prefers to use msg.sender as the refund address for native tokens in the refundExcessNative modifier, contrary to the general guideline of requiring explicit refund parameters.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-08-27T22:23:51.257Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/NativeWrapperFacet.sol:0-0
Timestamp: 2025-08-27T22:23:51.257Z
Learning: In src/Periphery/Lda/Facets/NativeWrapperFacet.sol, the wrapNative function should not validate msg.value == amountIn because CoreRouteFacet's DistributeNative command (command type 3) overrides the amountIn parameter with address(this).balance, making such validation incorrect and causing false failures.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-10-13T11:13:48.847Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.847Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.393Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-04-04T07:21:52.878Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1086
File: test/solidity/utils/TestBase.sol:195-205
Timestamp: 2025-04-04T07:21:52.878Z
Learning: In the TestBase.sol file, the Optimism network constants are set correctly with:
- ADDRESS_UNISWAP_OPTIMISM: 0x4A7b5Da61326A6379179b40d00F57E5bbDC962c2 (Uniswap V2 Router)
- ADDRESS_USDC_OPTIMISM: 0x0b2C639c533813f4Aa9D7837CAf62653d097Ff85 (Native USDC)
- USDC.e (bridged USDC from Ethereum) on Optimism has address 0x7f5c764cbc14f9669b88837ca1490cca17c31607
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-05-06T09:09:38.108Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1117
File: test/solidity/Periphery/LiFiDEXAggregator.t.sol:111-127
Timestamp: 2025-05-06T09:09:38.108Z
Learning: The `setupApechain()` function in LiFiDEXAggregator tests intentionally avoids calling `initTestBase()` to prevent needing to define standard contracts (like ADDRESS_USDC_PROXY and Uniswap) that aren't needed for Apechain-specific tests.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-08-28T02:41:07.505Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1321
File: docs/RelayDepositoryFacet.md:9-10
Timestamp: 2025-08-28T02:41:07.505Z
Learning: Unit tests for RelayDepositoryFacet cannot verify fund forwarding behavior after deposits because the facet delegates to external IRelayDepository contracts. The forwarding logic is implemented in the Relay Protocol V2 Depository contracts, not in the facet itself.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
📚 Learning: 2025-06-15T08:47:22.079Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1208
File: test/solidity/utils/TestAMM.sol:18-22
Timestamp: 2025-06-15T08:47:22.079Z
Learning: For test contracts in the lifinance/contracts repository, simpler error handling approaches are acceptable and preferred over more complex error bubbling mechanisms, as the focus is on functionality rather than detailed error reporting.
Applied to files:
test/solidity/Facets/GlacisFacet.t.sol
🪛 Gitleaks (8.28.0)
test/solidity/Facets/GlacisFacet.t.sol
[high] 774-774: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 775-775: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 793-793: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 794-794: 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (11)
docs/GlacisFacet.md (1)
3-88: LGTM! Comprehensive documentation for the multibridge routing feature.The documentation clearly explains:
- Version information and compatibility
- The new
outputTokenparameter and its behavior- Default routing vs. specific token routing with concrete examples
- Breaking change notice with migration guidance
This will help integrators understand and adopt the new functionality.
src/Facets/GlacisFacet.sol (2)
35-41: LGTM! Struct extension is clean and well-documented.The new
outputTokenfield is properly documented, and the bytes32 type is appropriate for representing either an address (padded) or the zero value for default routing.
155-168: LGTM! AIRLIFT integration correctly updated.The 6-parameter
send()call properly includes the newoutputTokenparameter, and the comments clearly explain the routing behavior. Event emission follows the team standard of emittingLiFiTransferStartedat the end of the_startBridgefunction.script/demoScripts/demoGlacis.ts (1)
44-46: LGTM! Demo script properly reflects the new API.The script correctly demonstrates:
- Using bytes32(0) for default routing in both
quoteSendandGlacisData- Extended function signature in the calldata output
- Debug logging for the new parameter
This will help developers understand how to use the multibridge routing feature.
Also applies to: 137-137, 183-183, 220-224, 248-248
test/solidity/Facets/GlacisFacet.t.sol (3)
12-15: LGTM! Test infrastructure properly extended for multibridge routing.The test base correctly:
- Documents the new multibridge routing capability
- Adds
outputTokenas an address field for test configuration- Converts it to bytes32 in GlacisData initialization with proper default handling
The conversion logic
outputToken == address(0) ? bytes32(0) : bytes32(uint256(uint160(outputToken)))correctly handles both default routing and specific token scenarios.Also applies to: 30-30, 139-146
43-60: LGTM! Enhanced test setup for multibridge scenarios.The setUp now properly funds the diamond and relevant addresses with both tokens and native currency to support multibridge routing tests.
765-801: LGTM! Test configurations for multibridge tokens.The test contracts for OpenUSDT and USDT0 properly configure:
- Optimism fork with specific block number for consistency
- Correct Airlift contract address
- Source and output token addresses for multibridge routing
- Destination chain (Unichain)
- Appropriate fuzzing ranges (1 to 10,000)
Note: The static analysis warnings about "API keys" on lines 774, 775, 793, 794 are false positives—these are legitimate Ethereum contract addresses.
src/Interfaces/IGlacisAirlift.sol (4)
2-2: LGTM! Version bump to 1.1.0 is appropriate.The minor version increment correctly reflects the addition of new function overloads and data structures.
23-36: LGTM! New data structures support routing functionality.The
TokenFacetConfigandBridgeRoutestructs provide the necessary infrastructure for multibridge routing:
TokenFacetConfig: Maps entry points and bridge keys to facet configurationsBridgeRoute: Defines routing parameters including destination chain, output token, and bridge selection
48-72: LGTM! Function overloads maintain interface compatibility.The interface extension strategy is sound:
- Existing
send()signature updated to returnbytes memory(line 54)- New
send()overload addsoutputTokenparameter (lines 65-72)- Documentation clearly explains the routing behavior
- Overload pattern allows callers to choose based on their routing needs
82-108: LGTM! quoteSend overload properly extends the quoting API.The new
quoteSend()overload (lines 100-108) correctly mirrors thesend()extension by adding theoutputTokenparameter, enabling fee estimation for specific routing scenarios. Documentation is clear.
Collaborator
Test Coverage ReportLine Coverage: 85.46% (2828 / 3309 lines) |
Collaborator
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
…nto upgrade-glacis-facet
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
script/tasks/diamondUpdateFacet.sh (2)
132-138: Inconsistent skip-simulation flag handling between zkEVM and non-zkEVM paths.Line 134 (zkEVM) hardcodes
--skip-simulationwhile line 137 (non-zkEVM) uses the dynamic$SKIP_SIMULATION_FLAGvariable. This inconsistency may be intentional (e.g., zkEVM always requires skip-simulation), but should be clarified via a comment or unified if unintended.For reference, line 123 sets
SKIP_SIMULATION_FLAGbased on environment configuration.
174-178: Use consistent private key handling across zkEVM and non-zkEVM STAGING paths.Line 175 (zkEVM) passes
--private-key $(getPrivateKey ...)directly as a flag, while line 177 (non-zkEVM) assigns the private key to an environment variable first (PRIVATE_KEY=$(getPrivateKey ...)). This asymmetry creates maintenance friction and the unquoted command substitution on line 175 triggers ShellCheck SC2046.For consistency and robustness, refactor line 175 to match line 177's approach:
if isZkEvmNetwork "$NETWORK"; then - RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND ./foundry-zksync/forge script "$SCRIPT_PATH" -f "$NETWORK" --json --broadcast --skip-simulation --slow --zksync --gas-limit 50000000 --private-key $(getPrivateKey "$NETWORK" "$ENVIRONMENT")) + RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_MUTABLE_DIAMOND PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") ./foundry-zksync/forge script "$SCRIPT_PATH" -f "$NETWORK" --json --broadcast --skip-simulation --slow --zksync --gas-limit 50000000)This aligns with line 177 and eliminates the ShellCheck quoting warning.
script/deploy/zksync/utils/ScriptBase.sol (1)
1-2: License identifier differs from stated repo guidelinesGuidelines specify
SPDX-License-Identifier: LGPL-3.0-onlyfor Solidity files underscript/, but this file still usesUNLICENSED. Consider aligning the header here (and elsewhere underscript/) with the repo standard, ideally in a small follow‑up PR so it’s applied consistently.script/helperFunctions.sh (2)
658-660: Make getZkSolcVersion resilient: fallback section + quote-agnostic parsing.Current grep only reads [profile.default.zksync] and assumes single quotes. Add fallback to [profile.zksync] and support ' or " to avoid breakage across repos.
function getZkSolcVersion() { local NETWORK="$1" if isZkEvmNetwork "$NETWORK"; then - # Extract zksolc version from default.zksync profile section - grep -A 10 "^\[profile\.default\.zksync\]" foundry.toml | grep "zksolc" | cut -d "'" -f 2 + # Try default.zksync first, then legacy [profile.zksync]; accept ' or " quotes + local v + v=$(awk ' + BEGIN{section=""} + /^\[profile\.default\.zksync\]/{section="default.zksync"; next} + /^\[profile\.zksync\]/{if (section=="") section="zksync"; next} + /^\[profile\./{if (section!="") exit} + section!="" && $0 ~ /zksolc/ { + if (match($0, /[\"\047]([^\"\047]+)[\"\047]/, m)) { print m[1]; exit } + }' foundry.toml) + echo "${v}" else echo "" fi }
4643-4643: Respect env override for FOUNDRY_ZKSYNC_VERSION (doc/code mismatch).Header says env var can specify version, but local var shadows it. Use defaulting to v0.0.32 while allowing override.
-install_foundry_zksync() { - # Foundry ZKSync version - local FOUNDRY_ZKSYNC_VERSION="v0.0.32" +install_foundry_zksync() { + # Foundry ZKSync version (allow env override, default to v0.0.32) + local FOUNDRY_ZKSYNC_VERSION="${FOUNDRY_ZKSYNC_VERSION:-v0.0.32}"Please confirm v0.0.32 matches the repo’s expected foundry-zksync release; if not, set it via env when calling the function.
script/deploy/zksync/DeployWhitelistManagerFacet.zksync.s.sol (1)
1-1: License identifier should be LGPL-3.0-only per coding guidelines.As per coding guidelines, Solidity files in
script/directory should useLGPL-3.0-onlylicense identifier. However, this may be intentional for zkSync-specific deployment scripts. Please verify ifUNLICENSEDis the intended license for this file.-// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: LGPL-3.0-onlyscript/deploy/zksync/UpdateWhitelistManagerFacet.zksync.s.sol (1)
22-33: Salt computation duplicated across scripts - consider extracting to shared constant.The salt computation (
keccak256(abi.encodePacked("MigrateWhitelistManager"))) is duplicated between this file andDeployWhitelistManagerFacet.zksync.s.sol. While the comment on line 28 acknowledges this must match, consider extracting to a shared constant in a base contract to prevent drift.That said, the current implementation is correct and the explicit comment helps maintain synchronization.
script/deploy/zksync/utils/UpdateScriptBase.sol (4)
8-24: New AccessManagerFacet import and helper structs look consistent; Approval currently unusedThe AccessManagerFacet import and the new
FunctionSelector/InvalidHexDigitadditions fit the existing patterns and look fine.Note:
Approvalis defined but not used within this base contract. If it is only intended for future external use (e.g., as a shared type in other scripts), that’s fine; otherwise, consider removing it or wiring it into actual logic to avoid dead code.
58-117: Clarify and constrainupdatersemantics for diamondCut_initto avoid silent misconfigurationThe new
update(string memory name, address updater)overload and wiring of:callData.length > 0 ? updater : address(0)into both the encoded
cutDataand the actualcutter.diamondCutcall cleanly generalize the initializer target beyondfacetitself.However, this makes
updaterthe_initaddress for the diamondCut delegatecall. If a derived script accidentally passes an EOA or a contract that does not implement the initializer encoded ingetCallData(), thediamondCutinitializer delegatecall can either:
- Succeed but do nothing (EOA, no code), leaving state uninitialized, or
- Revert in a non-obvious way (wrong contract/selector).
To make this harder to misuse, I’d suggest either:
- Documenting clearly (in a comment/NatSpec) that
updatermust be the address of the contract containing the initializer function referenced bygetCallData(), or- Narrowing the API so that common cases call
update(name)(current default:updater == facet), and only specialized scripts that truly need a different_inittarget use the 2‑arg overload with explicit comments at the call sites.This is mostly about preventing subtle configuration errors rather than changing behavior.
225-232: Minor nit:0 <= dcheck is redundant foruint8
dis auint8, so the0 <= dpart of the first branch is always true:if (0 <= d && d <= 9) { ... }This could be simplified to
if (d <= 9)for marginally clearer intent:if (d <= 9) { return bytes1(uint8(bytes1("0")) + d); } else if (d <= 15) { return bytes1(uint8(bytes1("a")) + d - 10); } revert InvalidHexDigit(d);The current code is correct; this is just a readability micro‑improvement.
245-321: Ensure approval helpers run within a broadcast context and consider path/json reuse side‑effectsThe new
approveRefundWalletandapproveDeployerWallethelpers are a useful addition: they pullFunctionSelector[]configs fromconfig/global.jsonand wire per‑selector permissions viaAccessManagerFacet(diamond).setCanExecute(bytes4(selector), wallet, true). The use ofFunctionSelector.selectorasbytesand casting tobytes4is reasonable given JSON/stdJson constraints.Two points to double‑check:
Broadcast context for
setCanExecutecallsThese helpers do not call
vm.startBroadcast/vm.stopBroadcastand they ignorenoBroadcast. In this base, onlyupdate()manages broadcasting:
- If typical update scripts only rely on
UpdateScriptBase.update(...)for broadcasting (i.e.,run()does not wrap everything in its ownvm.startBroadcast), then callingapproveRefundWallet()/approveDeployerWallet()afterupdate()will executesetCanExecutein a non‑broadcasted context, so no on‑chain state change will occur.- To avoid surprises, either:
- Call these helpers from within a broadcast section in derived scripts (e.g., wrap them in a
vm.startBroadcast(deployerPrivateKey)/vm.stopBroadcast()inrun()), or- Move the broadcast handling into shared helpers that also cover approvals, or
- Have these helpers themselves respect
noBroadcastand optionally manage broadcast similarly toupdate().Please verify how current update scripts are calling these functions so the intended approvals are actually mined.
Mutation of shared
path/jsonstateBoth helpers overwrite the contract‑level
pathandjsonto point atconfig/global.json. If any later logic in the same script expectspath/jsonto still reference the original deployments file, this could lead to confusing behavior. If that’s a concern, consider using local variables here:string memory globalPath = string.concat(root, "/config/global.json"); string memory globalJson = vm.readFile(globalPath);and then read from
globalJsoninstead of the sharedjsonfield.Overall structure is solid; the main thing is ensuring these approval calls are executed under the correct broadcast/ownership context.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)