LF-13997 LDA 2.0 #1328
LF-13997 LDA 2.0 #1328
Conversation
…ggregatorVelodromeV2UpgradeTest tests
… improved network validation logic
…tyleDex facets and added getDefaultAmountIn
…orTokenIn, improved event handling, and standardized swap execution logic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (7)
deployments/_deployments_log_file.json(13 hunks)deployments/apechain.diamond.lda.staging.json(1 hunks)deployments/apechain.staging.json(1 hunks)deployments/celo.diamond.lda.staging.json(1 hunks)deployments/celo.staging.json(1 hunks)deployments/ronin.diamond.lda.staging.json(1 hunks)deployments/ronin.staging.json(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- deployments/apechain.diamond.lda.staging.json
- deployments/ronin.diamond.lda.staging.json
- deployments/celo.staging.json
- deployments/ronin.staging.json
🧰 Additional context used
🧠 Learnings (25)
📓 Common learnings
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `deployments/*.diamond.json` and `deployments/*.json` files, the `LiFiDEXAggregator` contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/LDA/Facets/VelodromeV2Facet.sol:5-11
Timestamp: 2025-08-29T11:13:18.076Z
Learning: The correct directory structure in the LiFi contracts codebase is src/Periphery/LDA/ (uppercase "LDA"), not src/Periphery/Lda/ (mixed case). Import paths should use "lifi/Periphery/LDA/..." to match the actual filesystem structure. The directory src/Periphery/LDA/ exists and contains all the LDA facets, while src/Periphery/Lda/ does not exist.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#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: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/LDA/Facets/UniV3StyleFacet.sol:4-12
Timestamp: 2025-08-29T11:13:11.137Z
Learning: The correct directory structure for LDA imports in the LiFi contracts codebase is "src/Periphery/LDA/" (uppercase). Import paths should use "lifi/Periphery/LDA/..." to match the actual filesystem structure. The files exist at paths like src/Periphery/LDA/PoolCallbackAuthenticated.sol, src/Periphery/LDA/Errors/Errors.sol, and src/Periphery/LDA/BaseRouteConstants.sol.
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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:
deployments/apechain.staging.jsondeployments/celo.diamond.lda.staging.jsondeployments/_deployments_log_file.json
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: deployments/bsc.diamond.staging.json:4-7
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In files with "staging" in their filename, such as `deployments/bsc.diamond.staging.json`, it is acceptable for facets to have empty "Name" and "Version" fields. Do not flag missing facet information in such files during reviews.
Applied to files:
deployments/apechain.staging.jsondeployments/_deployments_log_file.json
📚 Learning: 2025-09-22T00:52:26.172Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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:
deployments/apechain.staging.jsondeployments/celo.diamond.lda.staging.jsondeployments/_deployments_log_file.json
📚 Learning: 2025-09-09T10:39:26.383Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
Applied to files:
deployments/apechain.staging.jsondeployments/celo.diamond.lda.staging.jsondeployments/_deployments_log_file.json
📚 Learning: 2024-11-01T11:53:57.162Z
Learnt from: ezynda3
PR: lifinance/contracts#846
File: deployments/cronos.diamond.json:28-31
Timestamp: 2024-11-01T11:53:57.162Z
Learning: In `deployments/cronos.diamond.json`, both `GenericSwapFacet` and `GenericSwapFacetV3` are distinct facets that should be included together, as they serve different purposes.
Applied to files:
deployments/apechain.staging.jsondeployments/celo.diamond.lda.staging.json
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
Applied to files:
deployments/apechain.staging.jsondeployments/_deployments_log_file.json
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `deployments/*.diamond.json` and `deployments/*.json` files, the `LiFiDEXAggregator` contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.
Applied to files:
deployments/apechain.staging.jsondeployments/celo.diamond.lda.staging.jsondeployments/_deployments_log_file.json
📚 Learning: 2024-09-27T07:10:15.586Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#812
File: deployments/polygon.diamond.json:4-11
Timestamp: 2024-09-27T07:10:15.586Z
Learning: In `deployments/polygon.diamond.json`, it's acceptable for certain facets to have empty names and versions when specified by the developer.
Applied to files:
deployments/apechain.staging.jsondeployments/celo.diamond.lda.staging.json
📚 Learning: 2025-02-12T09:44:12.961Z
Learnt from: mirooon
PR: lifinance/contracts#985
File: script/deploy/_targetState.json:0-0
Timestamp: 2025-02-12T09:44:12.961Z
Learning: The bsca network intentionally maintains different facet versions between staging and production environments, specifically:
1. CalldataVerificationFacet: v1.1.1 in staging vs v1.1.2 in production
2. EmergencyPauseFacet: present only in production
3. Permit2Proxy: present only in production
Applied to files:
deployments/apechain.staging.jsondeployments/_deployments_log_file.json
📚 Learning: 2025-04-21T03:15:12.236Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1109
File: deployments/worldchain.diamond.json:84-85
Timestamp: 2025-04-21T03:15:12.236Z
Learning: In deployment JSON files that contain "diamond" in their filename (located in the deployments folder), periphery contracts may have empty string values for their addresses. This indicates that the contract is not deployed on that particular chain and should not be flagged as an issue during code reviews.
Applied to files:
deployments/apechain.staging.jsondeployments/celo.diamond.lda.staging.jsondeployments/_deployments_log_file.json
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: ezynda3
PR: lifinance/contracts#806
File: deployments/_deployments_log_file.json:5780-5793
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The owner address `0x11f11121df7256c40339393b0fb045321022ce44` and the `DiamondCutFacet` address `0xd5cf40a2a18b633cfd6a1ae16d1771596498cf83` in the `LiFiDiamond` deployment on `xlayer` are correct and should not be flagged as issues, even if they are not referenced in the Solidity files.
Applied to files:
deployments/apechain.staging.jsondeployments/_deployments_log_file.json
📚 Learning: 2025-01-28T14:30:06.911Z
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Applied to files:
deployments/apechain.staging.jsondeployments/celo.diamond.lda.staging.json
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Facet contracts must import/use: ILiFi, LibAsset, LibSwap, LibAllowList; and use security mixins ReentrancyGuard, SwapperV2, Validatable; ECDSA optional
Applied to files:
deployments/apechain.staging.json
📚 Learning: 2025-09-16T01:39:54.099Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1381
File: deployments/arbitrum.json:65-69
Timestamp: 2025-09-16T01:39:54.099Z
Learning: When verifying facet deployments across .json and .diamond.json files, search by facet name rather than trying to cross-reference addresses between the files, as the same contract can have different addresses in different deployment files.
Applied to files:
deployments/apechain.staging.jsondeployments/celo.diamond.lda.staging.json
📚 Learning: 2024-11-26T01:01:18.499Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/arbitrum.diamond.json:148-150
Timestamp: 2024-11-26T01:01:18.499Z
Learning: In `deployments/*.diamond.json` configuration files, facets that are part of an open PR and not yet merged into the main branch may have missing `Name` and `Version` fields.
Applied to files:
deployments/celo.diamond.lda.staging.json
📚 Learning: 2024-11-26T01:04:16.637Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/fantom.diamond.json:92-94
Timestamp: 2024-11-26T01:04:16.637Z
Learning: In `deployments/*.diamond.json` files, it's acceptable for facets to have empty `Name` and `Version` fields, and these should not be flagged as issues.
Applied to files:
deployments/celo.diamond.lda.staging.json
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
Applied to files:
deployments/celo.diamond.lda.staging.json
📚 Learning: 2024-12-04T01:59:34.045Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#832
File: deployments/_deployments_log_file.json:23712-23720
Timestamp: 2024-12-04T01:59:34.045Z
Learning: In `deployments/_deployments_log_file.json`, duplicate deployment entries for the same version and address may occur because they correspond to deployments on different networks. These entries are acceptable and should not be flagged as duplicates in future reviews.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-08-07T10:20:01.383Z
Learnt from: mirooon
PR: lifinance/contracts#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.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-07-04T08:59:08.108Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-08-07T10:20:01.383Z
Learnt from: mirooon
PR: lifinance/contracts#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, understand that _targetState.json tracks contract version numbers (not addresses) and _deployments_log_file.json contains deployment records with addresses that may not be organized in obvious network sections. Always verify the actual file structure before flagging missing entries.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2024-11-26T01:03:43.597Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/scroll.diamond.json:111-111
Timestamp: 2024-11-26T01:03:43.597Z
Learning: When reviewing pull requests, only point out missing contract addresses in `deployments/<network>.diamond.json` if the contract's address is present in `deployments/<network>.json` but missing in `deployments/<network>.diamond.json`. If the contract is not deployed on a network (i.e., not present in `deployments/<network>.json`), then no action is needed.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-06-16T09:28:18.027Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2024-11-25T09:05:43.045Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/base.diamond.json:148-148
Timestamp: 2024-11-25T09:05:43.045Z
Learning: In deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contracts like `Permit2Proxy` may be placeholders and will be updated after approvals are released from the multisig safe.
Applied to files:
deployments/_deployments_log_file.json
🔇 Additional comments (1)
deployments/_deployments_log_file.json (1)
40533-40536: Constructor calldata for BSC LDADiamond is still missingThe BSC staging entry for
LiFiDEXAggregatorDiamondcontinues to log"CONSTRUCTOR_ARGS": "0x", yet the LDADiamond constructor requires two address parameters (_contractOwner,_diamondCutFacet). The other network entries already include the 64‑byte encoded calldata, so leaving this as0xkeeps the deployment record inaccurate. Please replace the placeholder with the actual ABI-encoded constructor arguments used for this deployment.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/solidity/Facets/AllBridgeFacet.t.sol (1)
1-2: Fix SPDX header and spacing per repo guidelinesUse LGPL-3.0-only and add the required blank line before pragma.
As per coding guidelines.
-// SPDX-License-Identifier: Unlicense -pragma solidity ^0.8.17; +// SPDX-License-Identifier: LGPL-3.0-only + +pragma solidity ^0.8.17;script/deploy/deployAllContracts.sh (1)
371-374: Fix broken call: updateDiamondLogForNetwork now requires 3rd arg (or use batch helper)This call omits DIAMOND_CONTRACT_NAME after the signature change, so logs won’t update. Prefer the new batch helper to update both diamonds.
Apply one of:
- updateDiamondLogForNetwork "$NETWORK" "$ENVIRONMENT" + updateDiamondLogs "$ENVIRONMENT" "$NETWORK"or, if you intend only LiFiDiamond here:
- updateDiamondLogForNetwork "$NETWORK" "$ENVIRONMENT" + updateDiamondLogForNetwork "$NETWORK" "$ENVIRONMENT" "$DIAMOND_CONTRACT_NAME"
🧹 Nitpick comments (5)
test/solidity/Facets/AllBridgeFacet.t.sol (3)
103-108: Replace magic number with named constantUse ALLBRIDGE_ID_POLYGON instead of 5 for clarity and to avoid drift.
- uint256 fees = ALLBRIDGE_ROUTER.getTransactionCost(5) + - ALLBRIDGE_ROUTER.getMessageCost( - 5, + uint256 fees = ALLBRIDGE_ROUTER.getTransactionCost(ALLBRIDGE_ID_POLYGON) + + ALLBRIDGE_ROUTER.getMessageCost( + ALLBRIDGE_ID_POLYGON, IAllBridge.MessengerProtocol.Allbridge );
219-221: Use the diamond address consistently for approvalsElsewhere you approve to _facetTestContractAddress. Align here for consistency/readability.
- usdc.approve(address(allBridgeFacet), bridgeData.minAmount); + usdc.approve(_facetTestContractAddress, bridgeData.minAmount);
49-59: Unify chain ID constants’ sourceSome LIFI_CHAIN_ID_* values are defined locally while others (e.g., SOLANA/TRON/SUI) appear sourced externally. Prefer a single shared source (e.g., the test base/constants lib) to avoid duplication and drift.
script/helperFunctions.sh (1)
4537-4550: Consider including LiFiDiamondImmutable in the batchupdateDiamondLogs currently processes only LiFiDiamond and LDA. If Immutable should be maintained too, add it to DIAMOND_CONTRACT_NAMES.
Apply:
- local DIAMOND_CONTRACT_NAMES=("LiFiDiamond" "LiFiDEXAggregatorDiamond") + local DIAMOND_CONTRACT_NAMES=("LiFiDiamond" "LiFiDiamondImmutable" "LiFiDEXAggregatorDiamond")script/tasks/diamondUpdateFacet.sh (1)
112-116: Flag naming across files is inconsistent (minor)USE_MUTABLE_DIAMOND is true for LDA here but false in updateDiamondLogForNetwork. It’s harmless now; consider harmonizing for clarity.
📜 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 (6)
deployments/_deployments_log_file.json(13 hunks)script/deploy/deployAllContracts.sh(1 hunks)script/deploy/tron/utils.ts(2 hunks)script/helperFunctions.sh(16 hunks)script/tasks/diamondUpdateFacet.sh(4 hunks)test/solidity/Facets/AllBridgeFacet.t.sol(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- script/deploy/tron/utils.ts
🧰 Additional context used
📓 Path-based instructions (5)
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
test/solidity/Facets/AllBridgeFacet.t.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Solidity test files must use .t.sol extension; organize imports with system libraries first, then project files
Test naming: successful tests prefixed test_, failure tests prefixed testRevert_, base/inherited tests prefixed testBase_; negative tests must check specific revert reasons
Each test contract must implement setUp() that configures block numbers, initializes base contracts, sets up facets, and uses vm.label; contracts inheriting TestBase.sol must call initTestBase() in setUp()
Use vm.startPrank()/vm.stopPrank() for user simulation; maintain blank-line formatting rules for tests (expectRevert separation, assertion grouping, spacing)
Use assertEq for equality, custom assertion modifiers for balance changes, vm.expectRevert with precise reasons, and vm.expectEmit(true,true,true,true,) to verify events and parameters
Files:
test/solidity/Facets/AllBridgeFacet.t.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
test/solidity/Facets/AllBridgeFacet.t.sol
script/**/*.sh
📄 CodeRabbit inference engine (conventions.md)
script/**/*.sh: Bash scripts must start with #!/bin/bash, be modular with sections (Logging, Error handling and logging, Deployment functions), follow DRY via helpers, and organize core operations into functions
Bash style: consistent indentation/naming, comments, usage instructions, and TODO/limitations documented
Bash error handling and logging: use helper logging functions, validate inputs early, check exit status with checkFailure, and use set -e where appropriate
Files:
script/deploy/deployAllContracts.shscript/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
{script/**/*.sh,preinstall.sh}
📄 CodeRabbit inference engine (conventions.md)
Environment: load from .env or config.sh, declare globals in config, update .env.example, validate env vars early; add system packages to preinstall.sh
Files:
script/deploy/deployAllContracts.shscript/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
🧠 Learnings (46)
📓 Common learnings
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/LDA/Facets/UniV3StyleFacet.sol:4-12
Timestamp: 2025-08-29T11:13:11.137Z
Learning: The correct directory structure for LDA imports in the LiFi contracts codebase is "src/Periphery/LDA/" (uppercase). Import paths should use "lifi/Periphery/LDA/..." to match the actual filesystem structure. The files exist at paths like src/Periphery/LDA/PoolCallbackAuthenticated.sol, src/Periphery/LDA/Errors/Errors.sol, and src/Periphery/LDA/BaseRouteConstants.sol.
Learnt from: mirooon
PR: lifinance/contracts#1328
File: src/Periphery/LDA/Facets/VelodromeV2Facet.sol:5-11
Timestamp: 2025-08-29T11:13:18.076Z
Learning: The correct directory structure in the LiFi contracts codebase is src/Periphery/LDA/ (uppercase "LDA"), not src/Periphery/Lda/ (mixed case). Import paths should use "lifi/Periphery/LDA/..." to match the actual filesystem structure. The directory src/Periphery/LDA/ exists and contains all the LDA facets, while src/Periphery/Lda/ does not exist.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/fuse.json:24-25
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The `LiFiDEXAggregator.sol` contract is located in `src/Periphery/`.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `deployments/*.diamond.json` and `deployments/*.json` files, the `LiFiDEXAggregator` contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}
Applied to files:
test/solidity/Facets/AllBridgeFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to test/solidity/**/*.t.sol : Each test contract must implement setUp() that configures block numbers, initializes base contracts, sets up facets, and uses vm.label; contracts inheriting TestBase.sol must call initTestBase() in setUp()
Applied to files:
test/solidity/Facets/AllBridgeFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Apply required modifiers where appropriate: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls
Applied to files:
test/solidity/Facets/AllBridgeFacet.t.sol
📚 Learning: 2025-07-16T01:03:08.106Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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:
test/solidity/Facets/AllBridgeFacet.t.sol
📚 Learning: 2025-07-11T09:43:22.393Z
Learnt from: mirooon
PR: lifinance/contracts#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/AllBridgeFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : In {facetName}Data, receiverAddress must be the first field and be validated against bridgeData.receiver; verify targetChainId equals bridgeData.destinationChain for EVM-to-EVM
Applied to files:
test/solidity/Facets/AllBridgeFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Facet contracts must be located in src/Facets and include "Facet" in the contract name
Applied to files:
test/solidity/Facets/AllBridgeFacet.t.sol
📚 Learning: 2025-01-28T14:29:00.823Z
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/utils/UpdateScriptBase.sol:112-178
Timestamp: 2025-01-28T14:29:00.823Z
Learning: The suggestion to modify `buildDiamondCut` function in `UpdateScriptBase.sol` to handle selectors from multiple old facets differently was deemed unnecessary by the maintainer.
Applied to files:
test/solidity/Facets/AllBridgeFacet.t.solscript/deploy/deployAllContracts.shscript/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2025-09-16T01:39:54.099Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1381
File: deployments/arbitrum.json:65-69
Timestamp: 2025-09-16T01:39:54.099Z
Learning: When verifying facet deployments across .json and .diamond.json files, search by facet name rather than trying to cross-reference addresses between the files, as the same contract can have different addresses in different deployment files.
Applied to files:
test/solidity/Facets/AllBridgeFacet.t.solscript/deploy/deployAllContracts.shscript/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : For non-EVM chains: receiverAddress must be bytes (not bytes32), must not be zero, and bridgeData.receiver must equal NON_EVM_ADDRESS
Applied to files:
test/solidity/Facets/AllBridgeFacet.t.sol
📚 Learning: 2025-09-09T10:39:26.383Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
Applied to files:
script/deploy/deployAllContracts.shdeployments/_deployments_log_file.jsonscript/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2025-06-05T10:00:01.583Z
Learnt from: mirooon
PR: lifinance/contracts#1145
File: script/tasks/diamondSyncWhitelistedAddresses.sh:208-209
Timestamp: 2025-06-05T10:00:01.583Z
Learning: Task scripts in script/tasks/ directory (like diamondSyncWhitelistedAddresses.sh) define functions that are sourced by deployAllContracts.sh and called from there, rather than executing directly. They don't need to call their own functions at the end of the file.
Applied to files:
script/deploy/deployAllContracts.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2024-11-26T01:18:52.125Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/deploySingleContract.sh:231-231
Timestamp: 2024-11-26T01:18:52.125Z
Learning: In the `deploySingleContract.sh` script, environment variable assignments preceding the `forge script` command (e.g., `DEPLOYSALT=$DEPLOYSALT ... forge script ...`) are acceptable and function correctly, even when variable expansions like `$DIAMOND_TYPE` and `$NETWORK` are used later in the same command.
Applied to files:
script/deploy/deployAllContracts.shscript/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
Applied to files:
script/deploy/deployAllContracts.shdeployments/_deployments_log_file.jsonscript/helperFunctions.sh
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to script/deploy/facets/Deploy*.s.sol : Deployment scripts for facets must be named Deploy{ContractName}.s.sol and inherit DeployScriptBase; deploy using deploy(type(ContractName).creationCode); omit getConstructorArgs() when not needed
Applied to files:
script/deploy/deployAllContracts.sh
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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:
script/deploy/deployAllContracts.shdeployments/_deployments_log_file.jsonscript/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2025-04-23T05:20:45.831Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1102
File: script/tasks/diamondSyncSigs_FAST.sh:14-18
Timestamp: 2025-04-23T05:20:45.831Z
Learning: In the lifinance/contracts repository, the `diamondSyncSigs_FAST` function in script/tasks/diamondSyncSigs_FAST.sh doesn't need explicit parameter validation (like for the ENVIRONMENT parameter) because it's called from a master script that ensures these parameters are available.
Applied to files:
script/deploy/deployAllContracts.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2025-04-21T03:15:12.236Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1109
File: deployments/worldchain.diamond.json:84-85
Timestamp: 2025-04-21T03:15:12.236Z
Learning: In deployment JSON files that contain "diamond" in their filename (located in the deployments folder), periphery contracts may have empty string values for their addresses. This indicates that the contract is not deployed on that particular chain and should not be flagged as an issue during code reviews.
Applied to files:
script/deploy/deployAllContracts.shdeployments/_deployments_log_file.jsonscript/helperFunctions.sh
📚 Learning: 2025-02-12T09:44:12.961Z
Learnt from: mirooon
PR: lifinance/contracts#985
File: script/deploy/_targetState.json:0-0
Timestamp: 2025-02-12T09:44:12.961Z
Learning: The bsca network intentionally maintains different facet versions between staging and production environments, specifically:
1. CalldataVerificationFacet: v1.1.1 in staging vs v1.1.2 in production
2. EmergencyPauseFacet: present only in production
3. Permit2Proxy: present only in production
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-09-22T00:52:26.172Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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:
deployments/_deployments_log_file.jsonscript/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: deployments/bsc.diamond.staging.json:4-7
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In files with "staging" in their filename, such as `deployments/bsc.diamond.staging.json`, it is acceptable for facets to have empty "Name" and "Version" fields. Do not flag missing facet information in such files during reviews.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2024-11-01T11:53:57.162Z
Learnt from: ezynda3
PR: lifinance/contracts#846
File: deployments/cronos.diamond.json:28-31
Timestamp: 2024-11-01T11:53:57.162Z
Learning: In `deployments/cronos.diamond.json`, both `GenericSwapFacet` and `GenericSwapFacetV3` are distinct facets that should be included together, as they serve different purposes.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2024-12-04T01:59:34.045Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#832
File: deployments/_deployments_log_file.json:23712-23720
Timestamp: 2024-12-04T01:59:34.045Z
Learning: In `deployments/_deployments_log_file.json`, duplicate deployment entries for the same version and address may occur because they correspond to deployments on different networks. These entries are acceptable and should not be flagged as duplicates in future reviews.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-08-07T10:20:01.383Z
Learnt from: mirooon
PR: lifinance/contracts#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.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-07-04T08:59:08.108Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: ezynda3
PR: lifinance/contracts#806
File: deployments/_deployments_log_file.json:5780-5793
Timestamp: 2024-10-09T03:47:21.269Z
Learning: The owner address `0x11f11121df7256c40339393b0fb045321022ce44` and the `DiamondCutFacet` address `0xd5cf40a2a18b633cfd6a1ae16d1771596498cf83` in the `LiFiDiamond` deployment on `xlayer` are correct and should not be flagged as issues, even if they are not referenced in the Solidity files.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `deployments/*.diamond.json` and `deployments/*.json` files, the `LiFiDEXAggregator` contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.
Applied to files:
deployments/_deployments_log_file.jsonscript/helperFunctions.sh
📚 Learning: 2024-11-26T01:03:43.597Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/scroll.diamond.json:111-111
Timestamp: 2024-11-26T01:03:43.597Z
Learning: When reviewing pull requests, only point out missing contract addresses in `deployments/<network>.diamond.json` if the contract's address is present in `deployments/<network>.json` but missing in `deployments/<network>.diamond.json`. If the contract is not deployed on a network (i.e., not present in `deployments/<network>.json`), then no action is needed.
Applied to files:
deployments/_deployments_log_file.jsonscript/helperFunctions.sh
📚 Learning: 2025-06-16T09:28:18.027Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2024-11-25T09:05:43.045Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/base.diamond.json:148-148
Timestamp: 2024-11-25T09:05:43.045Z
Learning: In deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contracts like `Permit2Proxy` may be placeholders and will be updated after approvals are released from the multisig safe.
Applied to files:
deployments/_deployments_log_file.json
📚 Learning: 2025-06-06T16:09:58.692Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: script/tasks/diamondSyncWhitelistedAddresses.sh:128-128
Timestamp: 2025-06-06T16:09:58.692Z
Learning: In script/tasks/diamondSyncWhitelistedAddresses.sh at line 128, there is an unquoted command substitution `$(getPrivateKey "$NETWORK" "$ENVIRONMENT")` that should be quoted as `"$(getPrivateKey "$NETWORK" "$ENVIRONMENT")"` to prevent word splitting issues. The user mirooon wants to be reminded about this issue in future PRs when this file is touched.
Applied to files:
script/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2024-10-08T05:10:51.995Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#715
File: script/utils/diamondEMERGENCYPauseGitHub.sh:105-105
Timestamp: 2024-10-08T05:10:51.995Z
Learning: In `script/utils/diamondEMERGENCYPauseGitHub.sh`, assigning the variable `OWNER` using `OWNER=$(cast call "$DIAMOND_ADDRESS" "owner() external returns (address)" --rpc-url "$RPC_URL")` is intentional to check if the diamond contract responds or is paused. Saving the result in a variable and executing it in a subshell allows the script to continue running without stopping, even if the variable is not used thereafter.
Applied to files:
script/helperFunctions.sh
📚 Learning: 2025-08-27T08:45:59.606Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1325
File: script/tasks/diamondSyncDEXs.sh:116-116
Timestamp: 2025-08-27T08:45:59.606Z
Learning: In script/tasks/diamondSyncDEXs.sh, user 0xDEnYO has chosen to selectively apply ShellCheck fixes, keeping array assignments using $() construct and other patterns as-is in their controlled deployment environment, prioritizing functionality over strict ShellCheck compliance.
Applied to files:
script/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2025-06-06T16:10:04.674Z
Learnt from: mirooon
PR: lifinance/contracts#1193
File: script/tasks/diamondSyncWhitelistedAddresses.sh:97-97
Timestamp: 2025-06-06T16:10:04.674Z
Learning: In script/tasks/diamondSyncWhitelistedAddresses.sh (lines 97 and 133), there are shell array assignments using command substitution that should be fixed using mapfile instead: `WHITELISTED_ADDRESSES=($(getApprovedWhitelistedAddresses))` should be `mapfile -t WHITELISTED_ADDRESSES < <(getApprovedWhitelistedAddresses)` and similarly for `WHITELISTED_ADDRESSES_UPDATED`. User mirooon requested to be reminded of this issue in future PRs when this file is modified.
Applied to files:
script/helperFunctions.sh
📚 Learning: 2024-11-26T01:01:18.499Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/arbitrum.diamond.json:148-150
Timestamp: 2024-11-26T01:01:18.499Z
Learning: In `deployments/*.diamond.json` configuration files, facets that are part of an open PR and not yet merged into the main branch may have missing `Name` and `Version` fields.
Applied to files:
script/helperFunctions.sh
📚 Learning: 2025-01-29T06:28:56.328Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#961
File: script/tasks/diamondUpdateFacet.sh:119-119
Timestamp: 2025-01-29T06:28:56.328Z
Learning: In shell scripts, environment variables can be safely assigned inline with commands (e.g., `VAR=value command`) when they are only needed for that specific command execution, as demonstrated in the `diamondUpdateFacet.sh` script.
Applied to files:
script/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2025-02-25T10:37:22.380Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1029
File: deployments/_deployments_log_file.json:25831-25847
Timestamp: 2025-02-25T10:37:22.380Z
Learning: In the LiFi contracts repository, deployment log files (like _deployments_log_file.json) are organized by contract type, then by network name, then by environment and version. The same network name (e.g., "sonic") can appear multiple times under different contract types, which is the expected structure and not an issue.
Applied to files:
script/helperFunctions.sh
📚 Learning: 2025-06-24T01:27:58.260Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1231
File: script/helperFunctions.sh:3641-3646
Timestamp: 2025-06-24T01:27:58.260Z
Learning: In the script/helperFunctions.sh codebase, the scripts have existing validation mechanisms that ensure the ENVIRONMENT parameter is always either "production" or "staging" before reaching functions like updateDiamondLogs, making additional validation within individual functions redundant.
Applied to files:
script/helperFunctions.shscript/tasks/diamondUpdateFacet.sh
📚 Learning: 2025-01-28T14:30:06.911Z
Learnt from: ezynda3
PR: lifinance/contracts#924
File: deployments/abstract.json:2-4
Timestamp: 2025-01-28T14:30:06.911Z
Learning: In the LiFi contract architecture, "LiFiDiamond" is the diamond proxy contract that connects all facets into a cohesive system. The facets are individual contracts that provide specific functionality, and the diamond proxy delegates calls to these facets.
Applied to files:
script/helperFunctions.sh
📚 Learning: 2025-06-13T08:30:26.220Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#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:
script/helperFunctions.sh
📚 Learning: 2024-10-11T22:01:09.826Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#715
File: script/utils/diamondEMERGENCYPauseGitHub.sh:146-148
Timestamp: 2024-10-11T22:01:09.826Z
Learning: In the script `diamondEMERGENCYPauseGitHub.sh`, it's acceptable that the script does not exit using the `RETURN` variable, as it executes subtasks in parallel and is intended to complete successfully even if some subtasks fail.
Applied to files:
script/helperFunctions.sh
📚 Learning: 2025-08-29T14:06:33.384Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/UpdateUniV2StyleFacet.s.sol:1-2
Timestamp: 2025-08-29T14:06:33.384Z
Learning: For LDA deployment and update scripts in script/deploy/facets/LDA/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for script/deploy/facets/LDA/UpdateUniV2StyleFacet.s.sol.
Applied to files:
script/tasks/diamondUpdateFacet.sh
📚 Learning: 2024-11-26T01:14:58.163Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: deployments/worldchain.diamond.json:5-63
Timestamp: 2024-11-26T01:14:58.163Z
Learning: When facet versions are set to empty strings in `deployments/*.diamond.json` files, do not report this issue, as it is already known and will be addressed separately.
Applied to files:
script/tasks/diamondUpdateFacet.sh
📚 Learning: 2024-11-26T01:50:51.809Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#874
File: script/tasks/diamondSyncSigs.sh:88-88
Timestamp: 2024-11-26T01:50:51.809Z
Learning: In the 'lifinance/contracts' repository, scripts like 'script/tasks/diamondSyncSigs.sh' are executed only on local machines where private keys are securely managed, so additional private key security improvements are not required.
Applied to files:
script/tasks/diamondUpdateFacet.sh
📚 Learning: 2025-02-13T03:07:05.721Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#994
File: script/config.example.sh:107-108
Timestamp: 2025-02-13T03:07:05.721Z
Learning: The DEPLOY_NEW_NETWORK_MODE flag in deployment scripts is required during initial contract deployment because ownership hasn't been transferred to SAFE yet. This mode allows direct execution of diamondCut and registerPeriphery transactions by the deployer.
Applied to files:
script/tasks/diamondUpdateFacet.sh
🧬 Code graph analysis (3)
script/deploy/deployAllContracts.sh (1)
script/tasks/diamondUpdateFacet.sh (1)
diamondUpdateFacet(3-225)
script/helperFunctions.sh (1)
script/tasks/diamondUpdateFacet.sh (1)
diamondUpdateFacet(3-225)
script/tasks/diamondUpdateFacet.sh (1)
script/helperFunctions.sh (4)
isZkEvmNetwork(3701-3725)install_foundry_zksync(4635-4782)getPrivateKey(3669-3700)saveDiamondFacets(869-1715)
🪛 Shellcheck (0.11.0)
script/helperFunctions.sh
[warning] 4414-4414: Declare and assign separately to avoid masking return values.
(SC2155)
script/tasks/diamondUpdateFacet.sh
[warning] 139-139: This assignment is only seen by the forked process.
(SC2097)
[warning] 139-139: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 142-142: This assignment is only seen by the forked process.
(SC2097)
[warning] 142-142: This expansion will not see the mentioned assignment.
(SC2098)
[warning] 180-180: Quote this to prevent word splitting.
(SC2046)
[warning] 182-182: This assignment is only seen by the forked process.
(SC2097)
[warning] 182-182: This expansion will not see the mentioned assignment.
(SC2098)
⏰ 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: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (20)
test/solidity/Facets/AllBridgeFacet.t.sol (2)
66-69: setUp override + base init look correctUsing override and calling initTestBase() aligns with the new harness pattern.
84-85: Explicit address casts for addFacet match the helper’s signatureGood switch to address(diamond) and address(allBridgeFacet).
script/helperFunctions.sh (12)
876-904: Facets intermediate-dir handling looks goodNew args and per-run temp dir fallback enable safe parallelization. No issues.
929-934: Guard against empty contract name fallbackIf getContractNameFromDeploymentLogs fails, NAME is empty and you’ll write an empty Name. Prefer a stable placeholder.
[ suggest_recommended_refactor ]
Apply:- NAME=$(getContractNameFromDeploymentLogs "$NETWORK" "$ENVIRONMENT" "$FACET_ADDRESS") - JSON_ENTRY="{\"$FACET_ADDRESS\": {\"Name\": \"$NAME\", \"Version\": \"\"}}" + NAME=$(getContractNameFromDeploymentLogs "$NETWORK" "$ENVIRONMENT" "$FACET_ADDRESS") || NAME="" + [[ -z "$NAME" ]] && NAME="Unknown" + JSON_ENTRY="{\"$FACET_ADDRESS\": {\"Name\": \"$NAME\", \"Version\": \"\"}}"
973-976: Skip periphery for LDA: correctAvoiding periphery on LiFiDEXAggregatorDiamond is intentional and safe.
1080-1124: Periphery intermediate-dir support is correctOptional PERIPHERY_DIR with mktemp fallback and scoped trap is a solid pattern.
2541-2542: Updated diamondUpdateFacet signature usage is correctPassing four params aligns with the new function signature.
4399-4421: Fix masked exit code when resolving DIAMOND_ADDRESSUsing command substitution in the declaration and then calling echoDebug before checking
$?masks the exit code; this breaks the “skipped” path and can misclassify failures. Declare, assign, capture status, then branch.Apply:
- local DIAMOND_ADDRESS=$(getContractAddressFromDeploymentLogs "$NETWORK" "$ENVIRONMENT" "$DIAMOND_CONTRACT_NAME") - - echoDebug "DIAMOND_ADDRESS=$DIAMOND_ADDRESS" - - if [[ $? -ne 0 || -z "$DIAMOND_ADDRESS" ]]; then + local DIAMOND_ADDRESS + DIAMOND_ADDRESS=$(getContractAddressFromDeploymentLogs "$NETWORK" "$ENVIRONMENT" "$DIAMOND_CONTRACT_NAME") + local _addr_rc=$? + echoDebug "DIAMOND_ADDRESS=$DIAMOND_ADDRESS" + if [[ $_addr_rc -ne 0 || -z "$DIAMOND_ADDRESS" ]]; then warning "[$NETWORK] Failed to get $DIAMOND_CONTRACT_NAME address on $NETWORK in $ENVIRONMENT environment - contract may not be deployed. Skipping." return 2 # Return a special exit code (2) for "skipped" instead of 0 (success) fiBased on learnings
4448-4463: Diamond file routing per type looks goodSeparate outputs for LiFiDiamond, LiFiDiamondImmutable, and LDA are correct.
4466-4469: Scoped temp-dir cleanup via RETURN trap is correctLimits side effects and avoids lingering dirs.
4476-4487: Parallel facet/periphery resolution is soundExporting DIAMOND_CONTRACT_NAME for inner functions and collecting PIDs is fine.
4515-4523: Conditional Periphery merge is correctLDA branch correctly omits the Periphery key.
4599-4607: Job exit-code handling is correctCapturing status immediately after wait preserves accurate outcomes; “skipped” (2) handling is clear.
4414-4414: No invocations of updateDiamondLogForNetwork found; no call sites require signature update.Likely an incorrect or invalid review comment.
script/deploy/deployAllContracts.sh (1)
187-187: Core facets update call aligns with new signaturePassing four params to diamondUpdateFacet is correct.
script/tasks/diamondUpdateFacet.sh (4)
85-89: LDA-aware script selection is correctChoosing LDA update scripts when targeting LiFiDEXAggregatorDiamond is appropriate.
92-107: Script path resolution per network/diamond is correctzkEVM/Non-zk paths and LDA directories are handled properly.
220-221: saveDiamondFacets call updated correctlyExtra placeholders align with the extended signature.
179-183: Bug: missing env exports in zkEVM staging; and quote private keyNETWORK/FILE_SUFFIX aren’t exported in the zkEVM staging run (unlike other branches), and the private key isn’t quoted.
Apply:
- RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync USE_DEF_DIAMOND=$USE_LDA_DIAMOND ./foundry-zksync/forge script "$SCRIPT_PATH" -f "$NETWORK" --json --broadcast --skip-simulation --slow --zksync --private-key $(getPrivateKey "$NETWORK" "$ENVIRONMENT")) + RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_LDA_DIAMOND ./foundry-zksync/forge script "$SCRIPT_PATH" -f "$NETWORK" --json --broadcast --skip-simulation --slow --zksync --private-key "$(getPrivateKey "$NETWORK" "$ENVIRONMENT")")And for non-zk staging:
- RAW_RETURN_DATA=$(NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_LDA_DIAMOND NO_BROADCAST=false PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") forge script "$SCRIPT_PATH" -f "$NETWORK" -vvvv --json --broadcast --legacy) + RAW_RETURN_DATA=$(NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_LDA_DIAMOND NO_BROADCAST=false PRIVATE_KEY="$(getPrivateKey "$NETWORK" "$ENVIRONMENT")" forge script "$SCRIPT_PATH" -f "$NETWORK" -vvvv --json --broadcast --legacy)deployments/_deployments_log_file.json (1)
42843-42851: Fill in the real constructor calldata for the BSC LDADiamond staging deployment.
LiFiDEXAggregatorDiamond’s constructor takes two address params (_contractOwner,_diamondCutFacet), so the calldata must be 64 bytes. Logging"CONSTRUCTOR_ARGS": "0x"makes this deployment record incorrect—please replace it with the actual ABI‑encoded constructor args used on-chain.
…romeV2Facet in zksync LDA
…nts. The script now deletes draft versions of contracts that duplicate main versions, ensuring cleaner typechain type generation and preventing "Duplicate identifier" TypeScript errors.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
script/helperFunctions.sh (1)
4468-4470: Unreachable attempts checkThe condition compares attempts to MAX+1; it never triggers given the loop bounds. Use >= MAX_ATTEMPTS_PER_SCRIPT_EXECUTION (or == MAX) to surface the warning.
- if [ $attempts -eq $((MAX_ATTEMPTS_PER_SCRIPT_EXECUTION + 1)) ]; then + if [ $attempts -ge "$MAX_ATTEMPTS_PER_SCRIPT_EXECUTION" ]; thenscript/deploy/deployAllContracts.sh (1)
371-372: Broken call: updateDiamondLogForNetwork now requires DIAMOND_CONTRACT_NAMEFunction signature is (NETWORK, ENVIRONMENT, DIAMOND_CONTRACT_NAME). This call passes only 2 args, so logs won’t update.
Apply one of:
- updateDiamondLogForNetwork "$NETWORK" "$ENVIRONMENT" + updateDiamondLogForNetwork "$NETWORK" "$ENVIRONMENT" "$DIAMOND_CONTRACT_NAME"or, to update all diamond types in one go:
- updateDiamondLogForNetwork "$NETWORK" "$ENVIRONMENT" + updateDiamondLogs "$ENVIRONMENT" "$NETWORK"
♻️ Duplicate comments (9)
test/solidity/Facets/GenericSwapFacetV3.t.sol (1)
7-7: Potential compilation issue already flagged in previous reviewThe removal of
TestHelpersfrom the inheritance chain (line 23) while still using helper methods likedeployFundAndWhitelistMockDEXthroughout the file was previously identified as a compilation concern. Verify thatTestBasenow includes these helpers or restore the inheritance.Also applies to: 23-23
script/demoScripts/utils/demoScriptHelpers.ts (1)
619-639: Fix env var name for hyphenated chains; use getRPCEnvVarName().
ETH_NODE_URI_${chain.toUpperCase()}produces invalid environment variable names for chains with hyphens (e.g.,tron-shasta→ETH_NODE_URI_TRON-SHASTAwith a hyphen). The repository standard is to convert hyphens to underscores viagetRPCEnvVarName()fromscript/utils/network.ts.Apply this diff:
export const getRpcUrl = (chain: SupportedChain): string => { - // First, try to get from environment variable - const envKey = `ETH_NODE_URI_${chain.toUpperCase()}` + // First, try to get from environment variable (normalized: hyphens -> underscores) + const envKey = getRPCEnvVarName(chain) const envRpcUrl = process.env[envKey] if (envRpcUrl) { return envRpcUrl } // Fallback to networks.json const networksConfig = networks as Record<string, { rpcUrl?: string }> const networkRpcUrl = networksConfig[chain.toLowerCase()]?.rpcUrl if (networkRpcUrl) { return networkRpcUrl } throw new Error( - `No RPC URL found for chain '${chain}' in environment variable '${envKey}' or networks.json` + `No RPC URL found for chain '${chain}'. Searched env '${envKey}' and networks.json.` ) }Also update the import at line 26:
-import { node_url } from '../../utils/network' +import { node_url, getRPCEnvVarName } from '../../utils/network'Based on learnings.
test/solidity/Facets/Across/V4/AcrossFacetV4OutputAmountCalculation.t.sol (1)
26-26: Duplicate contract name causes compilation collision.This file declares
AcrossFacetV4OutputAmountIntegrationTest, which duplicates the contract name from the Integration test file. This will cause a compilation error.Apply this diff:
-contract AcrossFacetV4OutputAmountIntegrationTest is TestBaseFacet { +contract AcrossFacetV4OutputAmountCalculationTest is TestBaseFacet {script/tasks/diamondUpdateFacet.sh (1)
179-183: Bug: missing NETWORK/FILE_SUFFIX in zkSync staging; quote private key.The zkSync staging call doesn't export
NETWORKandFILE_SUFFIX(unlike PROD and non-zk staging), which will break path resolution in scripts that rely on these variables. Additionally, the private key should be quoted to prevent word splitting.Apply this diff:
- RAW_RETURN_DATA=$(FOUNDRY_PROFILE=zksync NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_LDA_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_LDA_DIAMOND ./foundry-zksync/forge script "$SCRIPT_PATH" -f "$NETWORK" --json --broadcast --skip-simulation --slow --zksync --gas-limit 50000000 --private-key "$(getPrivateKey "$NETWORK" "$ENVIRONMENT")")Also quote the non-zk staging PRIVATE_KEY:
- RAW_RETURN_DATA=$(NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_LDA_DIAMOND NO_BROADCAST=false PRIVATE_KEY=$(getPrivateKey "$NETWORK" "$ENVIRONMENT") forge script "$SCRIPT_PATH" -f "$NETWORK" -vvvv --json --broadcast --legacy) + RAW_RETURN_DATA=$(NETWORK=$NETWORK FILE_SUFFIX=$FILE_SUFFIX USE_DEF_DIAMOND=$USE_LDA_DIAMOND NO_BROADCAST=false PRIVATE_KEY="$(getPrivateKey "$NETWORK" "$ENVIRONMENT")" forge script "$SCRIPT_PATH" -f "$NETWORK" -vvvv --json --broadcast --legacy)Note: SC2097/SC2098 warnings about inline env assignments in subshells are acceptable per project practice.
script/deploy/zksync/utils/UpdateScriptBase.sol (1)
125-126: Uninitialized memory array leads to undefined behavior.
bytes4[] memory excludeis declared but not initialized before being passed togetSelectorson line 151. This causes undefined behavior.Apply this diff:
- bytes4[] memory exclude; + bytes4[] memory exclude = new bytes4[](0);script/helperFunctions.sh (1)
4426-4449: SC2155: separate declaration from command substitution to keep exit codeDeclare DIAMOND_ADDRESS first, then assign, so you can reliably check failure and avoid masking the return status.
Apply:
- local DIAMOND_ADDRESS=$(getContractAddressFromDeploymentLogs "$NETWORK" "$ENVIRONMENT" "$DIAMOND_CONTRACT_NAME") + local DIAMOND_ADDRESS + DIAMOND_ADDRESS=$(getContractAddressFromDeploymentLogs "$NETWORK" "$ENVIRONMENT" "$DIAMOND_CONTRACT_NAME")Based on static analysis hints.
deployments/arbitrum.diamond.staging.json (1)
148-151: Remove older GlacisFacet entry to avoid duplicatesBoth 0xF82830… (v1.0.0) and 0x33EcEb… (v1.1.0) are present under LiFiDiamond.Facets. Keep the latest only to prevent tooling confusion.
"0xF82830B952Bc60b93206FA22f1cD4770cedb2840": { "Name": "GlacisFacet", "Version": "1.0.0" },Helper:
#!/bin/bash jq -r '.LiFiDiamond.Facets | to_entries[] | select(.value.Name=="GlacisFacet") | "\(.value.Version) \(.key)"' deployments/arbitrum.diamond.staging.jsonAlso applies to: 176-179
script/deploy/deploySingleContract.sh (1)
78-88: isLDAContract() function incomplete—missing zkSync LDA detection.The function checks for LDA contracts only in the non-zkEVM path (
script/deploy/facets/LDA/) but omits the zkEVM path (script/deploy/zksync/LDA/). This causes LDA facets on zkEVM networks to be misrouted toscript/deploy/zksync/instead ofscript/deploy/zksync/LDA/, resulting in deployment failures. This issue was flagged in a previous review.Apply this diff to fix the function:
isLDAContract() { local contract_name="$1" - # Check if contract name is LDA diamond or is in LDA facet + # Check if contract name is LDA diamond or has an LDA deploy script (facets or zksync) if [[ "$contract_name" == "LiFiDEXAggregatorDiamond" ]] || - [[ -f "script/deploy/facets/LDA/Deploy${contract_name}.s.sol" ]]; then + [[ -f "script/deploy/facets/LDA/Deploy${contract_name}.s.sol" ]] || + [[ -f "script/deploy/zksync/LDA/Deploy${contract_name}.zksync.s.sol" ]]; then return 0 # true else return 1 # false fi }deployments/_deployments_log_file.json (1)
46573-46586: Fix the missing constructor args for BSC LiFiDEXAggregatorDiamond entry.The
LiFiDEXAggregatorDiamond(LDADiamond) constructor requires twoaddressparameters (_contractOwnerand_diamondCutFacet), so the deployment transaction must encode 64 bytes of constructor data. Logging"CONSTRUCTOR_ARGS": "0x"for the BSC staging deployment is invalid—the deployment would have reverted with empty calldata for required constructor parameters. Please update this entry with the actual constructor calldata so the deployment record remains trustworthy. Compare with the correctly-encoded constructor args in the Arbitrum (line 46549) and Optimism (line 46564) entries.Apply this diff to fix the constructor args:
"staging": { "1.0.0": [ { "ADDRESS": "0x9257FA485C5714975247ca5C47A2cB0c4FA5A773", "OPTIMIZER_RUNS": "1000000", "TIMESTAMP": "2025-09-06 19:18:35", - "CONSTRUCTOR_ARGS": "0x", + "CONSTRUCTOR_ARGS": "0x<insert 64-byte encoded constructor args>", "SALT": "22345121", "VERIFIED": "true", "ZK_SOLC_VERSION": "" } ] }
🧹 Nitpick comments (6)
script/removeDuplicateEventsFromABI.ts (1)
54-73: Consider async file operations for consistency.The duplicate contract ABI removal logic is correct and error handling is appropriate. However, for consistency with the rest of the function which uses async operations (
readFile,writeFilefromfs/promises), consider usingunlinkfromfs/promisesinstead of the synchronousunlinkSync.Apply this diff for consistency:
-import { existsSync, unlinkSync } from 'fs' +import { existsSync } from 'fs' -import { readFile, writeFile } from 'fs/promises' +import { readFile, writeFile, unlink } from 'fs/promises'Then update the removal logic:
for (const file of duplicateContractFiles) { if (existsSync(file)) { try { - unlinkSync(file) + await unlink(file) console.log(`Removed duplicate contract ABI: ${file}`) } catch (error) {conventions.md (1)
397-398: Add language identifiers to fenced code blocks and ensure consistent markdown style.Multiple fenced code blocks lack language specifiers (MD040) and may not align with the project's markdown linting rules (MD046). Specifically:
- Lines 397–410 (Location Structure): No language spec; should be
bashor similar- Lines 474–483 (Test File Structure): No language spec; should be
bash- Lines 493–500 (setupForkConfig example): Missing language spec; should be
solidity- Lines 552–566 (Deployment Script Structure): Missing language spec; should be
solidityApply these diffs to fix:
#### Location Structure - ``` + ```bash src/Periphery/LDA/#### Test File Structure - ``` + ```bash test/solidity/Periphery/LDA/function _setupForkConfig() internal override { - ```solidity + ```solidity#### Deployment Script Structure - ```solidity + ```solidity // SPDX-License-Identifier: LGPL-3.0-onlyAlso applies to: 474-483, 493-500, 552-566
script/helperFunctions.sh (3)
895-904: Avoid global EXIT traps from library functions when a dir is providedThe EXIT trap is only set when FACETS_DIR is omitted, but global EXIT traps can collide with other traps. Prefer caller-managed lifecycle (always pass FACETS_DIR) or chain traps safely if you ever need the fallback.
1080-1124: Periphery: parametrize and isolate temp outputPERIPHERY_DIR handling mirrors facets; concurrency throttle is acceptable. Consider the same trap note as above to avoid global EXIT traps.
4505-4514: Reliance on exported env var for DIAMOND_CONTRACT_NAMEExporting DIAMOND_CONTRACT_NAME for downstream helpers works but couples functions via globals. Passing it explicitly to saveDiamondFacets would be cleaner. Optional.
test/solidity/Facets/Across/V4/AcrossFacetV4.t.sol (1)
388-393: Assert style nit (optional)Prefer asserting addresses directly instead of bool==true for readability.
assertEq(address(acrossFacetV4.SPOKEPOOL()), SPOKE_POOL); assertEq( acrossFacetV4.WRAPPED_NATIVE(), _convertAddressToBytes32(ADDRESS_WRAPPED_NATIVE) );
…n up unused entries in optimism.diamond.staging.json
Test Coverage ReportLine Coverage: 89.69% (2881 / 3212 lines) |
Which Jira task belongs to this PR?
LF-13997
Why did I implement it this way?
Documentation: https://www.notion.so/lifi/New-LDA-DEX-Checklist-25af0ff14ac78090a6a4db30b7831d46
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)