LF-13736 - Reapply DexManagerFacet -> WhitelistManagerFacet + getter functions and selectors array [WhitelistManagerFacet v1.0.0,IWhitelistManagerFacet v1.0.0,LibAllowList v2.0.0] #1193
Conversation
…tracts into reapply-dex-manager-update
WalkthroughThis change replaces the DexManagerFacet with a new WhitelistManagerFacet throughout the codebase. All references, configuration files, deployment scripts, and tests are updated to use whitelisted addresses and selectors instead of approved DEXs and signatures. New contracts, interfaces, scripts, and documentation for whitelist management are introduced, and legacy DexManagerFacet code and tests are removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lifi-action-bot
changed the title
Test Coverage ReportLine Coverage: 85.25% (2411 / 2828 lines) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (15)
script/deploy/resources/contractSpecificReminders.sh (1)
8-11: Ensure reminder variables are exported or consumed by deployment scripts
ShellCheck flags these assignments as unused (SC2034). Confirm that this file is sourced or that downstream scripts reference these variables. If they need to be available to child processes, consider exporting them. For example:+# Export contract reminders for downstream usage +export FeeCollector GasZipPeriphery LiFiDEXAggregator TokenWrapper🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 8-8: FeeCollector appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 9-9: GasZipPeriphery appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 10-10: LiFiDEXAggregator appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 11-11: TokenWrapper appears unused. Verify use (or export if used externally).
(SC2034)
docs/README.md (1)
14-14: Verify documentation references and link consistency
The new "Whitelist Manager Facet" entry correctly uses a./relative path. Please also:
- Remove any lingering references to the old
DexManagerFacetin this file and across docs.- Normalize link styles—for example, update the DeBridge DLN entry to use a relative path:
- [DeBridge DLN Facet](/docs/DeBridgeDlnFacet.md) + [DeBridge DLN Facet](./DeBridgeDlnFacet.md)docs/WhitelistManagerFacet.md (1)
11-11: Fix grammar issue in the caution section.There's a missing comma in the sentence. Also consider expanding the security recommendations.
Apply this fix:
-The Whitelist Manager manages which contracts and functions can be executed through the LI.FI main contract. This can be updated by a single admin key which if compromised could lead to malicious code being added to the allow list. +The Whitelist Manager manages which contracts and functions can be executed through the LI.FI main contract. This can be updated by a single admin key which, if compromised, could lead to malicious code being added to the allow list.Consider also adding recommendations for admin key security best practices (e.g., multi-sig, hardware wallets, regular rotation).
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Possible missing comma found.
Context: .... This can be updated by a single admin key which if compromised could lead to mali...(AI_HYDRA_LEO_MISSING_COMMA)
script/helperFunctions.sh (4)
1343-1344: Inconsistent informational message.The echo refers to
config/.global.json.autoWhitelistPeripheryContractsbut the script actually reads fromconfig/global.jsonand the propertyautoWhitelistPeripheryContracts. Update the message to reflect the real file path and JSON key.
1364-1364: Avoid masking return code.Shellcheck SC2155 warns that
local EXISTING_WHITELISTED_ADDRESSES=$(...)masks the command’s exit status. Split into two lines—declare the variable, then assign—to preserve the exit code.
1379-1379: Avoid masking return code.Shellcheck SC2155 also applies to
local EXISTS=$(...). Separate declaration and assignment to avoid swallowing failures.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 1379-1379: Declare and assign separately to avoid masking return values.
(SC2155)
1389-1391: Use secure temporary file.Creating
tmp.$$.jsoncan cause race conditions. Usemktemp, e.g.:local TMP_FILE TMP_FILE="$(mktemp)" || exit 1then clean it up.
test/solidity/Periphery/GasZipPeriphery.t.sol (1)
23-29: LGTM! Consistent function renaming and updates.All function renames and references have been systematically updated following the same pattern as other files in the refactor. The changes maintain the same functionality while improving terminology clarity.
Minor suggestion: Consider using consistent parameter naming - perhaps
_contractAddressfor both add and remove functions instead of_addressfor remove, though this is a minor nitpick.Also applies to: 269-269, 273-273, 361-361, 469-469, 479-481
script/deploy/healthCheck.ts (1)
623-634: Consider using a more specific type for the parameter.While the function implementation is correct, using
anyfor thewhitelistManagerparameter reduces type safety. If possible, consider defining a proper interface or type for better maintainability.-const getApprovedFunctionSignatures = async ( - whitelistManager: any -): Promise<Hex[]> => { +const getApprovedFunctionSignatures = async ( + whitelistManager: { read: { getApprovedFunctionSignatures: () => Promise<Hex[]> } } +): Promise<Hex[]> => {src/Interfaces/IWhitelistManagerFacet.sol (1)
60-86: Consider adding count functions for gas-efficient queries.For better gas efficiency when only counts are needed, consider adding:
getWhitelistedAddressCount()getApprovedFunctionSelectorCount()This would allow contracts to check sizes without retrieving full arrays.
test/solidity/Facets/WhitelistManagerFacet.t.sol (2)
38-66: Consider extracting function selector setup for better readability.While the current setup is correct, you could improve readability by creating a helper function that returns all WhitelistManagerFacet selectors.
function getWhitelistManagerSelectors() internal pure returns (bytes4[] memory) { bytes4[] memory selectors = new bytes4[](10); selectors[0] = WhitelistManagerFacet.addToWhitelist.selector; selectors[1] = WhitelistManagerFacet.removeFromWhitelist.selector; // ... rest of selectors return selectors; }
1-785: Exemplary test suite with comprehensive coverage.This test suite thoroughly validates all aspects of the WhitelistManagerFacet with excellent coverage of:
- Access control
- State management
- Edge cases
- Event emissions
- Index mapping integrity
Consider adding gas usage tests for large datasets (e.g., 1000+ addresses) to validate the gas warnings in the interface.
src/Facets/WhitelistManagerFacet.sol (3)
19-21: Extract repeated access control logic into a modifier.The access control pattern is repeated 6 times throughout the contract. Consider extracting it into a modifier to improve maintainability and reduce bytecode size.
Add this modifier after line 14:
modifier onlyAuthorized() { if (msg.sender != LibDiamond.contractOwner()) { LibAccess.enforceAccessControl(); } _; }Then simplify each function by replacing the access control blocks with the modifier:
-function addToWhitelist(address _contractAddress) external { - if (msg.sender != LibDiamond.contractOwner()) { - LibAccess.enforceAccessControl(); - } +function addToWhitelist(address _contractAddress) external onlyAuthorized { _addToWhitelist(_contractAddress); }Also applies to: 27-29, 39-41, 47-49, 63-65, 74-76
30-31: Add validation for empty arrays in batch operations.Consider adding validation to prevent unnecessary gas consumption when empty arrays are passed.
uint256 length = _addresses.length; +if (length == 0) return;Also applies to: 50-51
56-59: Add missing @inheritdoc for consistency.This function is missing the
@inheritdoctag while all other external functions have it.-/// @notice Adds or removes a specific function selector to/from the allowlist. -/// @param _selector The function selector to allow or disallow. -/// @param _approval Whether the function selector should be allowed. +/// @inheritdoc IWhitelistManagerFacet
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
audit/reports/2025.05.29_WhitelistManagerFacet(v1.0.0).pdfis excluded by!**/*.pdf
📒 Files selected for processing (64)
audit/auditLog.json(3 hunks)config/global.json(2 hunks)conventions.md(2 hunks)deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(2 hunks)deployments/arbitrum.staging.json(1 hunks)docs/DexManagerFacet.md(0 hunks)docs/README.md(1 hunks)docs/WhitelistManagerFacet.md(1 hunks)script/deploy/deployAllContracts.sh(2 hunks)script/deploy/facets/DeployDexManagerFacet.s.sol(0 hunks)script/deploy/facets/DeployWhitelistManagerFacet.s.sol(1 hunks)script/deploy/facets/UpdateCoreFacets.s.sol(2 hunks)script/deploy/facets/UpdateWhitelistManagerFacet.s.sol(1 hunks)script/deploy/healthCheck.ts(6 hunks)script/deploy/resources/contractSpecificReminders.sh(1 hunks)script/deploy/zksync/DeployDexManagerFacet.zksync.s.sol(0 hunks)script/deploy/zksync/DeployWhitelistManagerFacet.zksync.s.sol(1 hunks)script/deploy/zksync/UpdateCoreFacets.zksync.s.sol(2 hunks)script/helperFunctions.sh(5 hunks)script/tasks/diamondSyncSigs.ts(2 hunks)script/tasks/diamondSyncWhitelistedAddresses.sh(5 hunks)src/Facets/GenericSwapFacet.sol(1 hunks)src/Facets/GenericSwapFacetV3.sol(1 hunks)src/Facets/WhitelistManagerFacet.sol(1 hunks)src/Interfaces/IWhitelistManagerFacet.sol(1 hunks)src/Libraries/LibAllowList.sol(3 hunks)templates/facetTest.template.hbs(2 hunks)test/solidity/Facets/AcrossFacet.t.sol(2 hunks)test/solidity/Facets/AcrossFacetV3.t.sol(2 hunks)test/solidity/Facets/AllBridgeFacet.t.sol(2 hunks)test/solidity/Facets/ArbitrumBridgeFacet.t.sol(3 hunks)test/solidity/Facets/CBridge.t.sol(2 hunks)test/solidity/Facets/CBridgeAndFeeCollection.t.sol(2 hunks)test/solidity/Facets/CelerCircleBridgeFacet.t.sol(3 hunks)test/solidity/Facets/CelerIMFacet.t.sol(2 hunks)test/solidity/Facets/ChainflipFacet.t.sol(2 hunks)test/solidity/Facets/DeBridgeDlnFacet.t.sol(3 hunks)test/solidity/Facets/DexManagerFacet.t.sol(0 hunks)test/solidity/Facets/EmergencyPauseFacet.fork.t.sol(1 hunks)test/solidity/Facets/GasZipFacet.t.sol(3 hunks)test/solidity/Facets/GenericSwapFacet.t.sol(2 hunks)test/solidity/Facets/GenericSwapFacetV3.t.sol(10 hunks)test/solidity/Facets/GlacisFacet.t.sol(2 hunks)test/solidity/Facets/GnosisBridgeFacet.t.sol(3 hunks)test/solidity/Facets/HopFacet.t.sol(4 hunks)test/solidity/Facets/HopFacetOptimizedL1.t.sol(3 hunks)test/solidity/Facets/HopFacetOptimizedL2.t.sol(3 hunks)test/solidity/Facets/MayanFacet.t.sol(2 hunks)test/solidity/Facets/OmniBridgeFacet.t.sol(3 hunks)test/solidity/Facets/OmniBridgeL2Facet.t.sol(3 hunks)test/solidity/Facets/OptimismBridgeFacet.t.sol(3 hunks)test/solidity/Facets/PolygonBridgeFacet.t.sol(3 hunks)test/solidity/Facets/RelayFacet.t.sol(3 hunks)test/solidity/Facets/SquidFacet.t.sol(2 hunks)test/solidity/Facets/StargateFacetV2.t.sol(3 hunks)test/solidity/Facets/SymbiosisFacet.t.sol(2 hunks)test/solidity/Facets/ThorSwapFacet.t.sol(2 hunks)test/solidity/Facets/WhitelistManagerFacet.t.sol(1 hunks)test/solidity/Helpers/SwapperV2.t.sol(2 hunks)test/solidity/Libraries/LibAllowList.t.sol(1 hunks)test/solidity/Periphery/GasZipPeriphery.t.sol(5 hunks)test/solidity/utils/TestBase.sol(1 hunks)test/solidity/utils/TestHelpers.sol(2 hunks)
💤 Files with no reviewable changes (4)
- docs/DexManagerFacet.md
- script/deploy/facets/DeployDexManagerFacet.s.sol
- script/deploy/zksync/DeployDexManagerFacet.zksync.s.sol
- test/solidity/Facets/DexManagerFacet.t.sol
🧰 Additional context used
🧠 Learnings (6)
src/Facets/GenericSwapFacet.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
test/solidity/Facets/EmergencyPauseFacet.fork.t.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#829
File: deployments/optimism.diamond.staging.json:4-7
Timestamp: 2024-10-09T07:06:25.731Z
Learning: In `src/Facets/EmergencyPauseFacet.sol`, the `emergencyPause` and `emergencyUnpause` functions are correctly implemented and present.
src/Facets/GenericSwapFacetV3.sol (1)
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
script/deploy/deployAllContracts.sh (1)
Learnt from: mirooon
PR: lifinance/contracts#1145
File: script/tasks/diamondSyncWhitelistedAddresses.sh:208-209
Timestamp: 2025-06-05T10:00:01.558Z
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.
conventions.md (5)
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: audit/auditLog.json:19-21
Timestamp: 2024-10-07T02:43:45.445Z
Learning: In Solidity projects, test files may have the `.t.sol` extension and can be located in `test/solidity/` directories. Remember to include these when searching for test files.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: audit/auditLog.json:19-21
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In Solidity projects, test files may have the `.t.sol` extension and can be located in `test/solidity/` directories. Remember to include these when searching for test files.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: audit/auditLog.json:19-21
Timestamp: 2024-10-07T02:43:45.445Z
Learning: In Solidity projects, test files may have the `.t.sol` extension and can be located in `test/solidity/` directories. Ensure to check these directories when verifying test coverage.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: audit/auditLog.json:19-21
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In Solidity projects, test files may have the `.t.sol` extension and can be located in `test/solidity/` directories. Ensure to check these directories when verifying test coverage.
Learnt from: reednaa
PR: lifinance/contracts#1090
File: test/solidity/Facets/PioneerFacet.t.sol:11-21
Timestamp: 2025-04-08T10:29:48.915Z
Learning: Test files (typically in test/ directories with .t.sol extensions) may intentionally expose functions without access control for testing purposes. Access control suggestions are generally not applicable to test code.
script/deploy/healthCheck.ts (2)
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
🧬 Code Graph Analysis (2)
script/deploy/deployAllContracts.sh (1)
script/tasks/diamondSyncWhitelistedAddresses.sh (1)
diamondSyncWhitelistedAddresses(3-208)
script/tasks/diamondSyncWhitelistedAddresses.sh (1)
script/helperFunctions.sh (1)
getPrivateKey(2869-2900)
🪛 Shellcheck (0.10.0)
script/deploy/resources/contractSpecificReminders.sh
[warning] 8-8: FeeCollector appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 9-9: GasZipPeriphery appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 10-10: LiFiDEXAggregator appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 11-11: TokenWrapper appears unused. Verify use (or export if used externally).
(SC2034)
script/tasks/diamondSyncWhitelistedAddresses.sh
[warning] 97-97: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 128-128: Quote this to prevent word splitting.
(SC2046)
[warning] 133-133: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
script/helperFunctions.sh
[warning] 1363-1363: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1379-1379: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 LanguageTool
docs/WhitelistManagerFacet.md
[uncategorized] ~11-~11: Possible missing comma found.
Context: .... This can be updated by a single admin key which if compromised could lead to mali...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 GitHub Actions: VersionControlAndAuditVerification
src/Libraries/LibAllowList.sol
[error] 1-1: Could not find any logged audit for contract LibAllowList in version 1.0.2.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (112)
deployments/_deployments_log_file.json (2)
33791-33794:✅ Verification successful
Validate JSON syntax after insertion.
Ensure the comma after the previous facet block and the new"WhitelistManagerFacet"key maintain valid JSON structure.Run a quick lint to catch any syntax errors:
🏁 Script executed:
jq . deployments/_deployments_log_file.jsonLength of output: 65582
JSON syntax validated for deployments/_deployments_log_file.json
Ranjq . deployments/_deployments_log_file.jsonwith no errors—commas and new"WhitelistManagerFacet"entry maintain a valid JSON structure.
33795-33803:✅ Verification successful
Verify field types against schema.
The fieldsOPTIMIZER_RUNS,TIMESTAMP,CONSTRUCTOR_ARGS,SALT, andVERIFIEDare all strings. Confirm this matches the format of existing log entries or consider converting numeric/boolean values to proper JSON types (number/boolean) for consistency.
🏁 Script executed:
# Inspect the new entry in context jq '.[].WhitelistManagerFacet.arbitrum.staging["1.0.3"][0]' deployments/_deployments_log_file.jsonLength of output: 495
🏁 Script executed:
#!/usr/bin/env bash # Sample occurrences of our target fields to verify their JSON types fields=("TIMESTAMP" "OPTIMIZER_RUNS" "CONSTRUCTOR_ARGS" "SALT" "VERIFIED") for f in "${fields[@]}"; do echo -e "\n=== $f ===" rg -n "\"$f\"" deployments/_deployments_log_file.json | head -n 5 doneLength of output: 1837
All deployment log fields are consistently strings
Our sampling confirms thatTIMESTAMP,OPTIMIZER_RUNS,CONSTRUCTOR_ARGS,SALT, andVERIFIEDare stored as JSON strings across existing entries. The new entry matches this format. No changes are needed unless your schema specifically mandates non-string types.script/deploy/facets/UpdateWhitelistManagerFacet.s.sol (1)
11-11: LGTM - Facet name update aligns with migration.The parameter change from "DexManagerFacet" to "WhitelistManagerFacet" is consistent with the broader migration described in the PR objectives.
deployments/arbitrum.staging.json (1)
56-57: Verify coexistence of DexManagerFacet and WhitelistManagerFacet.The configuration now contains both
DexManagerFacet(line 5) andWhitelistManagerFacet(lines 56-57). Please confirm if both facets should coexist during the migration or if theDexManagerFacetentry should be removed as part of this replacement.deployments/arbitrum.diamond.staging.json (1)
24-27: LGTM - Consistent facet replacement.The replacement of DexManagerFacet with WhitelistManagerFacet is correctly implemented. The address
0x603f0c31B37E5ca3eA75D5730CCfaBCFF6D17aa3is consistent with the entry inarbitrum.staging.json, and version1.0.3indicates a mature implementation.src/Facets/GenericSwapFacetV3.sol (1)
14-15: LGTM - Documentation aligns with whitelist terminology.The NatSpec comment updates from "APPROVED" to "WHITELISTED" correctly reflect the migration to whitelist-based terminology while maintaining the existing logic that uses
LibAllowListfunctions.src/Facets/GenericSwapFacet.sol (1)
14-15: Documentation update aligns with whitelist refactoring.The terminology change from "APPROVED" to "WHITELISTED" correctly reflects the migration from DexManagerFacet to WhitelistManagerFacet. The documentation now accurately describes the contract's behavior with the new whitelist management system.
test/solidity/utils/TestBase.sol (1)
21-23: Function renaming maintains consistency with whitelist refactoring.The rename from
addDextoaddToWhitelistwith the more generic parameter name_contractAddressappropriately reflects the broader scope of the new whitelist management system. The underlying functionality remains unchanged.test/solidity/Facets/SymbiosisFacet.t.sol (2)
47-47: Function selector correctly updated for renamed function.The selector update from
addDex.selectortoaddToWhitelist.selectormaintains consistency with the function renaming in the test utilities.
56-56: Function call updated to match renamed function.The call to
addToWhitelist(address(uniswap))correctly reflects the function renaming while maintaining the same functionality of adding the Uniswap address to the whitelist.test/solidity/Facets/EmergencyPauseFacet.fork.t.sol (1)
17-19: Consistent function renaming maintains test utility functionality.The rename from
addDextoaddToWhitelistwith the updated parameter name follows the same pattern as other test utilities, ensuring consistency across the test suite while maintaining identical functionality.conventions.md (2)
189-189: Documentation update aligns with the codebase refactoring.The test file naming convention update from
DexManagerFacet.t.soltoWhitelistManagerFacet.t.solproperly reflects the broader transition from DexManager to WhitelistManager terminology.
199-200: Function naming examples updated consistently.The test function name examples have been appropriately updated from DEX-related terminology (
addDex,AddsDex) to whitelist-related terminology (addToWhitelist,AddsToWhitelist), maintaining consistency with the broader refactoring effort.script/tasks/diamondSyncSigs.ts (2)
77-78: Variable naming updated consistently with the refactoring.The comment and variable name have been appropriately updated from "dex manager" to "whitelist manager" terminology. The contract address and ABI remain unchanged, maintaining the same functionality while improving code clarity.
90-90: Contract reference updated to match the renamed variable.The spread operator correctly uses the renamed
whitelistManagerReadervariable, maintaining consistency with the variable name change.script/deploy/facets/UpdateCoreFacets.s.sol (1)
92-98: Diamond cut operations updated to use WhitelistManagerFacet.The comments, selector retrieval, and diamond cut operations have been correctly updated to reference
WhitelistManagerFacetinstead ofDexManagerFacet, maintaining the same deployment logic with the new facet.test/solidity/Facets/RelayFacet.t.sol (3)
25-27: Function renamed to use whitelist terminology.The function has been appropriately renamed from
addDextoaddToWhitelistwith a more generic parameter name_contractAddress. The underlying functionality usingLibAllowList.addAllowedContractremains unchanged, maintaining the same behavior while improving naming consistency.
63-63: Function selector updated to match the renamed function.The function selector correctly references the renamed
addToWhitelistfunction.
72-72: Function call updated consistently.The function call has been updated to use the new
addToWhitelistfunction name, maintaining the same functionality with improved naming consistency.test/solidity/Facets/GnosisBridgeFacet.t.sol (2)
17-19: LGTM - Consistent function renaming from DEX-specific to general whitelist management.The function renaming from
addDextoaddToWhitelistwith parameter change from_dexto_contractAddresscorrectly reflects the conceptual shift from DEX-specific management to general contract whitelist management. The internal logic correctly remains unchanged.
60-60:✅ Verification successful
Verify consistency of function renaming across all test files.
The function selector and call have been correctly updated to match the renamed function. Ensure all similar test files have been updated consistently.
Also applies to: 69-69
🏁 Script executed:
#!/bin/bash # Description: Verify all test files have been consistently updated from addDex to addToWhitelist # Expected: No remaining references to addDex in test files echo "Searching for any remaining addDex references in test files..." rg "addDex" test/ || echo "No addDex references found - good!" echo -e "\nSearching for addToWhitelist references to verify updates..." rg "addToWhitelist" test/ -A 2 -B 1Length of output: 42256
Consistent renaming verified across all tests
- Searched under test/solidity and found no remaining
addDexreferences- Every test now uses
addToWhitelist(including GnosisBridgeFacet.t.sol at lines 60 and 69)All occurrences have been updated correctly. Approving these changes.
script/deploy/zksync/DeployWhitelistManagerFacet.zksync.s.sol (1)
1-16:✅ Verification successful
LGTM - Well-structured deployment script following established patterns.
The deployment script correctly follows the standard pattern used by other facet deployment scripts:
- Proper inheritance from
DeployScriptBase- Correct constructor with facet name
- Standard deployment using
type().creationCode
🏁 Script executed:
#!/bin/bash # Description: Verify this deployment script follows the same pattern as other facet deployment scripts # Expected: Consistent structure and naming patterns echo "Checking other zkSync deployment scripts for pattern consistency..." fd -e sol -x cat {} \; script/deploy/zksync/ | head -50 echo -e "\nVerifying WhitelistManagerFacet import path exists..." fd "WhitelistManagerFacet.sol" src/Length of output: 833
🏁 Script executed:
#!/bin/bash # List all zkSync deployment scripts and show their initial structure for comparison echo "=== Deployment scripts in script/deploy/zksync/ ===" fd -e sol . script/deploy/zksync/ echo -e "\n=== First 20 lines of each script ===" for file in script/deploy/zksync/*.sol; do echo -e "\n--- $file ---" head -n 20 "$file" doneLength of output: 28372
✅ Deployment script verified and approved
script/deploy/zksync/DeployWhitelistManagerFacet.zksync.s.solmatches the established pattern used by other zkSync facet deployment scripts (license, pragma, imports, contract structure).- No constructor arguments or extra imports—consistent with zero-arg facets like
GenericSwapFacet.- Import path
src/Facets/WhitelistManagerFacet.solexists and is correct.- Uses
type(WhitelistManagerFacet).creationCodedeployment, as expected.All checks passed.
test/solidity/Facets/AllBridgeFacet.t.sol (2)
13-15: LGTM - Consistent with the whitelist management refactor.The function renaming from
addDextoaddToWhitelistis consistent with the changes in other test files and correctly reflects the shift to general whitelist management.
43-43: Function selector and call correctly updated.The function selector assignment and function call have been properly updated to match the renamed function.
Also applies to: 50-50
config/global.json (2)
49-49: Core facets list correctly updated.The replacement of "DexManagerFacet" with "WhitelistManagerFacet" in the coreFacets array is correct and consistent with the overall refactor.
21-21:❓ Verification inconclusive
Verify function signatures match the new WhitelistManagerFacet function names.
The function names have been correctly updated from DexManagerFacet to WhitelistManagerFacet terminology. However, ensure the function signatures (hex values) are correct for the new function names.
Also applies to: 25-25, 29-29
🏁 Script executed:
#!/bin/bash # Description: Verify function signatures match the new WhitelistManagerFacet functions # Expected: Function signatures should match the actual function selectors echo "Checking if WhitelistManagerFacet exists and finding function signatures..." if fd "WhitelistManagerFacet.sol" src/; then echo -e "\nExtracting function signatures from WhitelistManagerFacet contract..." ast-grep --pattern 'function batchAddToWhitelist($$$) external $$$' ast-grep --pattern 'function batchRemoveFromWhitelist($$$) external $$$' ast-grep --pattern 'function batchSetFunctionApprovalBySignature($$$) external $$$' else echo "WhitelistManagerFacet.sol not found" fi echo -e "\nChecking for any remaining DexManagerFacet references in config..." rg "DexManagerFacet" config/ || echo "No DexManagerFacet references found - good!"Length of output: 936
🏁 Script executed:
#!/bin/bash # Extract WhitelistManagerFacet function declarations for signature verification rg -n "function batchAddToWhitelist" -C2 src/Facets/WhitelistManagerFacet.sol rg -n "function batchRemoveFromWhitelist" -C2 src/Facets/WhitelistManagerFacet.sol rg -n "function batchSetFunctionApprovalBySignature" -C2 src/Facets/WhitelistManagerFacet.solLength of output: 737
#!/bin/bash # Extract current signatures from config/global.json echo "Config file signatures:" jq -r '.approvedSigsForDeployerWallet[] | "\(.name) \(.sig)"' config/global.json # Compute expected selectors for WhitelistManagerFacet functions echo -e "\nExpected selectors:" pip install --quiet eth-utils > /dev/null 2>&1 python3 - <<EOF from eth_utils import function_signature_to_4byte_selector functions = [ "batchAddToWhitelist(address[])", "batchRemoveFromWhitelist(address[])", "batchSetFunctionApprovalBySignature(bytes4,bool)" ] for fn in functions: sel = function_signature_to_4byte_selector(fn).hex() print(f"{fn} 0x{sel}") EOF
#!/usr/bin/env bash # Compare configured selectors with computed selectors for WhitelistManagerFacet echo "Configured signatures in config/global.json:" jq -r '.approvedSigsForDeployerWallet[] | "\(.name) \(.sig)"' config/global.json || exit 1 echo -e "\nComputed expected selectors:" python3 - <<'EOF' import hashlib functions = [ "batchAddToWhitelist(address[])", "batchRemoveFromWhitelist(address[])", "batchSetFunctionApprovalBySignature(bytes4,bool)" ] for fn in functions: sel = hashlib.sha3_256(fn.encode()).digest()[:4].hex() print(f"{fn} 0x{sel}") EOF
#!/usr/bin/env bash # Compare configured selectors with computed selectors for WhitelistManagerFacet functions echo "Configured signatures in config/global.json:" jq -r '.approvedSigsForDeployerWallet[] | "\(.name) \(.sig)"' config/global.json || exit 1 echo -e "\nComputed expected selectors:" python3 - <<'EOF' from sha3 import keccak_256 functions = [ "batchAddToWhitelist(address[])", "batchRemoveFromWhitelist(address[])", "batchSetFunctionApprovalBySignature(bytes4,bool)" ] for fn in functions: selector = keccak_256(fn.encode()).digest()[:4].hex() print(f"{fn} 0x{selector}") EOF
🏁 Script executed:
#!/bin/bash # Verify configured vs expected selectors for WhitelistManagerFacet functions set -e echo "Configured signatures in config/global.json:" jq -r '.approvedSigsForDeployerWallet[] | "\(.name) \(.sig)"' config/global.json echo -e "\nComputed expected selectors:" pip install --quiet eth-utils > /dev/null 2>&1 python3 - <<EOF from eth_utils import function_signature_to_4byte_selector functions = [ "batchAddToWhitelist(address[])", "batchRemoveFromWhitelist(address[])", "batchSetFunctionApprovalBySignature(bytes4,bool)" ] for fn in functions: sel = function_signature_to_4byte_selector(fn).hex() print(f"{fn} 0x{sel}") EOFLength of output: 2195
#!/usr/bin/env bash set -e echo "Configured selectors in config/global.json:" jq -r '.approvedSigsForDeployerWallet[] | "\(.name) \(.sig)"' config/global.json echo -e "\nComputed selectors via sha3_256:" python3 - <<'EOF' import hashlib functions = [ ("WhitelistManagerFacet.batchAddToWhitelist", "batchAddToWhitelist(address[])"), ("WhitelistManagerFacet.batchRemoveFromWhitelist", "batchRemoveFromWhitelist(address[])"), ("WhitelistManagerFacet.batchSetFunctionApprovalBySignature", "batchSetFunctionApprovalBySignature(bytes4,bool)") ] for name, sig in functions: selector = hashlib.sha3_256(sig.encode()).digest()[:4].hex() print(f"{name} 0x{selector}") EOF
Confirm WhitelistManagerFacet function selectors
Please manually verify that the hex selectors in config/global.json match the actual selectors generated by the WhitelistManagerFacet contract:
- WhitelistManagerFacet.batchAddToWhitelist → 0xfcd8e49e
- WhitelistManagerFacet.batchRemoveFromWhitelist → 0x9afc19c7
- WhitelistManagerFacet.batchSetFunctionApprovalBySignature → 0x44e2b18c
Ensure these values correspond exactly to the 4-byte selectors of:
batchAddToWhitelist(address[])batchRemoveFromWhitelist(address[])batchSetFunctionApprovalBySignature(bytes4,bool)test/solidity/Facets/CelerCircleBridgeFacet.t.sol (3)
16-18: LGTM! Function renaming aligns with the broader refactor.The function name
addToWhitelistand parameter_contractAddressare more descriptive and accurate than the previousaddDexterminology, reflecting the generalized whitelist management approach.
52-52: Function selector correctly updated.The selector reference has been properly updated to match the renamed function.
61-61: Function call correctly updated.The function call has been properly updated to use the new
addToWhitelistmethod.test/solidity/Facets/CelerIMFacet.t.sol (3)
27-29: LGTM! Consistent with the whitelist refactor pattern.The function renaming follows the same pattern established across other test files, maintaining consistency in the codebase refactor.
122-122: Function selector correctly updated.The selector reference properly reflects the renamed function.
130-130: Function call correctly updated.The function invocation has been properly updated to use the new method name.
docs/WhitelistManagerFacet.md (1)
1-12: Good documentation for the new WhitelistManagerFacet.The documentation clearly explains the purpose and functionality of the WhitelistManagerFacet, and appropriately highlights the security implications of centralized admin control.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Possible missing comma found.
Context: .... This can be updated by a single admin key which if compromised could lead to mali...(AI_HYDRA_LEO_MISSING_COMMA)
test/solidity/Facets/PolygonBridgeFacet.t.sol (3)
16-18: LGTM! Consistent refactor completion.The function renaming maintains consistency with the broader WhitelistManagerFacet refactor across all test files.
51-51: Function selector correctly updated.The selector reference has been properly updated to match the renamed function.
60-60: Function call correctly updated.The function invocation properly uses the new
addToWhitelistmethod name.test/solidity/Facets/ChainflipFacet.t.sol (2)
18-21: LGTM! Consistent function renaming aligned with whitelist migration.The function has been correctly renamed from
addDextoaddToWhitelistwith appropriate parameter name updates. The underlying functionality usingLibAllowList.addAllowedContractremains unchanged, which is correct for the migration to generalized whitelist management.
57-57: LGTM! Function selector and invocation updated consistently.Both the function selector array and the actual function call have been properly updated to use the new
addToWhitelistfunction name, maintaining consistency throughout the test setup.Also applies to: 64-64
test/solidity/Helpers/SwapperV2.t.sol (2)
29-31: LGTM! Consistent with whitelist migration pattern.The function renaming from
addDextoaddToWhitelistfollows the established pattern across the codebase migration. The parameter name change to_contractAddressappropriately reflects the more general purpose.
49-49: LGTM! Selector and invocation updates are consistent.The function selector and call have been properly updated to match the renamed function, maintaining consistency in the test setup.
Also applies to: 57-57
test/solidity/Facets/AcrossFacetV3.t.sol (2)
20-22: LGTM! Function renaming aligns with architectural migration.The function has been properly renamed from
addDextoaddToWhitelistwith the parameter name appropriately updated to_contractAddress. This change is consistent with the broader migration from DEX-specific to generalized whitelist management.
54-54: LGTM! Complete consistency in selector and invocation updates.Both the function selector registration and the actual function invocation have been correctly updated to use the new function name, ensuring the test setup remains functional after the migration.
Also applies to: 61-61
script/deploy/facets/DeployWhitelistManagerFacet.s.sol (1)
1-15: LGTM! Clean deployment script following established patterns.The new deployment script for
WhitelistManagerFacetfollows the standard pattern used throughout the codebase:
- Properly inherits from
DeployScriptBasewith the correct facet name- Uses the standard
type().creationCodedeployment approach- Returns the deployed contract instance as expected
- Maintains simplicity and focus appropriate for a deployment script
This script correctly replaces the previous DexManagerFacet deployment as part of the architectural migration.
test/solidity/Facets/CBridge.t.sol (2)
15-17: LGTM! Function rename aligns with whitelist management refactor.The function signature change from
addDextoaddToWhitelistand parameter rename from_dexto_contractAddressproperly reflects the shift from DEX-specific management to general whitelist management.
88-88: Consistent updates to function selector and call.The function selector and call updates properly reflect the renamed
addToWhitelistfunction, maintaining consistency throughout the test setup.Also applies to: 95-95
test/solidity/Facets/HopFacetOptimizedL1.t.sol (1)
13-15: Consistent refactoring pattern maintained.The function rename from
addDextoaddToWhitelist, selector update, and function call update follow the same pattern as other test files, maintaining consistency across the codebase refactoring.Also applies to: 54-54, 63-63
script/deploy/deployAllContracts.sh (3)
13-13: Script sourcing updated for whitelist management.The change from sourcing DEX synchronization script to whitelist addresses synchronization script aligns with the broader refactoring from DexManagerFacet to WhitelistManagerFacet.
150-151: Function call updated to manage whitelisted addresses.The change from
addPeripheryToDexsJsontoaddPeripheryToWhitelistedAddressesJsonproperly reflects the shift to whitelist-based management for periphery contracts.
155-155: Synchronization function updated for whitelist management.The change from
diamondSyncDEXstodiamondSyncWhitelistedAddressesensures the deployment script uses the new whitelist synchronization functionality instead of DEX-specific management.test/solidity/Facets/SquidFacet.t.sol (1)
19-21: Systematic refactoring pattern applied consistently.The function rename from
addDextoaddToWhitelist, function selector update, and function call update maintain the same consistent pattern applied across all test files in this refactoring effort.Also applies to: 56-56, 63-63
test/solidity/Facets/OmniBridgeL2Facet.t.sol (3)
16-17: LGTM! Improved abstraction with whitelist terminology.The function renaming from
addDextoaddToWhitelistand parameter renaming from_dexto_contractAddressbetter reflects the generalized whitelist management functionality. The internal logic correctly remains unchanged.
60-60: Function selector correctly updated.The selector reference has been properly updated to match the new function name.
69-69: Function call correctly updated.The function invocation has been properly updated to use the new
addToWhitelistmethod.test/solidity/Facets/AcrossFacet.t.sol (3)
18-19: LGTM! Consistent refactor pattern applied.The function renaming follows the same pattern as other test facets, maintaining consistency across the codebase. The generalized naming better reflects the whitelist management functionality.
48-48: Function selector properly updated.The selector has been correctly updated to reference the new function name.
55-55: Function call properly updated.The function invocation correctly uses the new
addToWhitelistmethod.test/solidity/Facets/OmniBridgeFacet.t.sol (3)
16-17: LGTM! Consistent with whitelist refactor.The function signature change maintains consistency with the broader refactor from DEX-specific to generalized whitelist management terminology.
51-51: Selector reference correctly updated.The function selector has been properly updated to match the new function name.
60-60: Function invocation properly updated.The call to the whitelist function has been correctly updated.
test/solidity/Facets/ThorSwapFacet.t.sol (3)
13-14: LGTM! Refactor consistently applied.The function renaming maintains the established pattern across all test facets, completing the transition from DEX-specific to generalized whitelist terminology.
41-41: Function selector appropriately updated.The selector reference correctly reflects the new function name.
49-49: Function call correctly updated.The function invocation properly uses the new
addToWhitelistmethod, completing the refactor pattern.test/solidity/Facets/HopFacet.t.sol (4)
14-16: LGTM! Consistent function renaming for whitelist management.The function has been appropriately renamed from
addDextoaddToWhitelistwith a more descriptive parameter name. The underlying logic remains correct.
52-52: Function selector properly updated.The selector array correctly reflects the renamed function.
67-67: Function call correctly updated.The function call has been properly updated to use the new whitelist terminology.
245-245: Selector reference consistently updated.The function selector is correctly updated in the second test setup.
script/deploy/zksync/UpdateCoreFacets.zksync.s.sol (2)
21-24: Deployment configuration correctly updated.The variable naming and config path have been properly updated to reflect the WhitelistManagerFacet replacement.
92-98: Facet deployment logic consistently updated.Comments and selector retrieval have been properly updated for the WhitelistManagerFacet.
test/solidity/Facets/OptimismBridgeFacet.t.sol (3)
11-13: LGTM! Function renaming aligns with whitelist management approach.The function has been consistently renamed with appropriate parameter naming.
61-61: Function selector correctly updated.The selector array properly reflects the renamed function.
78-78: Function call properly updated.The call to add uniswap to the whitelist uses the correct new function name.
test/solidity/Facets/MayanFacet.t.sol (3)
15-17: LGTM! Consistent function renaming across test contracts.The function naming follows the established pattern for the whitelist management migration.
113-113: Function selector appropriately updated.The selector array correctly includes the renamed function.
120-120: Function call correctly updated for whitelist management.The call to add ADDRESS_UNISWAP to the whitelist uses the proper new function name.
test/solidity/Facets/ArbitrumBridgeFacet.t.sol (1)
16-18: LGTM! Consistent refactoring from DEX-specific to generalized whitelist management.The function renaming from
addDextoaddToWhitelistwith parameter name change to_contractAddressbetter reflects the generalized whitelist management approach. All references including function selectors and calls have been properly updated while preserving the underlying logic.Also applies to: 55-55, 64-64
test/solidity/Facets/HopFacetOptimizedL2.t.sol (1)
13-15: LGTM! Consistent implementation of whitelist management refactor.The changes maintain the same pattern as other facets: function renamed to
addToWhitelistwith improved parameter naming, function selector properly updated, and all calls consistently updated. The underlying whitelist logic is preserved.Also applies to: 58-58, 67-67
test/solidity/Facets/StargateFacetV2.t.sol (1)
19-21: LGTM! Comprehensive whitelist management updates with multiple consistent references.The refactoring is well-executed with the function renamed to
addToWhitelist, function selector updated, and multiple calls (for bothuniswapandfeeCollector) consistently updated. The generalized naming better reflects the broader whitelist management purpose.Also applies to: 72-72, 82-83
test/solidity/Facets/DeBridgeDlnFacet.t.sol (1)
15-17: LGTM! Final consistent implementation of the whitelist management refactor.This completes the consistent pattern across all test facets: function renamed from
addDextoaddToWhitelistwith improved parameter naming, function selector and calls properly updated, while preserving the underlyingLibAllowList.addAllowedContractfunctionality. The refactor successfully generalizes from DEX-specific to broader whitelist management.Also applies to: 61-61, 71-71
templates/facetTest.template.hbs (3)
13-14: LGTM! Function renaming improves generalization.The renaming from
addDextoaddToWhitelistwith parameter change from_dexto_contractAddresssuccessfully generalizes the function from DEX-specific to broader whitelist management. The underlying functionality remains intact.
43-43: Function selector reference correctly updated.The selector reference has been properly updated to match the renamed function.
50-50: Function call correctly updated.The function call has been properly updated to use the new function name while maintaining the same functionality.
test/solidity/Facets/GlacisFacet.t.sol (3)
16-17: Consistent refactoring applied to GlacisFacet test.The function renaming and parameter update are consistent with the broader whitelist management refactoring. The functionality remains unchanged while improving naming clarity.
57-57: Function selector properly updated.The selector array correctly reflects the renamed function.
64-64: Function call correctly updated.The function invocation properly uses the new function name while maintaining the same behavior of whitelisting the Uniswap address.
test/solidity/Facets/CBridgeAndFeeCollection.t.sol (3)
15-16: Refactoring consistently applied to CBridge test.The function signature update aligns with the whitelist management refactoring while preserving the underlying allowlist functionality.
43-43: Selector array correctly updated.The function selector has been properly updated to reference the renamed function.
49-50: Function calls properly updated for whitelist management.Both the uniswap and feeCollector addresses are correctly whitelisted using the new function name, maintaining the same functionality while adopting the improved naming convention.
test/solidity/Facets/GenericSwapFacet.t.sol (3)
9-10: GenericSwapFacet test updated consistently.The function signature change from
addDextoaddToWhitelistwith the more descriptive parameter name_contractAddressaligns with the whitelist management refactoring.
38-38: Function selector correctly updated.The selector array properly references the renamed function.
46-46: Function call properly updated.The uniswap address is correctly whitelisted using the new function name, maintaining test functionality while adopting the improved naming convention.
audit/auditLog.json (1)
262-268: LGTM! Audit log entries are properly structured.The new audit entry for WhitelistManagerFacet v1.0.0 follows the established format and includes all required fields. The contract mappings correctly associate the audit with both LibAllowList v1.0.1 and the new WhitelistManagerFacet v1.0.0.
Also applies to: 463-464, 556-558
test/solidity/utils/TestHelpers.sol (2)
8-12: LGTM! Interface rename is consistent and well-structured.The rename from
DexManagertoIWhitelistManagerwith the corresponding method name change fromaddDextoaddToWhitelistaligns with the broader architectural shift from DEX-specific management to generalized whitelist management.
56-77: LGTM! Method calls updated correctly.All calls to the interface have been properly updated to use
IWhitelistManagerand theaddToWhitelistmethod. The function selector approvals remain unchanged, which is correct since the underlying mock DEX contract interface hasn't changed.script/tasks/diamondSyncWhitelistedAddresses.sh (1)
3-5: LGTM! Comprehensive and consistent rename from DEX to whitelist management.The function name, variable names, config file path, and contract method calls have all been properly updated to reflect the whitelist management functionality. The logic flow remains sound.
Also applies to: 70-70, 73-73, 78-78, 97-97, 128-128, 133-133
test/solidity/Facets/GasZipFacet.t.sol (2)
13-19: LGTM! Function renaming aligns with whitelist management refactor.The renaming from
addDex/removeDextoaddToWhitelist/removeFromWhitelistis consistent with the broader transition from DEX-specific management to general whitelist management. The underlyingLibAllowListfunctionality is preserved.
65-66: LGTM! All function references consistently updated.All selectors and function calls have been systematically updated to use the new whitelist function names, maintaining consistency throughout the test contract.
Also applies to: 75-76
test/solidity/Libraries/LibAllowList.t.sol (1)
1-111: Excellent comprehensive test coverage for LibAllowList library.This new test file provides thorough coverage of the LibAllowList functionality, including:
- Add/remove operations for both contracts and selectors
- Edge cases like removing non-existent entries
- Multiple entity management scenarios
- Proper state verification after operations
The test structure follows good practices with clear, descriptive test names and comprehensive assertions.
test/solidity/Facets/GenericSwapFacetV3.t.sol (2)
15-21: LGTM! Consistent function renaming across test contracts.Both
TestGenericSwapFacetV3andTestGenericSwapFacetcontracts have been consistently updated with the new whitelist function names. Function selectors in the setup are properly updated to match.Also applies to: 29-35, 71-72
106-106: LGTM! All function calls systematically updated throughout extensive test suite.All references to the renamed functions have been consistently updated across the comprehensive test scenarios, maintaining the same test logic while using the new whitelist terminology.
Also applies to: 120-120, 136-136, 144-144, 671-671, 827-827, 1135-1135, 1968-1968
script/deploy/healthCheck.ts (3)
63-65: LGTM!The import and variable naming changes properly reflect the transition from DEX-specific to general whitelist management.
265-283: Contract interaction properly updated.The migration from
dexManagertowhitelistManagerwith corresponding ABI and method updates is correctly implemented, maintaining proper error handling.
338-371: Excellent refactor of signature verification logic!The new implementation is more efficient (single contract call vs multiple), provides comprehensive bidirectional checking (missing and extra signatures), and offers better error reporting. The normalization of signatures ensures consistent comparison.
src/Interfaces/IWhitelistManagerFacet.sol (2)
9-19: Well-designed events with appropriate indexing.The events properly cover all state changes and use indexed parameters effectively for efficient log filtering.
21-86: Comprehensive and well-organized interface design.The interface provides a complete set of functions for whitelist management with:
- Single and batch operations for efficiency
- Clear separation of concerns
- Appropriate view modifiers
- Important gas warnings for unbounded reads (lines 61-63 and 78-80)
Excellent interface design that balances functionality with safety considerations.
test/solidity/Facets/WhitelistManagerFacet.t.sol (4)
245-332: Excellent access control test coverage.All protected functions are properly tested for unauthorized access with specific error expectations. The consistent testing pattern ensures comprehensive security validation.
193-211: Proper validation of invalid address inputs.Good coverage of edge cases including zero address and non-contract address validation, ensuring the contract properly rejects invalid inputs.
665-784: Critical tests for indexed storage integrity.Excellent test coverage for the indexed storage implementation. These tests are crucial for ensuring the LibAllowList maintains data integrity during additions and removals, especially the array compaction logic.
458-586: Thorough testing of selector retrieval functionality.Comprehensive test coverage for
getApprovedFunctionSelectorsincluding empty state, additions, and various removal scenarios. The tests properly verify array contents and ordering.src/Facets/WhitelistManagerFacet.sol (1)
116-147: Well-implemented internal functions with proper validation.The internal helper functions demonstrate good practices:
- Early returns to avoid unnecessary operations
- Self-authorization prevention
- State change verification before updates
- Appropriate event emissions
src/Libraries/LibAllowList.sol (3)
13-23: Proper storage upgrade with namespace versioning.The storage upgrade correctly uses a new namespace (v2) to prevent storage collisions. The addition of index mappings and arrays enables efficient O(1) removal operations while maintaining backward compatibility.
28-43: Excellent validation enhancements in addAllowedContract.The function now includes comprehensive validation:
- Zero address prevention
- Contract existence verification (including EIP-7702 AA wallet support)
- Duplicate prevention with early return
- Efficient indexing for O(1) removals
55-83:✅ Verification successful
Efficient O(1) removal implementation with proper edge case handling.
The removal functions implement a sophisticated swap-and-pop pattern with 1-based indexing. The implementation correctly:
- Handles the edge case when removing the last element
- Updates indices for the swapped element
- Cleans up all related mappings
- Maintains data integrity throughout the operation
To ensure the removal logic works correctly in all scenarios, consider verifying with this test:
Also applies to: 107-135
🏁 Script executed:
#!/bin/bash # Description: Search for test coverage of the removal edge cases # Look for tests that verify removal of last element and middle elements rg -A 10 "removeAllowedContract|removeAllowedSelector" test/solidity/Libraries/LibAllowList.t.solLength of output: 2691
Removal logic is well-tested and correct
The swap-and-pop removal pattern with 1-based indexing is efficient and handles all edge cases, and we have explicit test coverage for:
- Removing the only element
- Removing a non-existent element (no-op)
- Removing a middle element and verifying indices update correctly
- Equivalent selector removal scenarios
All mappings are cleaned up properly, and data integrity is maintained.
✅ Approve changes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
script/tasks/diamondSyncWhitelistedAddresses.sh (3)
97-97: Use mapfile to populate arrays safely. The current assignment (WHITELISTED_ADDRESSES=($(getWhitelistedAddresses))) risks splitting issues. Replace with:-mapfile -t WHITELISTED_ADDRESSES < <(getWhitelistedAddresses)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 97-97: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
128-128: Quote private-key command substitution. The unquoted$(getPrivateKey …)can cause word splitting; update to:--private-key $(getPrivateKey ...) +-private-key "$(getPrivateKey ...)"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 128-128: Quote this to prevent word splitting.
(SC2046)
133-133: Use mapfile for updated array. Apply the same safe array population as above:-mapfile -t WHITELISTED_ADDRESSES_UPDATED < <(getWhitelistedAddresses)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 133-133: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
🧹 Nitpick comments (3)
script/tasks/diamondSyncWhitelistedAddresses.sh (3)
70-70: Validate configuration file path. The jq command retrieves the whitelist as expected; consider adding a file‐existence check (e.g.[[ -r "./config/whitelistedAddresses.json" ]]) to fail early if the config is missing.
73-79: Safe output in helper function. IngetWhitelistedAddresses, wrapping the trimmedresultin quotes prevents unintended word splitting. For example:echo "${result:1:${#result}-2}" | tr ',' '\n' | tr '[:upper:]' '[:lower:]'
110-111: Loop iteration quoting. Iterating over$CFG_WHITELISTED_ADDRESSEScan split on whitespace; consider loading it into an array (viamapfile) and iterating over"${CFG_WHITELISTED_ADDRESSES[@]}".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
script/tasks/diamondSyncWhitelistedAddresses.sh(5 hunks)src/Facets/GenericSwapFacet.sol(1 hunks)src/Facets/GenericSwapFacetV3.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Facets/GenericSwapFacetV3.sol
- src/Facets/GenericSwapFacet.sol
🧰 Additional context used
🧠 Learnings (1)
script/tasks/diamondSyncWhitelistedAddresses.sh (2)
Learnt from: mirooon
PR: lifinance/contracts#1193
File: script/tasks/diamondSyncWhitelistedAddresses.sh:97-97
Timestamp: 2025-06-06T16:10:04.646Z
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.
Learnt from: mirooon
PR: lifinance/contracts#1193
File: script/tasks/diamondSyncWhitelistedAddresses.sh:128-128
Timestamp: 2025-06-06T16:09:58.662Z
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.
🧬 Code Graph Analysis (1)
script/tasks/diamondSyncWhitelistedAddresses.sh (1)
script/helperFunctions.sh (1)
getPrivateKey(2869-2900)
🪛 Shellcheck (0.10.0)
script/tasks/diamondSyncWhitelistedAddresses.sh
[warning] 97-97: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 128-128: Quote this to prevent word splitting.
(SC2046)
[warning] 133-133: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: generate-tag
🔇 Additional comments (10)
script/tasks/diamondSyncWhitelistedAddresses.sh (10)
3-5: Rename function and update logging. The function signature and initial log messages now correctly reflect the whitelisting purpose and align with naming conventions.
100-102: Error logging is clear. Failure to fetch on-chain data is correctly captured inFAILED_LOG_FILEwith context.
117-117: Appending new addresses. The array append syntax is correct and preserves each address as a separate element.
122-126: Parameter construction and retry init. BuildingPARAMSand initializing the attempt counter (ATTEMPTS) are implemented correctly.
135-139: Verification error handling is solid. The script clearly reports verification failures and logs them for post-mortem.
144-146: Missing-address check logic. The comparison and accumulation of unwhitelisted addresses function as intended.
151-153: Retry success logging. Success conditions are logged clearly, signaling when all addresses are whitelisted.
157-157: Retry loop increment. The attempt counter update properly drives the retry mechanism.
162-162: Detailed failure context. Emitting the attempted-address list enhances troubleshooting of partial failures.
207-208: Script completion log. The final log entry confirms the end of execution.
lifi-action-bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
script/helperFunctions.sh (2)
1871-1874:EXISTING_WHITELISTED_ADDRESSEScan benull→ arithmetic crashIf the network key is still absent,
jq --arg network "$NETWORK" '.[$network] | length'returns
null, so the later arithmetic withADD_COUNTERfails (null + n).
This is the exact issue flagged before and still unresolved.
1906-1913: Final validation always passes – same bug as previously reported
EXPECTED_WHITELISTED_ADDRESSESis computed and then compared against… the very
same expression, guaranteeing the check is never false:if [ $EXPECTED_WHITELISTED_ADDRESSES -eq $((EXISTING_WHITELISTED_ADDRESSES + ADD_COUNTER)) ]; thenIt should compare against the actual post-update count in the JSON file, e.g.:
-# check how many whitelisted addresses are in the whitelistedAddresses.json now -local ADDRESS_COUNTER=${#CONTRACTS[@]} +local ADDRESS_COUNTER +ADDRESS_COUNTER=$(jq --arg network "$NETWORK" '(.[$network] // []) | length' \ + "$FILEPATH_WHITELISTED_ADDRESSES") ... -if [ $EXPECTED_WHITELISTED_ADDRESSES -eq $((EXISTING_WHITELISTED_ADDRESSES + ADD_COUNTER)) ]; then +if [ "$EXPECTED_WHITELISTED_ADDRESSES" -eq "$ADDRESS_COUNTER" ]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
audit/auditLog.json(1 hunks)deployments/_deployments_log_file.json(28 hunks)deployments/arbitrum.diamond.staging.json(1 hunks)deployments/arbitrum.staging.json(1 hunks)deployments/base.staging.json(1 hunks)deployments/corn.diamond.staging.json(1 hunks)deployments/corn.staging.json(1 hunks)docs/README.md(1 hunks)package.json(1 hunks)script/demoScripts/demoPatcherDest.ts(0 hunks)script/demoScripts/utils/cowSwapHelpers.ts(2 hunks)script/helperFunctions.sh(3 hunks)
💤 Files with no reviewable changes (1)
- script/demoScripts/demoPatcherDest.ts
✅ Files skipped from review due to trivial changes (2)
- script/demoScripts/utils/cowSwapHelpers.ts
- deployments/corn.diamond.staging.json
🚧 Files skipped from review as they are similar to previous changes (6)
- audit/auditLog.json
- docs/README.md
- package.json
- deployments/arbitrum.diamond.staging.json
- deployments/arbitrum.staging.json
- deployments/_deployments_log_file.json
🧰 Additional context used
📓 Path-based instructions (1)
script/**/*.sh
📄 CodeRabbit Inference Engine (conventions.md)
script/**/*.sh: Bash scripts must begin with#!/bin/bash.
Bash scripts should organize code into modular functions with clear sections (e.g., Logging, Error handling, Deployment functions).
Bash scripts should load environment variables from.envorconfig.shand validate them early.
Use helper functions for logging (e.g.,echoDebug,error,warning,success) in Bash scripts.
Check function exit status withcheckFailurein Bash scripts.
Useset -efor error handling where appropriate in Bash scripts.
Files:
script/helperFunctions.sh
🧠 Learnings (48)
📓 Common learnings
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.
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.
Learnt from: mirooon
PR: lifinance/contracts#1187
File: .github/workflows/versionControlAndAuditCheck.yml:596-612
Timestamp: 2025-06-05T11:25:43.443Z
Learning: The user mirooon has an internal system for tracking tickets and technical debt. They acknowledged the security concern about disabled commit hash verification in the audit workflow and plan to implement conditional logic to skip verification only for revert PRs in a future update.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:49-49
Timestamp: 2024-11-25T09:05:03.917Z
Learning: The `RelayFacet` contract, when missing from the source code but referenced in deployment configurations, should be treated the same way as `OpBNBBridgeFacet` and can be ignored in code reviews.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1168
File: script/deploy/_targetState.json:1564-1589
Timestamp: 2025-05-27T12:36:26.987Z
Learning: When reviewing deployment PRs in the lifinance/contracts repository, target state configuration files (like script/deploy/_targetState.json) may be updated for multiple networks even when the PR is focused on deploying to a specific network. The scope should be determined by the PR title and description, not just by all configuration changes present in the files.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1237
File: conventions.md:56-60
Timestamp: 2025-07-03T01:44:43.968Z
Learning: Always consult the conventions.md file for the latest rules and conventions when reviewing PRs or code changes in the lifinance/contracts repository. Make suggestions when code changes do not match the documented conventions in this file.
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.
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.
Learnt from: mirooon
PR: lifinance/contracts#1048
File: script/deploy/safe/safe-decode-utils.ts:0-0
Timestamp: 2025-03-21T13:48:54.006Z
Learning: When reviewing PRs with a specific focused purpose (like migrating between package managers), restrict suggestions to issues directly related to that purpose rather than general code improvements.
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.
Learnt from: mirooon
PR: lifinance/contracts#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
PR: lifinance/contracts#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.
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts must reside in `src/Facets/` and names must include "Facet".
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
Learnt from: ezynda3
PR: lifinance/contracts#937
File: config/dexs.json:95-99
Timestamp: 2025-01-21T11:04:46.116Z
Learning: In the dexs.json configuration, utility contracts (like FeeCollector, LiFuelFeeCollector, TokenWrapper) can be treated as DEX contracts since they are handled similarly in the system's logic.
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/base.diamond.json:123-123
Timestamp: 2024-10-04T09:01:56.514Z
Learning: In the `lifinance/contracts` repository, it's acceptable to retain references to the old `LiFuelFeeCollector` address (`0xc4f7A34b8d283f66925eF0f5CCdFC2AF3030DeaE`) in deployment files when updating them is not necessary.
📚 Learning: for deployment prs involving address updates like the relayfacet to worldchain, verify the actual pr...
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/base.staging.jsondeployments/corn.staging.jsonscript/helperFunctions.sh
📚 Learning: in files with "staging" in their filename, such as `deployments/bsc.diamond.staging.json`, it is acc...
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/base.staging.jsondeployments/corn.staging.json
📚 Learning: for deployment prs updating contract addresses (like relayfacet on worldchain), verify the presence ...
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/base.staging.jsondeployments/corn.staging.jsonscript/helperFunctions.sh
📚 Learning: in deployment json files that contain "diamond" in their filename (located in the deployments folder...
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/base.staging.jsondeployments/corn.staging.jsonscript/helperFunctions.sh
📚 Learning: in the `deployments/*.diamond.json` and `deployments/*.json` files, the `lifidexaggregator` contract...
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/base.staging.jsondeployments/corn.staging.jsonscript/helperFunctions.sh
📚 Learning: in deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contr...
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/base.staging.jsondeployments/corn.staging.json
📚 Learning: when analyzing deployment prs in the lifinance/contracts repository, carefully verify that target st...
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/base.staging.jsonscript/helperFunctions.sh
📚 Learning: the bsca network intentionally maintains different facet versions between staging and production env...
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/base.staging.jsondeployments/corn.staging.json
📚 Learning: in the lifinance/contracts repository, deployment environments are distinguished by filename pattern...
Learnt from: 0xDEnYO
PR: lifinance/contracts#1123
File: deployments/polygon.diamond.json:124-127
Timestamp: 2025-04-28T10:33:48.525Z
Learning: In the lifinance/contracts repository, deployment environments are distinguished by filename patterns. Production deployments use standard names like "polygon.diamond.json", while staging deployments use names with "staging" suffix like "polygon.diamond.staging.json". When checking deployment logs for consistency, ensure to look in the correct environment section (production/staging) based on the filename pattern.
Applied to files:
deployments/base.staging.json
📚 Learning: applies to src/facets/*facet.sol : facet contracts must reside in `src/facets/` and names must inclu...
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts must reside in `src/Facets/` and names must include "Facet".
Applied to files:
deployments/base.staging.jsondeployments/corn.staging.json
📚 Learning: in `deployments/cronos.diamond.json`, both `genericswapfacet` and `genericswapfacetv3` are distinct ...
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/base.staging.jsondeployments/corn.staging.json
📚 Learning: the owner address `0x11f11121df7256c40339393b0fb045321022ce44` and the `diamondcutfacet` address `0x...
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/base.staging.jsondeployments/corn.staging.json
📚 Learning: lifidiamond contains generic withdrawal logic, so individual facet contracts (e.g., pioneerfacet) do...
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:
deployments/base.staging.jsondeployments/corn.staging.json
📚 Learning: in the lifi contract architecture, "lifidiamond" is the diamond proxy contract that connects all fac...
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/base.staging.jsondeployments/corn.staging.json
📚 Learning: version inconsistencies of 'genericswapfacetv3' across deployment configurations are acceptable if t...
Learnt from: 0xDEnYO
PR: lifinance/contracts#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:
deployments/base.staging.jsondeployments/corn.staging.json
📚 Learning: in `deployments/polygon.diamond.json`, it's acceptable for certain facets to have empty names and ve...
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/base.staging.jsondeployments/corn.staging.json
📚 Learning: the chainflipdata struct in chainflipfacet has three fields: 1. uint32 dsttoken - the destination to...
Learnt from: ezynda3
PR: lifinance/contracts#984
File: docs/ChainflipFacet.md:0-0
Timestamp: 2025-02-13T08:57:00.095Z
Learning: The ChainflipData struct in ChainflipFacet has three fields:
1. uint32 dstToken - The destination token identifier in Chainflip
2. bytes32 nonEvmAddress - The non-EVM destination address
3. bytes cfParameters - Additional parameters for Chainflip protocol
Applied to files:
deployments/base.staging.jsondeployments/corn.staging.json
📚 Learning: in `debridgedlnfacet.sol`, direct access to the `debridgechainid` mapping is acceptable, and using t...
Learnt from: ezynda3
PR: lifinance/contracts#827
File: src/Facets/DeBridgeDlnFacet.sol:0-0
Timestamp: 2024-11-22T08:49:35.205Z
Learning: In `DeBridgeDlnFacet.sol`, direct access to the `deBridgeChainId` mapping is acceptable, and using the `getDeBridgeChainId` function is not necessary in these instances.
Applied to files:
deployments/base.staging.json
📚 Learning: in `deployments/*.diamond.json` configuration files, facets that are part of an open pr and not yet ...
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/corn.staging.json
📚 Learning: when setting up new network deployments, it's expected that contract addresses in the deployment jso...
Learnt from: mirooon
PR: lifinance/contracts#985
File: deployments/bsca.json:0-0
Timestamp: 2025-02-12T09:44:10.963Z
Learning: When setting up new network deployments, it's expected that contract addresses in the deployment JSON files (e.g., bsca.json, bscb.json) may not have deployed code initially, as these files serve as target configurations for future deployments.
Applied to files:
deployments/corn.staging.jsonscript/helperFunctions.sh
📚 Learning: in `deployments/*.diamond.json` files, it's acceptable for facets to have empty `name` and `version`...
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/corn.staging.json
📚 Learning: task scripts in script/tasks/ directory (like diamondsyncwhitelistedaddresses.sh) define functions t...
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/helperFunctions.sh
📚 Learning: in the 'worldchain' network, the addresses `0x50d5a8acfae13dceb217e9a071f6c6bd5bdb4155`, `0x8f023b41...
Learnt from: ezynda3
PR: lifinance/contracts#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/helperFunctions.sh
📚 Learning: in `config/dexs.json`, it's acceptable for a whitelisted dex address to be the same across multiple ...
Learnt from: 0xDEnYO
PR: lifinance/contracts#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/helperFunctions.sh
📚 Learning: in script/tasks/diamondsyncwhitelistedaddresses.sh (lines 97 and 133), there are shell array assignm...
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: in `script/demoscripts/demopermit2.ts`, the `nextnonce` function should be called on the `permit2_pr...
Learnt from: maxklenk
PR: lifinance/contracts#782
File: script/demoScripts/demoPermit2.ts:0-0
Timestamp: 2024-11-05T17:16:19.946Z
Learning: In `script/demoScripts/demoPermit2.ts`, the `nextNonce` function should be called on the `PERMIT2_PROXY_ADDRESS` because the nonce is stored in the proxy contract, not in the `Permit2` contract (`PERMIT2_ADDRESS`).
Applied to files:
script/helperFunctions.sh
📚 Learning: in the dexs.json configuration, utility contracts (like feecollector, lifuelfeecollector, tokenwrapp...
Learnt from: ezynda3
PR: lifinance/contracts#937
File: config/dexs.json:95-99
Timestamp: 2025-01-21T11:04:46.116Z
Learning: In the dexs.json configuration, utility contracts (like FeeCollector, LiFuelFeeCollector, TokenWrapper) can be treated as DEX contracts since they are handled similarly in the system's logic.
Applied to files:
script/helperFunctions.sh
📚 Learning: in script/tasks/diamondsyncwhitelistedaddresses.sh at line 128, there is an unquoted command substit...
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.sh
📚 Learning: in the script `script/deploy/safe/add-owners-to-safe.ts`, additional defensive checks for network co...
Learnt from: ezynda3
PR: lifinance/contracts#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/helperFunctions.sh
📚 Learning: in `script/deploy/safe/add-owners-to-safe.ts`, validation for network configuration when casting imp...
Learnt from: ezynda3
PR: lifinance/contracts#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/helperFunctions.sh
📚 Learning: in `script/deploy/facets/deployrelayfacet.s.sol`, additional validation for the configuration file e...
Learnt from: ezynda3
PR: lifinance/contracts#843
File: script/deploy/facets/DeployRelayFacet.s.sol:0-0
Timestamp: 2024-10-31T09:07:36.301Z
Learning: In `script/deploy/facets/DeployRelayFacet.s.sol`, additional validation for the configuration file existence and address validity is unnecessary because the code will already fail appropriately if the configuration file is missing or the addresses are invalid.
Applied to files:
script/helperFunctions.sh
📚 Learning: in script/deploy/deployallcontracts.sh, the string comparison for balance checking (`"$balance" == "...
Learnt from: 0xDEnYO
PR: lifinance/contracts#1232
File: script/deploy/deployAllContracts.sh:0-0
Timestamp: 2025-06-24T07:47:41.478Z
Learning: In script/deploy/deployAllContracts.sh, the string comparison for BALANCE checking (`"$BALANCE" == "0"`) works fine as-is according to the maintainer and doesn't need to be changed to numeric comparison.
Applied to files:
script/helperFunctions.sh
📚 Learning: in `deployments/_deployments_log_file.json`, duplicate deployment entries for the same version and a...
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:
script/helperFunctions.sh
📚 Learning: when reviewing shell scripts in `.github/workflows/protectauditlabels.yml`, ensure that added `fi` s...
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: .github/workflows/protectAuditLabels.yml:95-95
Timestamp: 2024-11-26T01:46:18.625Z
Learning: When reviewing shell scripts in `.github/workflows/protectAuditLabels.yml`, ensure that added `fi` statements properly close existing `if` blocks, recognizing that such additions in the diff may be fixing previous syntax errors rather than introducing new ones.
Applied to files:
script/helperFunctions.sh
📚 Learning: ensure that scripts used for codebase verification produce valid and accurate results before reporti...
Learnt from: 0xDEnYO
PR: lifinance/contracts#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:
script/helperFunctions.sh
📚 Learning: do not suggest using `mapfile` or `read -a` for array assignments in the lifinance/contracts reposit...
Learnt from: 0xDEnYO
PR: lifinance/contracts#1102
File: script/tasks/diamondSyncSigs_FAST.sh:112-117
Timestamp: 2025-04-17T02:40:15.827Z
Learning: Do not suggest using `mapfile` or `read -a` for array assignments in the lifinance/contracts repository as these features may not be available in the user's environment. The scripts are designed with the knowledge that network names will only ever consist of one word, making word splitting concerns less relevant.
Applied to files:
script/helperFunctions.sh
📚 Learning: in script/tasks/diamondsyncselectors.sh (line 112), there is a shell array assignment using command ...
Learnt from: mirooon
PR: lifinance/contracts#1193
File: script/tasks/diamondSyncSelectors.sh:112-117
Timestamp: 2025-06-15T11:05:21.990Z
Learning: In script/tasks/diamondSyncSelectors.sh (line 112), there is a shell array assignment using command substitution that should be fixed using mapfile instead: `CFG_SELECTORS=($(jq -r '.[] | @sh' "./config/whitelistedSelectors.json" | tr -d \' | tr '[:upper:]' '[:lower:]'))` should be `mapfile -t CFG_SELECTORS < <(jq -r '.[]' "./config/whitelistedSelectors.json" | tr '[:upper:]' '[:lower:]')`. User mirooon requested to be reminded of this issue in future PRs when this file is modified.
Applied to files:
script/helperFunctions.sh
📚 Learning: in the `script/deploy/safe/add-owners-to-safe.ts` script, explicit error handling for missing networ...
Learnt from: ezynda3
PR: lifinance/contracts#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/helperFunctions.sh
📚 Learning: in `script/utils/diamondemergencypausegithub.sh`, temporary hardcoded network lists may be included ...
Learnt from: 0xDEnYO
PR: lifinance/contracts#715
File: script/utils/diamondEMERGENCYPauseGitHub.sh:127-127
Timestamp: 2024-10-08T05:15:07.688Z
Learning: In `script/utils/diamondEMERGENCYPauseGitHub.sh`, temporary hardcoded network lists may be included during testing in pull requests, with the plan to remove them before merging into the main branch.
Applied to files:
script/helperFunctions.sh
📚 Learning: in script/helperfunctions.sh, 0xdenyo prefers to keep the existing file locking implementation (usin...
Learnt from: 0xDEnYO
PR: lifinance/contracts#1140
File: script/helperFunctions.sh:110-133
Timestamp: 2025-07-15T03:34:58.855Z
Learning: In script/helperFunctions.sh, 0xDEnYO prefers to keep the existing file locking implementation (using EXIT trap) unchanged even when potential improvements are suggested, prioritizing working code stability over refactoring when the current solution is functional.
Applied to files:
script/helperFunctions.sh
📚 Learning: in script/helperfunctions.sh, the user 0xdenyo prefers to keep the existing bash test condition usin...
Learnt from: 0xDEnYO
PR: lifinance/contracts#1196
File: script/helperFunctions.sh:1483-1494
Timestamp: 2025-06-19T07:47:42.662Z
Learning: In script/helperFunctions.sh, the user 0xDEnYO prefers to keep the existing bash test condition using `-a` operator (e.g., `[ $COMMAND_STATUS -ne 0 -a $RETRY_COUNT -lt $MAX_RETRIES ]`) rather than refactoring to separate tests with `&&` operators, indicating they prioritize keeping working code unchanged over style improvements.
Applied to files:
script/helperFunctions.sh
📚 Learning: in `config/dexs.json`, it's expected that some addresses appear multiple times across different netw...
Learnt from: 0xDEnYO
PR: lifinance/contracts#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/helperFunctions.sh
📚 Learning: in lifinance/contracts, the addresses stored in config/dexs.json are handled case-insensitively, so ...
Learnt from: 0xDEnYO
PR: lifinance/contracts#1201
File: config/dexs.json:536-537
Timestamp: 2025-06-11T08:55:44.396Z
Learning: In lifinance/contracts, the addresses stored in config/dexs.json are handled case-insensitively, so they do not need to be written in EIP-55 checksummed format.
Applied to files:
script/helperFunctions.sh
📚 Learning: in config/permit2proxy.json, the addresses represent the permit2 contract addresses on each chain, n...
Learnt from: 0xDEnYO
PR: lifinance/contracts#1140
File: config/permit2Proxy.json:3-3
Timestamp: 2025-07-15T05:05:28.134Z
Learning: In config/permit2Proxy.json, the addresses represent the Permit2 contract addresses on each chain, NOT the Permit2Proxy contract addresses. These Permit2 addresses are used as constructor arguments when deploying the Permit2Proxy contract. The Permit2Proxy acts as a wrapper/proxy that interacts with the underlying Permit2 contract on each network.
Applied to files:
script/helperFunctions.sh
📚 Learning: in the `lifinance/contracts` repository, contracts may have different addresses across networks. do ...
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
Applied to files:
script/helperFunctions.sh
📚 Learning: when reviewing pull requests, only point out missing contract addresses in `deployments/.di...
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:
script/helperFunctions.sh
📚 Learning: in the lifinance/contracts repository, it's acceptable that not every contract is deployed on all ne...
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/moonriver.diamond.json:68-68
Timestamp: 2024-10-04T09:08:42.225Z
Learning: In the lifinance/contracts repository, it's acceptable that not every contract is deployed on all networks, so empty addresses for contracts not deployed on a network are acceptable in deployment files.
Applied to files:
script/helperFunctions.sh
🪛 Biome (2.1.2)
deployments/base.staging.json
[error] 4-4: The key LiFiDEXAggregator was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🪛 Gitleaks (8.27.2)
deployments/corn.staging.json
2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Shellcheck (0.10.0)
script/helperFunctions.sh
[warning] 1872-1872: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1888-1888: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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). (1)
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (1)
deployments/corn.staging.json (1)
1-15: WhitelistManagerFacet missing from the deployment mapEvery other network/config updated in this PR includes a WhitelistManagerFacet entry. Omitting it here breaks functional parity and will make selector-whitelisting calls fail at runtime on corn-staging.
After deploying the facet, add:
"WhitelistManagerFacet": "0x…"and verify the facet’s selectors are cut into LiFiDiamond during the upgrade script.
lifi-action-bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
script/helperFunctions.sh (2)
1871-1873:jqnull-handling and SC2155 issue still unfixedPrevious review already pointed out that when the network key is absent,
jq '.[$network] | length'returnsnull, breaking the arithmetic below and
potentially masking the command’s exit status (SC2155).
Please apply the earlier fix instead of re-introducing the same bug.- local EXISTING_WHITELISTED_ADDRESSES=$(jq --arg network "$NETWORK" '.[$network] | length' "$FILEPATH_WHITELISTED_ADDRESSES") + local EXISTING_WHITELISTED_ADDRESSES + EXISTING_WHITELISTED_ADDRESSES=$(jq --arg network "$NETWORK" \ + '(.[$network] // []) | length' "$FILEPATH_WHITELISTED_ADDRESSES")
1906-1913: Final validation logic is still tautological—always passesYou compare the expected count to the same expression used to compute
EXPECTED_WHITELISTED_ADDRESSES, so the check can never fail.
Re-compute the post-update length from the JSON file and compare against the
expected value.-# check how many whitelisted addresses are in the whitelistedAddresses.json now -local ADDRESS_COUNTER=${#CONTRACTS[@]} - -EXPECTED_WHITELISTED_ADDRESSES=$((EXISTING_WHITELISTED_ADDRESSES + ADD_COUNTER)) - -# make sure whitelistedAddresses.json has been updated correctly -if [ $EXPECTED_WHITELISTED_ADDRESSES -eq $((EXISTING_WHITELISTED_ADDRESSES + ADD_COUNTER)) ]; then +# re-count addresses after the update +local ADDRESS_COUNTER +ADDRESS_COUNTER=$(jq --arg network "$NETWORK" \ + '(.[$network] // []) | length' "$FILEPATH_WHITELISTED_ADDRESSES") + +EXPECTED_WHITELISTED_ADDRESSES=$((EXISTING_WHITELISTED_ADDRESSES + ADD_COUNTER)) + +# final sanity check +if [ "$ADDRESS_COUNTER" -eq "$EXPECTED_WHITELISTED_ADDRESSES" ]; then
🧹 Nitpick comments (2)
conventions.md (1)
392-410: Example snippet compiles-with-errors as writtenThe illustrative contract fails rudimentary compilation:
uniswapis undeclared – use an interface constant or placeholder such asIUniswapV2Router(uniswapRouter).swapExactTokensForTokens.selector.ADDRESS_UNISWAPis undefined in the snippet; either add its declaration or leave a TODO comment.- A blank line sneaks in before
contract FacetTest, causing linters (e.g. MD046) to complain about fenced-code termination spacing.Recommend tightening the snippet to something self-contained:
contract TestFacet is TestWhitelistManagerBase {} contract FacetTest is TestBase { TestFacet internal facet; address constant UNISWAP_ROUTER = 0x...; // sample address function setUp() public { initTestBase(); facet = new TestFacet(); // Whitelist address and selector facet.addToWhitelist(UNISWAP_ROUTER); facet.setFunctionWhitelistBySelector( IUniswapV2Router(UNISWAP_ROUTER).swapExactTokensForTokens.selector ); } }This guards against copy-paste errors and passes both Solidity and markdown-lint checks.
script/helperFunctions.sh (1)
1887-1889: Minor: avoid masking return statusDeclare
EXISTSfirst, then assign; this removes ShellCheck SC2155 and makes
error handling more reliable.- local EXISTS=$(jq --arg address "$CONTRACT_ADDRESS" --arg network "$NETWORK" '(.[$network] // []) | any(. == $address)' $FILEPATH_WHITELISTED_ADDRESSES) + local EXISTS + EXISTS=$(jq --arg address "$CONTRACT_ADDRESS" --arg network "$NETWORK" \ + '(.[$network] // []) | any(. == $address)' "$FILEPATH_WHITELISTED_ADDRESSES")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
conventions.md(1 hunks)deployments/_deployments_log_file.json(35 hunks)deployments/arbitrum.diamond.staging.json(1 hunks)deployments/arbitrum.staging.json(1 hunks)script/helperFunctions.sh(3 hunks)src/Libraries/LibAllowList.sol(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- deployments/arbitrum.staging.json
- deployments/arbitrum.diamond.staging.json
- deployments/_deployments_log_file.json
🧰 Additional context used
📓 Path-based instructions (3)
{src/**/*.sol,test/solidity/**/*.sol,script/**/*.sol}
📄 CodeRabbit Inference Engine (conventions.md)
All our own Solidity files must use the LGPL-3.0 license identifier. This applies to all contracts in
src/, all test files intest/solidity/, and all deployment and task scripts inscript/.
Files:
src/Libraries/LibAllowList.sol
**/*.sol
📄 CodeRabbit Inference Engine (conventions.md)
**/*.sol: Use custom error types rather than genericrevert()statements.
Do not define interfaces in the same file as their implementation.
Every contract MUST have the following NatSpec tags in this order: @title, @author, @notice, @Custom:version.
The @author tag MUST always be "LI.FI (https://li.fi)" in contract NatSpec.
The @Custom:version MUST follow semantic versioning (X.Y.Z format) in contract NatSpec.
Every public/external function must have NatSpec comments describing the function, parameters, and return values.
All Solidity files must follow the rules defined in.solhint.json.
All Solidity files must use the EVM and Solidity version defined infoundry.tomlunless networks require lower versions.
Files:
src/Libraries/LibAllowList.sol
script/**/*.sh
📄 CodeRabbit Inference Engine (conventions.md)
script/**/*.sh: Bash scripts must begin with#!/bin/bash.
Bash scripts should organize code into modular functions with clear sections (e.g., Logging, Error handling, Deployment functions).
Bash scripts should load environment variables from.envorconfig.shand validate them early.
Use helper functions for logging (e.g.,echoDebug,error,warning,success) in Bash scripts.
Check function exit status withcheckFailurein Bash scripts.
Useset -efor error handling where appropriate in Bash scripts.
Files:
script/helperFunctions.sh
🪛 markdownlint-cli2 (0.17.2)
conventions.md
392-392: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🪛 Shellcheck (0.10.0)
script/helperFunctions.sh
[warning] 1872-1872: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 1888-1888: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (10)
conventions.md (1)
382-391: Function-naming inconsistency may confuse future contributorsThe bullet list mixes “whitelist” and “approval” terminology:
- setFunctionWhitelistBySelector(bytes4) - removeFunctionApprovalBySelector(bytes4)If the canonical API really is
removeFunctionApprovalBySelector, consider adding a short note that it’s intentionally asymmetric; otherwise, rename the doc entry toremoveFunctionWhitelistBySelectorfor symmetry.src/Libraries/LibAllowList.sol (9)
4-5: LGTM! Good addition of required imports.The imports align with the enhanced validation logic and address the past feedback about using custom error types.
7-10: NatSpec and version look good.The NatSpec documentation follows the required format with all mandatory tags. Version 2.0.0 appropriately reflects the breaking storage layout changes as discussed in previous comments.
Note: The audit log entry for v2.0.0 will still need to be added once the audit is completed.
17-24: Excellent storage structure improvements.The enhanced storage design with arrays and index mappings enables efficient O(1) removal operations while maintaining the boolean mappings for quick lookups. The 1-based indexing approach is a smart way to distinguish between "not found" (0) and valid indices.
28-44: Well-implemented validation and indexing logic.The function properly addresses previous feedback by using
InvalidCallDatafor invalid inputs and includes comprehensive validation. The duplicate checking and 1-based indexing implementation is correct and efficient.
48-52: LGTM! Correct mapping reference update.The function correctly uses the renamed
contractAllowListmapping.
56-84: Excellent implementation of efficient removal logic.The swap-and-pop removal pattern is correctly implemented with proper index management. The function handles all edge cases appropriately and maintains data consistency across mappings and arrays.
93-103: Consistent and correct selector management implementation.The function mirrors the solid pattern used in
addAllowedContractwith appropriate adaptations for selector management. The duplicate checking and indexing logic is correct.
107-135: Correct implementation of selector removal logic.The function properly implements the swap-and-pop pattern for efficient selector removal, maintaining consistency with the contract removal implementation.
144-146: Useful addition of selector getter function.The
getAllowedSelectorsfunction provides symmetry withgetAllowedContractsand enables external access to the complete selector allow list.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deployments/optimism.diamond.staging.json (1)
172-172: Patcher periphery placeholder – remember to fill once deployed
"Patcher": ""was introduced underPeriphery. Leaving it empty in staging is fine, but ensure the address is populated once the contract is actually deployed; some scripts rely on this field to skip/execute patch calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deployments/_deployments_log_file.json(47 hunks)deployments/optimism.diamond.staging.json(2 hunks)deployments/optimism.staging.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deployments/_deployments_log_file.json
🧰 Additional context used
🧠 Learnings (29)
📓 Common learnings
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.
Learnt from: mirooon
PR: lifinance/contracts#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.
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.
Learnt from: 0xDEnYO
PR: lifinance/contracts#782
File: script/deploy/_targetState.json:49-49
Timestamp: 2024-11-25T09:05:03.917Z
Learning: The `RelayFacet` contract, when missing from the source code but referenced in deployment configurations, should be treated the same way as `OpBNBBridgeFacet` and can be ignored in code reviews.
Learnt from: mirooon
PR: lifinance/contracts#1187
File: .github/workflows/versionControlAndAuditCheck.yml:596-612
Timestamp: 2025-06-05T11:25:43.443Z
Learning: The user mirooon has an internal system for tracking tickets and technical debt. They acknowledged the security concern about disabled commit hash verification in the audit workflow and plan to implement conditional logic to skip verification only for revert PRs in a future update.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1168
File: script/deploy/_targetState.json:1564-1589
Timestamp: 2025-05-27T12:36:26.987Z
Learning: When reviewing deployment PRs in the lifinance/contracts repository, target state configuration files (like script/deploy/_targetState.json) may be updated for multiple networks even when the PR is focused on deploying to a specific network. The scope should be determined by the PR title and description, not just by all configuration changes present in the files.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1237
File: conventions.md:56-60
Timestamp: 2025-07-03T01:44:43.968Z
Learning: Always consult the conventions.md file for the latest rules and conventions when reviewing PRs or code changes in the lifinance/contracts repository. Make suggestions when code changes do not match the documented conventions in this file.
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.
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.
Learnt from: mirooon
PR: lifinance/contracts#1048
File: script/deploy/safe/safe-decode-utils.ts:0-0
Timestamp: 2025-03-21T13:48:54.006Z
Learning: When reviewing PRs with a specific focused purpose (like migrating between package managers), restrict suggestions to issues directly related to that purpose rather than general code improvements.
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
Learnt from: mirooon
PR: lifinance/contracts#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: ezynda3
PR: lifinance/contracts#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`.
Learnt from: ezynda3
PR: lifinance/contracts#937
File: config/dexs.json:95-99
Timestamp: 2025-01-21T11:04:46.116Z
Learning: In the dexs.json configuration, utility contracts (like FeeCollector, LiFuelFeeCollector, TokenWrapper) can be treated as DEX contracts since they are handled similarly in the system's logic.
Learnt from: 0xDEnYO
PR: lifinance/contracts#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.
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.
📚 Learning: the user mirooon prefers to maintain both dexmanagerfacet and whitelistmanagerfacet entries in stagi...
Learnt from: mirooon
PR: lifinance/contracts#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:
deployments/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: for deployment prs updating contract addresses (like relayfacet on worldchain), verify the presence ...
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/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: in files with "staging" in their filename, such as `deployments/bsc.diamond.staging.json`, it is acc...
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/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: for deployment prs involving address updates like the relayfacet to worldchain, verify the actual pr...
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/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: in `deployments/bsc.diamond.staging.json`, the trailing comma after the last property in the `periph...
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: deployments/bsc.diamond.staging.json:100-101
Timestamp: 2024-10-24T06:17:25.211Z
Learning: In `deployments/bsc.diamond.staging.json`, the trailing comma after the last property in the `Periphery` object is acceptable and should not be flagged as a JSON formatting error.
Applied to files:
deployments/optimism.staging.json
📚 Learning: in `deployments/polygon.diamond.json`, it's acceptable for certain facets to have empty names and ve...
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/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: in deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contr...
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/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: hyphenfacet v1.0.0 is intentionally included in the bsc staging environment and should not be remove...
Learnt from: ezynda3
PR: lifinance/contracts#807
File: script/deploy/_targetState.json:181-181
Timestamp: 2024-12-03T11:02:14.195Z
Learning: HyphenFacet v1.0.0 is intentionally included in the BSC staging environment and should not be removed even if not present in production environments.
Applied to files:
deployments/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: the bsca network intentionally maintains different facet versions between staging and production env...
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/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: in `deployments/cronos.diamond.json`, both `genericswapfacet` and `genericswapfacetv3` are distinct ...
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/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: applies to src/facets/*facet.sol : facet contracts must reside in `src/facets/` and names must inclu...
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-07-28T07:54:14.853Z
Learning: Applies to src/Facets/*Facet.sol : Facet contracts must reside in `src/Facets/` and names must include "Facet".
Applied to files:
deployments/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: lifidiamond contains generic withdrawal logic, so individual facet contracts (e.g., pioneerfacet) do...
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:
deployments/optimism.staging.json
📚 Learning: the cbridgefacetpacked and cbridge addresses in addtokenapprovalstocbridgefacetpacked.s.sol must not...
Learnt from: 0xDEnYO
PR: lifinance/contracts#988
File: script/tasks/solidity/AddTokenApprovalsToCBridgeFacetPacked.s.sol:17-21
Timestamp: 2025-02-11T10:35:03.536Z
Learning: The CBridgeFacetPacked and cBridge addresses in AddTokenApprovalsToCBridgeFacetPacked.s.sol must not be zero addresses as they are required for token approvals to function properly.
Applied to files:
deployments/optimism.staging.json
📚 Learning: the owner address `0x11f11121df7256c40339393b0fb045321022ce44` and the `diamondcutfacet` address `0x...
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/optimism.staging.json
📚 Learning: version inconsistencies of 'genericswapfacetv3' across deployment configurations are acceptable if t...
Learnt from: 0xDEnYO
PR: lifinance/contracts#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:
deployments/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: in `gaszipperiphery.sol`, `libutil` and `validatable` are used, so ensure not to suggest their remov...
Learnt from: 0xDEnYO
PR: lifinance/contracts#807
File: src/Periphery/GasZipPeriphery.sol:4-14
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In `GasZipPeriphery.sol`, `LibUtil` and `Validatable` are used, so ensure not to suggest their removal in future reviews.
Applied to files:
deployments/optimism.staging.jsondeployments/optimism.diamond.staging.json
📚 Learning: in config/permit2proxy.json, the addresses represent the permit2 contract addresses on each chain, n...
Learnt from: 0xDEnYO
PR: lifinance/contracts#1140
File: config/permit2Proxy.json:3-3
Timestamp: 2025-07-15T05:05:28.134Z
Learning: In config/permit2Proxy.json, the addresses represent the Permit2 contract addresses on each chain, NOT the Permit2Proxy contract addresses. These Permit2 addresses are used as constructor arguments when deploying the Permit2Proxy contract. The Permit2Proxy acts as a wrapper/proxy that interacts with the underlying Permit2 contract on each network.
Applied to files:
deployments/optimism.staging.json
📚 Learning: in `deployments/*.diamond.json` configuration files, facets that are part of an open pr and not yet ...
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/optimism.diamond.staging.json
📚 Learning: when facet versions are set to empty strings in `deployments/*.diamond.json` files, do not report th...
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:
deployments/optimism.diamond.staging.json
📚 Learning: in `deployments/*.diamond.json` files, it's acceptable for facets to have empty `name` and `version`...
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/optimism.diamond.staging.json
📚 Learning: in deployment json files that contain "diamond" in their filename (located in the deployments folder...
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/optimism.diamond.staging.json
📚 Learning: gaszipperiphery deployment depends on having a safeaddress available in the network configuration. i...
Learnt from: 0xDEnYO
PR: lifinance/contracts#1171
File: config/gaszip.json:27-27
Timestamp: 2025-05-29T09:40:46.846Z
Learning: GasZipPeriphery deployment depends on having a safeAddress available in the network configuration. If safeAddress is empty, GasZipPeriphery deployment should be deferred until the Safe multisig address is set up for the network.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: the patcher contract in src/periphery/patcher.sol is designed to be used in conjunction with an api ...
Learnt from: ezynda3
PR: lifinance/contracts#1124
File: src/Periphery/Patcher.sol:76-82
Timestamp: 2025-06-05T14:31:33.404Z
Learning: The Patcher contract in src/Periphery/Patcher.sol is designed to be used in conjunction with an API and is never meant to hold user funds or user approvals. This architectural design makes the arbitrary delegatecall functionality less risky as the contract operates in a stateless/ephemeral manner.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: in src/periphery/patcher.sol, the unlimited token approval to finaltarget in depositandexecutewithdy...
Learnt from: ezynda3
PR: lifinance/contracts#1124
File: src/Periphery/Patcher.sol:160-167
Timestamp: 2025-06-05T14:48:58.816Z
Learning: In src/Periphery/Patcher.sol, the unlimited token approval to finalTarget in depositAndExecuteWithDynamicPatches and depositAndExecuteWithMultiplePatches functions is intentional behavior, as confirmed by ezynda3. This design choice aligns with the contract's stateless/ephemeral architecture.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: in the lifinance/contracts repository, it's normal and expected for periphery contracts to have empt...
Learnt from: 0xDEnYO
PR: lifinance/contracts#1112
File: deployments/soneium.diamond.json:81-81
Timestamp: 2025-04-22T09:04:44.244Z
Learning: In the lifinance/contracts repository, it's normal and expected for periphery contracts to have empty address values in deployment files when they are not deployed on a particular network. This applies to all periphery contracts including ReceiverChainflip, ReceiverStargateV2, and others. PRs should not flag empty periphery contract addresses as issues.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: contract address validations for periphery contracts like receiverchainflip are handled in deploymen...
Learnt from: ezynda3
PR: lifinance/contracts#984
File: src/Periphery/ReceiverChainflip.sol:48-62
Timestamp: 2025-02-24T09:35:34.908Z
Learning: Contract address validations for periphery contracts like ReceiverChainflip are handled in deployment scripts rather than in the constructor.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: in the patcher contract at src/periphery/patcher.sol, the `_getdynamicvalue` function intentionally ...
Learnt from: ezynda3
PR: lifinance/contracts#1124
File: src/Periphery/Patcher.sol:29-38
Timestamp: 2025-06-05T14:56:32.603Z
Learning: In the Patcher contract at src/Periphery/Patcher.sol, the `_getDynamicValue` function intentionally does not validate that return data is at least 32 bytes before using `mload(add(valueData, 32))`. This design choice is intentional and accepted by the team despite the potential for out-of-bounds memory access if external contracts return insufficient data.
Applied to files:
deployments/optimism.diamond.staging.json
📚 Learning: in the `lifinance/contracts` repository, `receiverstargatev2` may not be deployed to all networks, a...
Learnt from: 0xDEnYO
PR: lifinance/contracts#819
File: deployments/boba.diamond.json:68-68
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `lifinance/contracts` repository, `ReceiverStargateV2` may not be deployed to all networks, and it's acceptable for its address to remain empty in deployment files when it is not deployed. PRs unrelated to `ReceiverStargateV2` should not raise comments about its absence.
Applied to files:
deployments/optimism.diamond.staging.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (2)
deployments/optimism.diamond.staging.json (2)
148-159: Unidentified facet placeholders – please confirm intentThree new facet entries were added with empty
Name/Versionfields:0xc2A0D799744536C621Af9B2933CdB4Ad959980bF 0x8962d191Ba0f2Bc29b949ACA222f8251B241190b 0x36e1375B0755162d720276dFF6893DF02bd49225Keeping blanks is acceptable in staging configs per team convention, but before these addresses make it into production we should know which contracts they map to and what code version they run.
Please double-check that:
- Each address is indeed a deployed facet on Optimism-staging.
- The missing metadata is added (or the facet removed) when promoting the deployment file to non-staging environments.
160-163: WhitelistManagerFacet entry looks consistent
WhitelistManagerFacet(v1.0.0) has been added with address0x0f4D0D8890bAF9036D3895315559a6F0d573e2EC.
DexManagerFacet is intentionally kept for migration/rollback (per past guidance), so the coexistence of both facets in this staging file is acceptable.No further action needed here.
mirooon
changed the title
Which Jira task belongs to this PR?
LF-13736
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)