added fork tests to verify storage post WhitelistManager migration (mainnet, arbitrum, base) #1448
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
…ainnet, arbitrum, base)
Contributor
WalkthroughAdds two CLI scripts to compute and verify whitelist values and adds three fork-based Solidity migration tests (Arbitrum, Base, Mainnet) that perform a diamondCut and validate on-chain whitelist data against local configs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Collaborator
Test Coverage ReportLine Coverage: 86.89% (2797 / 3219 lines) |
![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: 4
📜 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)
script/tasks/checkWhitelistValues.ts(1 hunks)script/tasks/verifyWhitelistMigration.ts(1 hunks)test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.sol(1 hunks)test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol(1 hunks)test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
script/**/*.ts
📄 CodeRabbit inference engine (conventions.md)
script/**/*.ts: All TypeScript scripts must follow .eslintrc.cjs, use async/await, try/catch error handling, proper logging, environment variables, correct typings, use citty for CLI parsing, consola for logging, validate env vars via getEnvVar(), and exit with appropriate codes
Prefer existing helper functions over reimplementation (e.g., getDeployments, getProvider, getWalletFromPrivateKeyInDotEnv, sendTransaction, ensureBalanceAndAllowanceToDiamond, getUniswapData*)
Always use proper TypeChain types (e.g., ILiFi.BridgeDataStruct) and never use any for bridge data or contract structures
Before/after changes: verify imports and types exist, ensure typechain is generated, keep changes comprehensive and consistent, run TS compilation, remove unused imports, and ensure function signatures match usage
Scripts must execute with bunx tsx
Files:
script/tasks/verifyWhitelistMigration.tsscript/tasks/checkWhitelistValues.ts
{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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
🧠 Learnings (39)
📓 Common learnings
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: 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.
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.
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: 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: 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: mirooon
Repo: lifinance/contracts PR: 1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:153-181
Timestamp: 2025-07-11T09:48:47.211Z
Learning: In fork-based tests like AllowListMigratorFacet.t.sol, dynamically counting JSON array elements can cause EvmError: MemoryOOG due to memory constraints in the fork environment. Using hardcoded counts is acceptable when the array only grows (selectors are added but not removed), as the risk of mismatch is minimal.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/avalanche.diamond.json:105-105
Timestamp: 2024-10-04T09:17:19.275Z
Learning: In the `lifinance/contracts` repository, contracts may have different addresses across networks. Do not check if contracts have the same addresses across all networks in future reviews.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/avalanche.diamond.json:105-105
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `lifinance/contracts` repository, contracts may have different addresses across networks. Do not check if contracts have the same addresses across all networks in future reviews.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/polygon.json:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: Ensure that scripts used for codebase verification produce valid and accurate results before reporting issues, especially when checking Ethereum address checksums in `deployments/polygon.json`.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/polygon.json:0-0
Timestamp: 2024-10-04T09:21:59.708Z
Learning: Ensure that scripts used for codebase verification produce valid and accurate results before reporting issues, especially when checking Ethereum address checksums in `deployments/polygon.json`.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1377
File: deployments/mainnet.json:7-7
Timestamp: 2025-09-12T11:48:46.333Z
Learning: When running verification scripts for contract deployments, ensure the scripts are syntactically correct and thoroughly tested before making claims about contract validity. False negatives from faulty scripts can incorrectly flag valid deployments as problematic.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: test/solidity/utils/TestWhitelistManagerBase.sol:11-19
Timestamp: 2025-06-15T13:22:56.249Z
Learning: For test utility contracts like TestWhitelistManagerBase, it's acceptable and preferred to have relaxed access controls (external functions without modifiers) to make testing more flexible and convenient, rather than mirroring production access control patterns.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1377
File: deployments/_deployments_log_file.json:5607-5619
Timestamp: 2025-09-15T12:33:51.069Z
Learning: When running verification scripts for contract deployments, ensure the scripts are syntactically correct and properly target the specific data structures being validated (e.g., deployment record arrays vs. all JSON objects) before making claims about missing fields or validation failures.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1169
File: script/deploy/zksync/DeployFeeCollector.zksync.s.sol:32-37
Timestamp: 2025-05-27T12:00:43.940Z
Learning: The lifinance/contracts repository has deployment scripts that perform validation checks (including zero-address validation) before executing individual deploy scripts, making runtime validation checks in the deploy scripts themselves redundant.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1196
File: script/helperFunctions.sh:1447-1462
Timestamp: 2025-06-19T06:23:47.848Z
Learning: 0xDEnYO prefers to keep eval usage in local bash scripts when the security risk is acceptable in their controlled environment, prioritizing simplicity over security hardening for local tooling.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1212
File: .github/workflows/convertForkedPRsToInternal.yml:81-106
Timestamp: 2025-07-16T06:18:02.682Z
Learning: 0xDEnYO prefers to use printf "%q" for shell escaping in GitHub workflows to increase security and protection from potential injections, even when it might cause formatting issues, prioritizing security over convenience.
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to script/**/*.ts : Before/after changes: verify imports and types exist, ensure typechain is generated, keep changes comprehensive and consistent, run TS compilation, remove unused imports, and ensure function signatures match usage
Applied to files:
script/tasks/verifyWhitelistMigration.tsscript/tasks/checkWhitelistValues.ts
📚 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 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/tasks/verifyWhitelistMigration.tstest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2024-11-21T08:24:05.881Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 861
File: script/deploy/safe/add-owners-to-safe.ts:8-13
Timestamp: 2024-11-21T08:24:05.881Z
Learning: In `script/deploy/safe/add-owners-to-safe.ts`, validation for network configuration when casting imported JSON data to `NetworksObject` is not required as per the user's preference.
Applied to files:
script/tasks/verifyWhitelistMigration.tsscript/tasks/checkWhitelistValues.ts
📚 Learning: 2025-07-04T08:59:08.108Z
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.
Applied to files:
script/tasks/verifyWhitelistMigration.tsscript/tasks/checkWhitelistValues.ts
📚 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:
script/tasks/verifyWhitelistMigration.ts
📚 Learning: 2024-11-21T08:23:50.099Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 861
File: script/deploy/safe/add-owners-to-safe.ts:48-48
Timestamp: 2024-11-21T08:23:50.099Z
Learning: In the script `script/deploy/safe/add-owners-to-safe.ts`, additional defensive checks for network configuration may be unnecessary because the script will fail anyway when the network configuration is missing.
Applied to files:
script/tasks/verifyWhitelistMigration.tsscript/tasks/checkWhitelistValues.ts
📚 Learning: 2025-08-07T10:20:01.383Z
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.
Applied to files:
script/tasks/verifyWhitelistMigration.ts
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to script/**/*.ts : Prefer existing helper functions over reimplementation (e.g., getDeployments, getProvider, getWalletFromPrivateKeyInDotEnv, sendTransaction, ensureBalanceAndAllowanceToDiamond, getUniswapData*)
Applied to files:
script/tasks/verifyWhitelistMigration.tsscript/tasks/checkWhitelistValues.ts
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/polygon.json:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: Ensure that scripts used for codebase verification produce valid and accurate results before reporting issues, especially when checking Ethereum address checksums in `deployments/polygon.json`.
Applied to files:
script/tasks/verifyWhitelistMigration.tsscript/tasks/checkWhitelistValues.ts
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
script/tasks/verifyWhitelistMigration.tstest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2025-04-21T03:15:12.236Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/tasks/verifyWhitelistMigration.ts
📚 Learning: 2024-11-26T01:03:43.597Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
script/tasks/verifyWhitelistMigration.ts
📚 Learning: 2024-11-25T09:05:43.045Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
script/tasks/verifyWhitelistMigration.ts
📚 Learning: 2024-11-21T08:39:29.530Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 861
File: config/dexs.json:748-752
Timestamp: 2024-11-21T08:39:29.530Z
Learning: In the 'worldchain' network, the addresses `0x50D5a8aCFAe13Dceb217E9a071F6c6Bd5bDB4155`, `0x8f023b4193a6b18C227B4a755f8e28B3D30Ef9a1`, and `0x603a538477d44064eA5A5d8C345b4Ff6fca1142a` are used as DEXs and should be included in `config/dexs.json`.
Applied to files:
script/tasks/verifyWhitelistMigration.ts
📚 Learning: 2024-10-21T01:27:47.808Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 836
File: config/dexs.json:396-397
Timestamp: 2024-10-21T01:27:47.808Z
Learning: In `config/dexs.json`, it's acceptable for a whitelisted DEX address to be the same across multiple networks.
Applied to files:
script/tasks/verifyWhitelistMigration.ts
📚 Learning: 2024-11-04T02:25:07.478Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 846
File: config/dexs.json:296-300
Timestamp: 2024-11-04T02:25:07.478Z
Learning: In `config/dexs.json`, it's expected that some addresses appear multiple times across different networks.
Applied to files:
script/tasks/verifyWhitelistMigration.ts
📚 Learning: 2025-08-28T02:02:56.965Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1136
File: config/dexs.json:0-0
Timestamp: 2025-08-28T02:02:56.965Z
Learning: Different networks in config/dexs.json naturally and appropriately have different DEX addresses. Do not flag different addresses across different networks as issues - only flag if the same network's addresses appear to have changed unintentionally between branches.
Applied to files:
script/tasks/verifyWhitelistMigration.ts
📚 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2025-08-01T22:11:04.478Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: deployments/base.staging.json:6-6
Timestamp: 2025-08-01T22:11:04.478Z
Learning: The user mirooon prefers to maintain both DexManagerFacet and WhitelistManagerFacet entries in staging deployment configurations during migration periods, even when the DexManagerFacet contract no longer exists in the codebase. This approach allows for backward compatibility testing and rollback capabilities in the staging environment.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2025-06-15T13:22:56.249Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: test/solidity/utils/TestWhitelistManagerBase.sol:11-19
Timestamp: 2025-06-15T13:22:56.249Z
Learning: For test utility contracts like TestWhitelistManagerBase, it's acceptable and preferred to have relaxed access controls (external functions without modifiers) to make testing more flexible and convenient, rather than mirroring production access control patterns.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
Repo: lifinance/contracts PR: 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2025-07-11T09:48:47.211Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:153-181
Timestamp: 2025-07-11T09:48:47.211Z
Learning: In fork-based tests like AllowListMigratorFacet.t.sol, dynamically counting JSON array elements can cause EvmError: MemoryOOG due to memory constraints in the fork environment. Using hardcoded counts is acceptable when the array only grows (selectors are added but not removed), as the risk of mismatch is minimal.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
Repo: lifinance/contracts PR: 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2024-11-26T01:16:27.721Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: deployments/worldchain.diamond.json:40-43
Timestamp: 2024-11-26T01:16:27.721Z
Learning: Version inconsistencies of 'GenericSwapFacetV3' across deployment configurations are acceptable if they are only due to differences in Solidity pragma directives.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
Repo: lifinance/contracts PR: 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2024-10-10T03:18:20.721Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.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:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2025-09-16T01:39:54.099Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to test/solidity/**/*.t.sol : Use assertEq for equality, custom assertion modifiers for balance changes, vm.expectRevert with precise reasons, and vm.expectEmit(true,true,true,true,<addr>) to verify events and parameters
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2024-11-21T08:22:38.484Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 861
File: script/deploy/safe/add-owners-to-safe.ts:8-13
Timestamp: 2024-11-21T08:22:38.484Z
Learning: In `script/deploy/safe/add-owners-to-safe.ts`, runtime validation for network configuration using Zod is not required; type assertions are acceptable.
Applied to files:
script/tasks/checkWhitelistValues.ts
📚 Learning: 2024-11-21T08:24:22.802Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 861
File: script/deploy/healthCheck.ts:387-388
Timestamp: 2024-11-21T08:24:22.802Z
Learning: In `script/deploy/healthCheck.ts`, handling the case when `networkConfig` is undefined is unnecessary because the code will fail anyway.
Applied to files:
script/tasks/checkWhitelistValues.ts
📚 Learning: 2024-11-21T08:23:39.984Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 861
File: script/deploy/safe/add-owners-to-safe.ts:48-48
Timestamp: 2024-11-21T08:23:39.984Z
Learning: In the `script/deploy/safe/add-owners-to-safe.ts` script, explicit error handling for missing network configuration is not required because the script will fail anyway if the network is not configured.
Applied to files:
script/tasks/checkWhitelistValues.ts
📚 Learning: 2025-01-28T14:29:00.823Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
🧬 Code graph analysis (1)
script/tasks/verifyWhitelistMigration.ts (2)
script/utils/viemScriptHelpers.ts (3)
castEnv(266-270)getContractAddressForNetwork(130-149)getViemChainForNetworkName(35-70)script/common/types.ts (1)
SupportedChain(3-3)
⏰ 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
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.sol
Show resolved
Hide resolved
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
Show resolved
Hide resolved
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.sol (1)
247-274: Optional enhancement: Mismatch reporter could provide more diagnostic detail.The underflow issue from the previous review has been addressed with the guard at line 260-262. However, the current implementation emits
diff = 0when addresses are missing (actual < expected), which is less informative than the originally suggested approach of emitting separate "missing addresses" diagnostics withexpectedCount - actualAddresses.length.Consider enhancing the reporter to distinguish between "extra" and "missing" cases:
- uint256 diff = actualAddresses.length >= expectedCount - ? actualAddresses.length - expectedCount - : 0; - emit log_named_uint("Difference (extra addresses on-chain)", diff); + if (actualAddresses.length >= expectedCount) { + emit log_named_uint( + "Difference (extra addresses on-chain)", + actualAddresses.length - expectedCount + ); + } else { + emit log_named_uint( + "Difference (missing addresses on-chain)", + expectedCount - actualAddresses.length + ); + }The same pattern would apply to
_reportSelectorCountMismatchand_reportPairsCountMismatch.test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol (1)
247-274: Optional enhancement: Provide clearer diagnostics for missing entries.The guard at line 260-262 prevents underflow, addressing the issue from the prior review. However, when
actualAddresses.length < expectedCount, emittingdiff = 0is less informative than separately reporting missing entries.Consider this enhancement:
- uint256 diff = actualAddresses.length > expectedCount - ? actualAddresses.length - expectedCount - : 0; - emit log_named_uint("Difference (extra addresses on-chain)", diff); + if (actualAddresses.length > expectedCount) { + emit log_named_uint( + "Difference (extra addresses on-chain)", + actualAddresses.length - expectedCount + ); + } else if (actualAddresses.length < expectedCount) { + emit log_named_uint( + "Difference (missing addresses on-chain)", + expectedCount - actualAddresses.length + ); + }Apply the same to
_reportSelectorCountMismatchand_reportPairsCountMismatchfor consistency.test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol (1)
245-272: Optional enhancement: Improve diagnostic output for missing entries.The ternary guard at lines 258-260 successfully prevents underflow. To make test failures more actionable when entries are missing, consider explicitly reporting the missing count:
- uint256 diff = actualAddresses.length >= expectedCount - ? actualAddresses.length - expectedCount - : 0; - emit log_named_uint("Difference (extra addresses on-chain)", diff); + if (actualAddresses.length >= expectedCount) { + emit log_named_uint( + "Difference (extra addresses on-chain)", + actualAddresses.length - expectedCount + ); + } else { + emit log_named_uint( + "Difference (missing addresses on-chain)", + expectedCount - actualAddresses.length + ); + }The same pattern would benefit
_reportSelectorCountMismatchand_reportPairsCountMismatch.
📜 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 (3)
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.sol(1 hunks)test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol(1 hunks)test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
🧠 Learnings (29)
📓 Common learnings
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: 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: 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: 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.
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: 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.
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: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/avalanche.diamond.json:105-105
Timestamp: 2024-10-04T09:17:19.275Z
Learning: In the `lifinance/contracts` repository, contracts may have different addresses across networks. Do not check if contracts have the same addresses across all networks in future reviews.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/avalanche.diamond.json:105-105
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `lifinance/contracts` repository, contracts may have different addresses across networks. Do not check if contracts have the same addresses across all networks in future reviews.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/polygon.json:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: Ensure that scripts used for codebase verification produce valid and accurate results before reporting issues, especially when checking Ethereum address checksums in `deployments/polygon.json`.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/polygon.json:0-0
Timestamp: 2024-10-04T09:21:59.708Z
Learning: Ensure that scripts used for codebase verification produce valid and accurate results before reporting issues, especially when checking Ethereum address checksums in `deployments/polygon.json`.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1196
File: script/helperFunctions.sh:1447-1462
Timestamp: 2025-06-19T06:23:47.848Z
Learning: 0xDEnYO prefers to keep eval usage in local bash scripts when the security risk is acceptable in their controlled environment, prioritizing simplicity over security hardening for local tooling.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1212
File: .github/workflows/convertForkedPRsToInternal.yml:81-106
Timestamp: 2025-07-16T06:18:02.682Z
Learning: 0xDEnYO prefers to use printf "%q" for shell escaping in GitHub workflows to increase security and protection from potential injections, even when it might cause formatting issues, prioritizing security over convenience.
📚 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-06-15T13:22:56.249Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: test/solidity/utils/TestWhitelistManagerBase.sol:11-19
Timestamp: 2025-06-15T13:22:56.249Z
Learning: For test utility contracts like TestWhitelistManagerBase, it's acceptable and preferred to have relaxed access controls (external functions without modifiers) to make testing more flexible and convenient, rather than mirroring production access control patterns.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-08-01T22:11:04.478Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: deployments/base.staging.json:6-6
Timestamp: 2025-08-01T22:11:04.478Z
Learning: The user mirooon prefers to maintain both DexManagerFacet and WhitelistManagerFacet entries in staging deployment configurations during migration periods, even when the DexManagerFacet contract no longer exists in the codebase. This approach allows for backward compatibility testing and rollback capabilities in the staging environment.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
Repo: lifinance/contracts PR: 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
Repo: lifinance/contracts PR: 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-07-11T09:48:47.211Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:153-181
Timestamp: 2025-07-11T09:48:47.211Z
Learning: In fork-based tests like AllowListMigratorFacet.t.sol, dynamically counting JSON array elements can cause EvmError: MemoryOOG due to memory constraints in the fork environment. Using hardcoded counts is acceptable when the array only grows (selectors are added but not removed), as the risk of mismatch is minimal.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2024-11-26T01:16:27.721Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: deployments/worldchain.diamond.json:40-43
Timestamp: 2024-11-26T01:16:27.721Z
Learning: Version inconsistencies of 'GenericSwapFacetV3' across deployment configurations are acceptable if they are only due to differences in Solidity pragma directives.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
Repo: lifinance/contracts PR: 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2024-10-10T03:18:20.721Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2024-12-02T06:33:33.309Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-12-02T06:33:33.309Z
Learning: In Solidity version 0.8.0 and above, arithmetic underflows and overflows automatically cause a revert; therefore, explicit checks for arithmetic underflows are not necessary in functions like `_startBridge` in `DeBridgeDlnFacet.sol`.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.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:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2024-10-04T09:21:59.708Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/polygon.json:0-0
Timestamp: 2024-10-04T09:21:59.708Z
Learning: Ensure that scripts used for codebase verification produce valid and accurate results before reporting issues, especially when checking Ethereum address checksums in `deployments/polygon.json`.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.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 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:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2024-10-10T03:26:23.793Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:79-80
Timestamp: 2024-10-10T03:26:23.793Z
Learning: In Solidity tests, it's acceptable to use `UnAuthorized.selector` in `vm.expectRevert()`; avoid suggesting changing it to string messages.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.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:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-09-16T01:39:54.099Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to test/solidity/**/*.t.sol : Use assertEq for equality, custom assertion modifiers for balance changes, vm.expectRevert with precise reasons, and vm.expectEmit(true,true,true,true,<addr>) to verify events and parameters
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-02-17T07:59:54.979Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 975
File: test/solidity/Facets/DeBridgeDlnFacet.t.sol:0-0
Timestamp: 2025-02-17T07:59:54.979Z
Learning: In DeBridgeDlnFacet, chain ID mappings are hardcoded and not designed to be dynamically updated, so tests for mapping updates are not needed.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2024-10-04T09:17:19.275Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/avalanche.diamond.json:105-105
Timestamp: 2024-10-04T09:17:19.275Z
Learning: In the `lifinance/contracts` repository, contracts may have different addresses across networks. Do not check if contracts have the same addresses across all networks in future reviews.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-01-28T14:29:00.823Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 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/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
📚 Learning: 2024-10-09T03:47:21.269Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 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:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.soltest/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol
📚 Learning: 2025-06-19T10:42:55.379Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1198
File: test/solidity/Helpers/SwapperV2.t.sol:50-69
Timestamp: 2025-06-19T10:42:55.379Z
Learning: Test code in Solidity (e.g., contracts in test/ directories) typically doesn't require the same level of input validation as production code, as tests are designed to validate specific scenarios with controlled inputs and may need to test edge cases.
Applied to files:
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol
🔇 Additional comments (3)
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.arbitrum.t.sol (1)
118-156: LGTM: Fork test setup is well-structured.The setUp implementation correctly:
- Forks from the appropriate RPC endpoint
- Initializes facet interfaces at the production diamond address
- Verifies TimeLock ownership before proceeding
- Executes the diamondCut via privileged TimeLock account
- Caches getter results once for reuse across tests
This follows the established pattern for fork-based migration tests.
test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.mainnet.t.sol (1)
158-163: LGTM: Migration status verification is straightforward.The test correctly validates that
isMigrated()returns true after the diamondCut, confirming the migration completed successfully.test/solidity/Facets/WhitelistManagerFacet/WhitelistManagerFacet.migration.fork.base.t.sol (1)
336-401: LGTM: Comprehensive contract-selector pair validation.The test thoroughly validates each pair across both DEX and PERIPHERY sections by checking:
- Presence in approvedDexs()
- Presence in getWhitelistedAddresses()
- Presence in getAllContractSelectorPairs()
- isContractSelectorWhitelisted() returns true
- isAddressWhitelisted() returns true
- isFunctionSelectorWhitelisted() returns true
- getWhitelistedSelectorsForContract() contains the selector
This seven-point validation ensures comprehensive post-migration state verification.
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!!!)