Deploy to Sonic #918
Deploy to Sonic #918
Conversation
WalkthroughThis pull request introduces configuration and deployment support for the new "sonic" network across multiple configuration files. The changes include adding network-specific details to various JSON configuration files, updating deployment logs, creating a new diamond deployment configuration, and modifying the Foundry configuration to support the sonic network. The modifications span across configuration management, network integration, and deployment preparation. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Coverage ReportLine Coverage: 78.66% (2238 / 2845 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: 2
🧹 Nitpick comments (1)
deployments/_deployments_log_file.json (1)
730-730: Empty SALT values may lead to non-deterministic deployments.All contracts are deployed with empty SALT values. Consider using deterministic SALTs for reproducible deployments.
Also applies to: 731-731, 732-732
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/diamondEmergencyPause.yml(1 hunks)config/amarok.json(2 hunks)config/dexs.json(2 hunks)config/gaszip.json(1 hunks)config/global.json(2 hunks)config/networks.json(2 hunks)config/stargate.json(2 hunks)config/tokenwrapper.json(1 hunks)deployments/_deployments_log_file.json(23 hunks)deployments/sonic.diamond.json(1 hunks)deployments/sonic.json(1 hunks)foundry.toml(2 hunks)networks(1 hunks)script/deploy/_targetState.json(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/diamondEmergencyPause.yml
- networks
🧰 Additional context used
🪛 Gitleaks (8.21.2)
deployments/sonic.diamond.json
73-73: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
deployments/sonic.json
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (20)
config/gaszip.json (1)
21-21: LGTM! Router address follows the established pattern.The added router address for the sonic network matches the standardized address used across other networks.
✅ Verification successful
Verified: Router address follows the established pattern
The added router address for "sonic" network (0x2a37D63EAdFe4b4682a3c28C1c2cD4F109Cc2762) matches the standardized address used across 17 other networks including mainnet, arbitrum, and avalanche.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the router address pattern across networks rg -U --multiline '{\s*"gasZipRouters".*"0x2a37D63EAdFe4b4682a3c28C1c2cD4F109Cc2762"' config/Length of output: 236
Script:
#!/bin/bash # First, let's see the content of gaszip.json cat config/gaszip.json # Then search for the specific router address across config files rg "0x2a37D63EAdFe4b4682a3c28C1c2cD4F109Cc2762" config/Length of output: 2857
config/tokenwrapper.json (1)
38-38: Verify automated generation of this entry.The file is marked as being generated by a backend script. Ensure this addition was made through the appropriate backend process rather than manual editing.
deployments/sonic.diamond.json (2)
61-73: Address empty periphery configurations.Several periphery entries are empty strings:
- Permit2Proxy
- ReceiverAcrossV3
- ReceiverStargateV2
- RelayerCelerIM
Please clarify if these empty configurations are intentional or need to be populated before deployment.
🧰 Tools
🪛 Gitleaks (8.21.2)
73-73: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
4-59: Verify facet versions against production standards.The facet versions vary across components. Ensure these versions align with the current production standards and have undergone necessary audits as required by PR objectives.
✅ Verification successful
✓ Facet versions verified and aligned with production standards
All facet versions in sonic.diamond.json are consistent with versions used across other deployments, representing the current production standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check facet versions across deployments find deployments -name "*.diamond.json" -exec jq -r '.LiFiDiamond.Facets[] | select(.Name != null) | "\(.Name): \(.Version)"' {} \; | sort | uniq -cLength of output: 2557
foundry.toml (1)
59-59: Ensure environment variables are configured.New environment variables have been introduced:
ETH_NODE_URI_SONICSONIC_ETHERSCAN_API_KEYThese must be properly configured in the deployment environment.
Also applies to: 105-105
config/global.json (2)
143-143: Address consistency verified.The native address for sonic matches the address in tokenwrapper.json, maintaining consistency across configurations.
Line range hint
143-149: Verify if safe configuration is needed.The sonic network has been added to
nativeAddressbut doesn't have corresponding entries in:
safeAddressessafeApiUrlsPlease confirm if safe configuration is required for the sonic network.
config/amarok.json (1)
273-278: Verify if Connext protocol support is planned for sonic chainThe connextHandler is set to the zero address (0x0000000000000000000000000000000000000000) which indicates Connext protocol is not supported. Empty domain and tokensToApprove suggest cross-chain messaging is not configured.
Run this script to check if Connext protocol support is planned:
✅ Verification successful
Connext protocol is not supported or planned for sonic chain
The zero address configuration for sonic chain follows the same pattern as other chains where Connext is not supported, with no evidence in configurations, code, or documentation suggesting planned support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any pending configurations or deployments for Connext on sonic chain # Test: Search for any references to Connext deployment on sonic chain rg -i "connext.*sonic|sonic.*connext" --type mdLength of output: 49
Script:
#!/bin/bash # Search for Connext configurations across all chains echo "=== Checking Connext configurations across chains ===" rg "connextHandler" -A 2 -B 2 echo -e "\n=== Checking for sonic chain configurations and TODOs ===" rg -i "sonic.*chain|chain.*sonic" -A 2 -B 2 echo -e "\n=== Looking for TODO comments about Connext or sonic ===" rg -i "todo.*(connext|sonic)" -A 2 -B 2Length of output: 82991
config/stargate.json (1)
243-243: Verify if Stargate protocol support is planned for sonic chainThe composer address is set to the zero address (0x0000000000000000000000000000000000000000) which indicates Stargate protocol is not supported.
Run this script to check if Stargate protocol support is planned:
✅ Verification successful
Stargate protocol is not supported or planned for sonic chain
Multiple indicators confirm this:
- Chain is marked as inactive in network configuration
- No Stargate contract deployments exist
- No Stargate facets included in target state
- Zero address configuration is intentional
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any pending configurations or deployments for Stargate on sonic chain # Test: Search for any references to Stargate deployment on sonic chain rg -i "stargate.*sonic|sonic.*stargate" --type mdLength of output: 51
Script:
#!/bin/bash # Search for sonic chain configuration and mentions rg -i "sonic" --type=json -A 5 -B 5 # Look for Stargate protocol configuration patterns rg -i "stargate" --type=json -A 5 -B 5 # Check README and documentation fd -e md -x cat {}Length of output: 67003
config/networks.json (1)
560-577: Verify infrastructure contract addresses for sonic chainThe configuration includes all required infrastructure addresses. However, since the network status is set to "inactive", verify that these addresses are deployed and operational:
- Multicall: 0xcA11bde05977b3631167028862bE2a173976CA11
- Safe: 0x37DC6C2C8db8B027E818592CBae6B5acdc1381BE
- Wrapped Native: 0x039e2fB66102314Ce7b64Ce5Ce3E5183bc94aD38
Run this script to verify the contract addresses:
✅ Verification successful
Infrastructure contracts verified and operational on sonic chain
All required infrastructure contracts are properly deployed and operational:
- Multicall3 contract at 0xcA11bde05977b3631167028862bE2a173976CA11
- Safe proxy contract at 0x37DC6C2C8db8B027E818592CBae6B5acdc1381BE
- Wrapped Native token at 0x039e2fB66102314Ce7b64Ce5Ce3E5183bc94aD38
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify infrastructure contract addresses on sonic chain # Test: Check if the addresses have contract code deployed # 1. Check if RPC is accessible curl -s -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"eth_blockNumber","params":[],"id":1}' https://rpc.soniclabs.com # 2. Check if explorer API is accessible curl -s "https://api.sonicscan.org/api?module=contract&action=getabi&address=0xcA11bde05977b3631167028862bE2a173976CA11" # 3. Check if Safe transaction service is accessible curl -s "https://safe-transaction-sonic.safe.global/api/v1/about/"Length of output: 1447
Script:
#!/bin/bash # Description: Check contract code existence via RPC # Test: Use eth_getCode to verify contract deployment for address in "0xcA11bde05977b3631167028862bE2a173976CA11" "0x37DC6C2C8db8B027E818592CBae6B5acdc1381BE" "0x039e2fB66102314Ce7b64Ce5Ce3E5183bc94aD38"; do echo "Checking $address..." curl -s -X POST -H "Content-Type: application/json" \ --data "{\"jsonrpc\":\"2.0\",\"method\":\"eth_getCode\",\"params\":[\"$address\", \"latest\"],\"id\":1}" \ https://rpc.soniclabs.com echo -e "\n" doneLength of output: 14819
script/deploy/_targetState.json (1)
1565-1592: Verify LiFiDEXAggregator version for sonic chainThe LiFiDEXAggregator version (1.5.0) differs from other chains which use version 1.0.0. Verify if this is intentional.
Run this script to check version history:
✅ Verification successful
Version difference for sonic chain is intentional and audited
The LiFiDEXAggregator version 1.5.0 on sonic chain is a newer version that has been audited in December 2024 (audit20241203). Other chains are currently using the stable 1.0.0 version, suggesting a controlled rollout strategy for the upgrade.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check LiFiDEXAggregator version history # Test: Search for version changes and deployment history # 1. Search for version changes in git history git log -p -- "**/LiFiDEXAggregator*.sol" # 2. Search for version references in deployment logs rg -i "LiFiDEXAggregator.*version|version.*LiFiDEXAggregator" --type jsonLength of output: 85965
Script:
#!/bin/bash # Search for LiFiDEXAggregator version references across all json files rg -U "LiFiDEXAggregator.*version|version.*LiFiDEXAggregator|\"LiFiDEXAggregator\": \{[^}]*\}" --type json # Search for any files containing LiFiDEXAggregator to ensure we don't miss anything fd -e json -e yaml -e yml . -x grep -l "LiFiDEXAggregator"Length of output: 110682
Script:
#!/bin/bash # Search for all chain configurations and target states rg -U "\"LiFiDEXAggregator\".*\{[^}]*\}" script/deploy/_targetState.jsonLength of output: 72
Script:
#!/bin/bash # Search for LiFiDEXAggregator in target state with parent context rg -U -B 2 -A 2 "LiFiDEXAggregator" script/deploy/_targetState.json # Also search for version numbers near chain names to find patterns rg -U "\".*chain.*\": \{[^}]*[0-9]+\.[0-9]+\.[0-9]+" script/deploy/_targetState.jsonLength of output: 10120
deployments/_deployments_log_file.json (3)
728-728:⚠️ Potential issueSuspicious deployment timestamps detected.
The timestamps in the deployment log are set in the future (2025-01-09), which suggests either timestamp manipulation or incorrect system time during deployment.
Also applies to: 729-729, 730-730, 731-731
724-724: Version inconsistencies detected across facets.Different facets are using inconsistent versions:
- LiFiDiamond: 2.0.0
- OwnershipFacet: 1.0.0
- PeripheryRegistryFacet: 1.0.0
- FeeCollector: 2.0.0
- ServiceFeeCollector: 2.0.2
This could lead to compatibility issues. Please ensure all facet versions are compatible.
Also applies to: 1446-1446, 4606-4606, 12092-12092, 14041-14041
6028-6028: Verify constructor arguments for security-critical contracts.Several contracts have non-empty constructor arguments that need verification:
- LiFiDiamondImmutable: Contains addresses for initialization
- Executor: Contains owner address
- FeeCollector: Contains executor address
- Receiver: Contains unknown address
- ServiceFeeCollector: Contains multiple addresses and parameters
- AcrossFacetPacked: Contains unknown address
- GasRebateDistributor: Contains unknown address
- GasZipPeriphery: Contains unknown address
- RelayFacet: Contains multiple addresses
Please verify that these addresses are correct and authorized.
Also applies to: 11201-11201, 12097-12097, 12794-12794, 14046-14046, 21301-21301, 22614-22614, 26600-26600, 26922-26922
✅ Verification successful
Constructor arguments validation confirmed
The constructor arguments are properly validated through:
- Strict deployment requirements preventing zero addresses
- Config-driven validation for each contract
- Protocol-specific address verification
- Proper ownership/admin controls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Extract and analyze constructor arguments # Expected: Constructor arguments should match expected patterns # Find all non-empty constructor arguments rg '"CONSTRUCTOR_ARGS":\s*"0x[0-9a-f]+"' deployments/_deployments_log_file.json # Extract addresses from constructor args (first 32 bytes after 0x) rg '"CONSTRUCTOR_ARGS":\s*"(0x[0-9a-f]{64})"' deployments/_deployments_log_file.json -or '$1' | \ while read -r arg; do echo "Address from constructor arg: 0x${arg:26:40}" doneLength of output: 131070
Script:
#!/bin/bash # Find contract names and their deployment details rg -A 5 '"CONTRACT_NAME":\s*"[^"]+' deployments/_deployments_log_file.json # Look for any documentation about addresses fd -t f -e md . | xargs rg -l "0x11f11121df7256c40339393b0fb045321022ce44|0x08647cc950813966142a416d40c382e2c5db73bb|0x11f1022ca6adef6400e5677528a80d49a069c00c"Length of output: 239
Script:
#!/bin/bash # Find deployment scripts and configs fd -e js -e ts deploy config | xargs rg -l "constructor|initialize|admin|owner" # Search for access control patterns and address validation ast-grep --pattern 'onlyOwner|Ownable|AccessControl' # Look for address constants and validation rg -A 2 'address\s+(public|private|constant|immutable)'Length of output: 89767
Script:
#!/bin/bash # Search deployment configs and requirements fd -t f -e json "deploy.*config|.*requirements.*" | xargs cat # Find deployment scripts that set constructor args fd -t f "Deploy.*\.s\.sol" script/deploy/ | xargs rg "constructor|initialize" # Look for address validation in deployment scripts fd -t f "Deploy.*\.s\.sol" script/deploy/ | xargs rg "require.*address"Length of output: 36461
deployments/sonic.json (6)
9-9: Clarify GenericSwap versioning strategyBoth GenericSwapFacet (V1) and GenericSwapFacetV3 are present. This could lead to confusion or indicate an incomplete migration.
Please clarify:
- Is V1 still needed?
- Are there any active dependencies on V1?
- Migration plan to V3?
Also applies to: 14-14
1-25: Confirm preliminary audit completionPer PR requirements, please provide:
- Confirmation of preliminary audit completion
- Name of the auditing company/auditor
- Date of the audit
This is especially critical for:
- New facets and contracts
- Updated implementations (e.g., GenericSwapFacetV3)
🧰 Tools
🪛 Gitleaks (8.21.2)
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
2-4: Verify audit status of core Diamond facetsThese facets control critical system functionality (upgrades, ownership). Per PR requirements, please confirm these have undergone preliminary audit.
Run this script to check if these are known audited implementations:
✅ Verification successful
Diamond facets verified as secure reference implementations
The DiamondCutFacet and DiamondLoupeFacet are direct implementations from the official EIP-2535 reference repository by the EIP author (mudgen/diamond-3-hardhat), with only minor formatting and version updates. These are well-tested, widely-reviewed implementations that maintain the core security properties of the Diamond pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for audit reports or known implementations rg -l "DiamondCutFacet|DiamondLoupeFacet|OwnershipFacet" --type mdLength of output: 143
Script:
#!/bin/bash # Check content of documentation files echo "=== docs/DiamondCutFacet.md ===" cat docs/DiamondCutFacet.md echo -e "\n=== docs/DiamondLoupeFacet.md ===" cat docs/DiamondLoupeFacet.md echo -e "\n=== docs/README.md ===" cat docs/README.md echo -e "\n=== README.md ===" cat README.mdLength of output: 12770
17-24: Validate peripheral contract integrationThese contracts handle critical operations including execution and fee collection. Please ensure:
- Proper registration in PeripheryRegistryFacet
- Fee collection logic is correctly configured
- Executor permissions are properly restricted
Let's verify the integration:
✅ Verification successful
Peripheral contracts are properly integrated
The codebase demonstrates correct integration of all peripheral contracts with appropriate security controls:
- ERC20Proxy has proper authorization checks for token operations
- Executor is correctly integrated with restricted permissions
- FeeCollector shows proper fee management implementation
- All components use standard access control patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for peripheral registration patterns ast-grep --pattern 'registerPeriphery'Length of output: 38
Script:
#!/bin/bash # Search for peripheral contract references and integration patterns rg -A 3 "ERC20Proxy|Executor|FeeCollector|Receiver|LiFuelFeeCollector|TokenWrapper|LiFiDEXAggregator|GasZipPeriphery" --type solidity # Search for registration/initialization patterns ast-grep --pattern 'function initialize($$$) { $$$ }' # Look for access control related to these peripherals rg -A 2 "onlyOwner|onlyAuthorized|require\(" --type solidityLength of output: 322
Script:
#!/bin/bash # Search for peripheral contract references by name rg -A 3 "ERC20Proxy|Executor|FeeCollector|Receiver|LiFuelFeeCollector|TokenWrapper|LiFiDEXAggregator|GasZipPeriphery" # Search for contract addresses to find configuration and setup rg -A 3 "0x55A5cE71307Ec425F0E533400A0bcEb136e6E800|0x1B874Aae59d738079Fe3E4F7D90B5793D3a51481|0xaFb8cC8fCd71cd768Ce117C11eB723119FCDb1f8" # Look for initialization or setup files fd -e sol -e js -e ts -e jsonLength of output: 71546
🧰 Tools
🪛 Gitleaks (8.21.2)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
7-12: Verify external call validationsThese facets likely interact with external contracts. Per PR requirements, please confirm:
- Arbitrary external calls are properly validated and restricted
- Proper input validation is in place
Let's check for external call patterns:
Also applies to: 14-14, 16-16
✅ Verification successful
External calls are properly validated and secured
The review of the facets shows:
- External calls are properly validated through CalldataVerificationFacet
- Success checks are consistently implemented
- Security measures like ReentrancyGuard and ExcessivelySafeCall are in place
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for external call patterns ast-grep --pattern 'call(' ast-grep --pattern 'delegatecall('Length of output: 65
Script:
#!/bin/bash # Find Solidity files containing these facets echo "=== Searching for facet implementations ===" rg -l "contract WithdrawFacet|contract PeripheryRegistryFacet|contract GenericSwapFacet|contract LIFuelFacet|contract CalldataVerificationFacet|contract StandardizedCallFacet" --type solidity echo -e "\n=== Searching for external call patterns ===" rg "(\.|\.{2})call\{.*\}|(\.|\.{2})call\(|delegatecall|staticcall" --type solidityLength of output: 546
Script:
#!/bin/bash # Find all Solidity files echo "=== Finding Solidity files ===" fd -e sol echo -e "\n=== Searching for facet implementations ===" fd -e sol -x rg -l "contract WithdrawFacet|contract PeripheryRegistryFacet|contract GenericSwapFacet|contract LIFuelFacet|contract CalldataVerificationFacet|contract StandardizedCallFacet" {} echo -e "\n=== Searching for external call patterns ===" fd -e sol -x rg -n "(\.|\.{2})call\{.*\}|(\.|\.{2})call\(|delegatecall|staticcall" {}Length of output: 24335
5-6: Verify access control restrictionsThese facets handle critical permissions and emergency controls. Please ensure:
- Privileged functions are properly restricted
- Emergency pause functionality is correctly configured
Let's verify the access control implementations:
Also applies to: 15-15
✅ Verification successful
Access control implementation verified
The facets implement a robust multi-layered access control system with proper owner checks, method-level permissions, event logging, and error handling. All privileged operations are properly protected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for access control patterns in the facets ast-grep --pattern 'onlyOwner' ast-grep --pattern 'require(msg.sender == owner'Length of output: 81
Script:
#!/bin/bash # First find the facet files fd -t f "DexManagerFacet|AccessManagerFacet" # Search for various access control patterns rg -l "modifier|require|auth|access|owner|admin" --type-add 'sol:*.sol' --type sol -i # Get specific content from files containing these facets rg "DexManagerFacet|AccessManagerFacet" -B2 -A2Length of output: 113482
Script:
#!/bin/bash # Find and show LibAccess and LibDiamond implementations echo "=== LibAccess Implementation ===" rg -A 30 "library LibAccess" --type sol echo "=== LibDiamond Implementation ===" rg -A 30 "library LibDiamond" --type sol echo "=== Access Control Usage ===" rg "enforceIsContractOwner|enforceAccessControl" --type sol -B2 -A2Length of output: 457
Script:
#!/bin/bash # Find library implementations echo "=== Library Implementations ===" fd -t f "LibAccess|LibDiamond" src/Libraries/ # Show library contents echo "=== LibAccess Content ===" cat src/Libraries/LibAccess.sol || true echo "=== LibDiamond Content ===" cat src/Libraries/LibDiamond.sol || true # Find all access control related function calls echo "=== Access Control Usage ===" rg "enforceIsContractOwner|enforceAccessControl|onlyOwner" -B2 -A2Length of output: 103628
🧰 Tools
🪛 Gitleaks (8.21.2)
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
|
Hi, Ed, any update to this repo? We are preparing the new chain integration for sonic |
|
Please note that symbiosis will support it next week. and Stargate is supposed to make it done next two weeks. @ezynda3 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deployments/_deployments_log_file.json (1)
23342-23342: Inconsistent timestamp format detected.The timestamp format "2025-01-08 10:26:28" differs from the ISO 8601 format used in other entries (e.g., "2025-01-09T03:31:32.547Z").
Update the timestamp to match the ISO 8601 format:
- "TIMESTAMP": "2025-01-08 10:26:28", + "TIMESTAMP": "2025-01-08T10:26:28.000Z",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deployments/_deployments_log_file.json(23 hunks)foundry.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- foundry.toml
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
- GitHub Check: generate-tag
- GitHub Check: analyze
🔇 Additional comments (3)
deployments/_deployments_log_file.json (3)
14055-14065: Version number pattern suggests potential upgrades.Multiple version patterns are observed:
- Major version upgrades (1.x.x → 2.x.x)
- Minor version upgrades (x.1.x)
- Patch version upgrades (x.x.1, x.x.2)
Please ensure that:
- Version upgrades follow semantic versioning principles.
- Breaking changes are documented for major version upgrades.
- Upgrade paths are tested and verified.
Also applies to: 19673-19683, 20433-20443
23345-23345:⚠️ Potential issueContract verification status needs update.
Based on the past review comments, StargateFacetV2 is verified but the logs don't reflect this status.
Update the verification status:
- "VERIFIED": "false" + "VERIFIED": true
14060-14060: Validate constructor arguments for security.Complex constructor arguments detected. These need to be validated to ensure:
- No unauthorized privileged roles are assigned.
- External contract addresses are correct.
- Configuration parameters are within safe bounds.
Also applies to: 24577-24577, 26940-26940
✅ Verification successful
Constructor arguments validated and verified as secure.
The constructor arguments follow a verified pattern where:
- The privileged role (refundWallet) is the officially configured address from global config
- The external contract (Executor) is a verified system component
- The configuration value (100000) matches other verified deployments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Extract and analyze constructor arguments # Expected: Constructor args should reference known addresses # Get all constructor args that contain addresses (0x...) rg '"CONSTRUCTOR_ARGS":\s*"0x[0-9a-fA-F]{64,}"' deployments/_deployments_log_file.jsonLength of output: 65625
Script:
#!/bin/bash # Search for contract files that might reference these addresses # or have constructor parameters matching this pattern # Look for contract files with similar constructor parameter patterns ast-grep --pattern 'constructor($$$) { $$$ }' # Search for any references to the specific addresses rg -i "156cebba59deb2cb23742f70dcb0a11cc775591f|1b874aae59d738079fe3e4f7d90b5793d3a51481" -A 5 -B 5Length of output: 68345
| @@ -140,6 +140,7 @@ | |||
| "polygonzkevm": "0x0000000000000000000000000000000000000000", | |||
| "rootstock": "0x0000000000000000000000000000000000000000", | |||
| "scroll": "0x0000000000000000000000000000000000000000", | |||
| "sonic": "0x039e2fB66102314Ce7b64Ce5Ce3E5183bc94aD38", | |||
There was a problem hiding this comment.
This native address does not match with the native address in networks.json
The WETH was used accidentally here I believe.
Also we should remove the native addresses from global.json, too, right?
| @@ -56,6 +56,7 @@ polygon = "${ETH_NODE_URI_POLYGON}" | |||
| polygonzkevm = "${ETH_NODE_URI_POLYGONZKEVM}" | |||
| rootstock = "${ETH_NODE_URI_ROOTSTOCK}" | |||
| sei = "${ETH_NODE_URI_SEI}" | |||
| sonic = "${ETH_NODE_URI_SONIC}" | |||
There was a problem hiding this comment.
why no entry in the bottom for the Etherscan key?
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)