Deploy Testnet Contracts #1312
Deploy Testnet Contracts #1312
Conversation
github-actions
bot
added
M-contracts
![github-advanced-security[bot]](https://github.com/avatars/in/57789?s=60&v=4){kind=small}
| @@ -0,0 +1,36 @@ | |||
| // SPDX-License-Identifier: AGPL-3.0 | |||
| pragma solidity ^0.8.13; | |||
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
| @@ -0,0 +1,28 @@ | |||
| // SPDX-License-Identifier: AGPL-3.0 | |||
| pragma solidity >=0.6.0; | |||
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1312 +/- ##
===================================================
- Coverage 60.09064% 58.12856% -1.96209%
===================================================
Files 532 565 +33
Lines 32877 34102 +1225
Branches 277 277
===================================================
+ Hits 19756 19823 +67
- Misses 11334 12464 +1130
- Partials 1787 1815 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
It seems that the existing sections are still valid, so I'll keep them as they are. TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (36)
- packages/contracts-core/deployments/arb_sepolia/CREATE3.json
- packages/contracts-core/deployments/arb_sepolia/Destination.json
- packages/contracts-core/deployments/arb_sepolia/GasOracle.json
- packages/contracts-core/deployments/arb_sepolia/LightInbox.json
- packages/contracts-core/deployments/arb_sepolia/LightManager.json
- packages/contracts-core/deployments/arb_sepolia/Origin.json
- packages/contracts-core/deployments/arb_sepolia/PingPongClient.json
- packages/contracts-core/deployments/arb_sepolia/TestClient.json
- packages/contracts-core/deployments/optimism_sepolia/CREATE3.json
- packages/contracts-core/deployments/optimism_sepolia/Destination.json
- packages/contracts-core/deployments/optimism_sepolia/GasOracle.json
- packages/contracts-core/deployments/optimism_sepolia/LightInbox.json
- packages/contracts-core/deployments/optimism_sepolia/LightManager.json
- packages/contracts-core/deployments/optimism_sepolia/Origin.json
- packages/contracts-core/deployments/optimism_sepolia/PingPongClient.json
- packages/contracts-core/deployments/optimism_sepolia/TestClient.json
- packages/contracts-core/deployments/sepolia/CREATE3.json
- packages/contracts-core/deployments/sepolia/Destination.json
- packages/contracts-core/deployments/sepolia/GasOracle.json
- packages/contracts-core/deployments/sepolia/LightInbox.json
- packages/contracts-core/deployments/sepolia/LightManager.json
- packages/contracts-core/deployments/sepolia/Origin.json
- packages/contracts-core/deployments/sepolia/PingPongClient.json
- packages/contracts-core/deployments/sepolia/TestClient.json
- packages/contracts-core/deployments/synapse_sepolia/BondingManager.json
- packages/contracts-core/deployments/synapse_sepolia/CREATE3.json
- packages/contracts-core/deployments/synapse_sepolia/Destination.json
- packages/contracts-core/deployments/synapse_sepolia/GasOracle.json
- packages/contracts-core/deployments/synapse_sepolia/Inbox.json
- packages/contracts-core/deployments/synapse_sepolia/Origin.json
- packages/contracts-core/deployments/synapse_sepolia/PingPongClient.json
- packages/contracts-core/deployments/synapse_sepolia/Summit.json
- packages/contracts-core/deployments/synapse_sepolia/TestClient.json
- packages/contracts-core/foundry.toml
- packages/contracts-core/script/configs/Messaging003.dc.json
- packages/contracts-core/script/configs/Messaging003AgentRoot.dc.json
Files selected for processing (14)
- .gitmodules (1 hunks)
- ethergo/internal/test-data (1 hunks)
- packages/contracts-core/contracts/create3/CREATE3Factory.sol (1 hunks)
- packages/contracts-core/contracts/create3/ICREATE3Factory.sol (1 hunks)
- packages/contracts-core/contracts/libs/Constants.sol (1 hunks)
- packages/contracts-core/lib/create3-factory (1 hunks)
- packages/contracts-core/lib/forge-std (1 hunks)
- packages/contracts-core/lib/solmate (1 hunks)
- packages/contracts-core/script/DeployCREATE3.s.sol (1 hunks)
- packages/contracts-core/script/DeployClients003.s.sol (5 hunks)
- packages/contracts-core/script/DeployMessaging003Base.s.sol (9 hunks)
- packages/contracts-core/script/DeployMessaging003SynChain.s.sol (6 hunks)
- packages/contracts-core/script/utils/DeployerUtils.sol (7 hunks)
- services/cctp-relayer/external/synapse-contracts (1 hunks)
Files skipped from review due to trivial changes (8)
- .gitmodules
- packages/contracts-core/contracts/create3/ICREATE3Factory.sol
- packages/contracts-core/lib/create3-factory
- packages/contracts-core/lib/solmate
- packages/contracts-core/script/DeployClients003.s.sol
- packages/contracts-core/script/DeployMessaging003Base.s.sol
- packages/contracts-core/script/DeployMessaging003SynChain.s.sol
- packages/contracts-core/script/utils/DeployerUtils.sol
Additional comments: 10
services/cctp-relayer/external/synapse-contracts (1)
- 1-1: The hunk provided is empty. Please provide a valid hunk for review.
packages/contracts-core/lib/forge-std (1)
- 1-1: The hunk provided does not contain any code to review. Please provide the relevant code for review.
ethergo/internal/test-data (1)
- 1-1: The hunk is empty, there is no code to review.
packages/contracts-core/contracts/libs/Constants.sol (1)
- 47-53: The changes to
MAX_CONTENT_BYTESandSYNAPSE_DOMAINconstants seem fine. However, ensure that these changes do not break any existing functionality that relies on these constants. Also, verify that the new value ofSYNAPSE_DOMAINis correct and aligns with your intended configuration.packages/contracts-core/contracts/create3/CREATE3Factory.sol (3)
2-2: The pragma version has been updated to
^0.8.13. Ensure that this version is compatible with the rest of your codebase and that it doesn't introduce any breaking changes. Also, consider using a fixed version to avoid unexpected behavior due to compiler updates.14-23: The
deployfunction now takes abytes32 saltandbytes memory creationCodeas parameters, and returns anaddressof the deployed contract. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.26-35: The
getDeployedfunction now takes an additionaladdress deployerparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.packages/contracts-core/script/DeployCREATE3.s.sol (3)
1-26: The contract
DeployCREATE3is well-structured and follows Solidity best practices. The use ofpragma solidity 0.8.17;ensures compatibility with the latest compiler features and security fixes. The contract imports necessary libraries and contracts, and defines a constructor and anexternalfunctionrun(). Therun()function creates a new instance ofCREATE3Factoryand saves the deployment information. The contract also uses thestdJsonandStringslibraries for string manipulation, which is a good practice for handling string data in Solidity.16-18: The constructor calls
setupPK("CREATE3_PRIVATE_KEY")to set up a private key. Ensure that this private key is securely managed and not exposed in any public repositories or logs.20-25: The
run()function creates a new instance ofCREATE3Factoryand saves the deployment information. Ensure that thestartBroadcast(true)andstopBroadcast()functions are correctly managing the broadcast state, and that thesaveDeploymentfunction is correctly saving the deployment information.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (29)
- packages/contracts-core/deployments/arb_sepolia/Destination.json
- packages/contracts-core/deployments/arb_sepolia/GasOracle.json
- packages/contracts-core/deployments/arb_sepolia/LightInbox.json
- packages/contracts-core/deployments/arb_sepolia/LightManager.json
- packages/contracts-core/deployments/arb_sepolia/Origin.json
- packages/contracts-core/deployments/arb_sepolia/PingPongClient.json
- packages/contracts-core/deployments/arb_sepolia/TestClient.json
- packages/contracts-core/deployments/optimism_sepolia/Destination.json
- packages/contracts-core/deployments/optimism_sepolia/GasOracle.json
- packages/contracts-core/deployments/optimism_sepolia/LightInbox.json
- packages/contracts-core/deployments/optimism_sepolia/LightManager.json
- packages/contracts-core/deployments/optimism_sepolia/Origin.json
- packages/contracts-core/deployments/optimism_sepolia/PingPongClient.json
- packages/contracts-core/deployments/optimism_sepolia/TestClient.json
- packages/contracts-core/deployments/sepolia/Destination.json
- packages/contracts-core/deployments/sepolia/GasOracle.json
- packages/contracts-core/deployments/sepolia/LightInbox.json
- packages/contracts-core/deployments/sepolia/LightManager.json
- packages/contracts-core/deployments/sepolia/Origin.json
- packages/contracts-core/deployments/sepolia/PingPongClient.json
- packages/contracts-core/deployments/sepolia/TestClient.json
- packages/contracts-core/deployments/synapse_sepolia/BondingManager.json
- packages/contracts-core/deployments/synapse_sepolia/Destination.json
- packages/contracts-core/deployments/synapse_sepolia/GasOracle.json
- packages/contracts-core/deployments/synapse_sepolia/Inbox.json
- packages/contracts-core/deployments/synapse_sepolia/Origin.json
- packages/contracts-core/deployments/synapse_sepolia/PingPongClient.json
- packages/contracts-core/deployments/synapse_sepolia/Summit.json
- packages/contracts-core/deployments/synapse_sepolia/TestClient.json
Files selected for processing (2)
- packages/contracts-core/script/DeployClients003.s.sol (5 hunks)
- packages/contracts-core/script/DeployMessaging003Base.s.sol (9 hunks)
Files skipped from review due to trivial changes (2)
- packages/contracts-core/script/DeployClients003.s.sol
- packages/contracts-core/script/DeployMessaging003Base.s.sol
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files ignored due to filter (31)
- packages/contracts-core/deployments/arb_sepolia/Destination.json
- packages/contracts-core/deployments/arb_sepolia/GasOracle.json
- packages/contracts-core/deployments/arb_sepolia/LightInbox.json
- packages/contracts-core/deployments/arb_sepolia/LightManager.json
- packages/contracts-core/deployments/arb_sepolia/Origin.json
- packages/contracts-core/deployments/arb_sepolia/PingPongClient.json
- packages/contracts-core/deployments/arb_sepolia/TestClient.json
- packages/contracts-core/deployments/optimism_sepolia/Destination.json
- packages/contracts-core/deployments/optimism_sepolia/GasOracle.json
- packages/contracts-core/deployments/optimism_sepolia/LightInbox.json
- packages/contracts-core/deployments/optimism_sepolia/LightManager.json
- packages/contracts-core/deployments/optimism_sepolia/Origin.json
- packages/contracts-core/deployments/optimism_sepolia/PingPongClient.json
- packages/contracts-core/deployments/optimism_sepolia/TestClient.json
- packages/contracts-core/deployments/sepolia/Destination.json
- packages/contracts-core/deployments/sepolia/GasOracle.json
- packages/contracts-core/deployments/sepolia/LightInbox.json
- packages/contracts-core/deployments/sepolia/LightManager.json
- packages/contracts-core/deployments/sepolia/Origin.json
- packages/contracts-core/deployments/sepolia/PingPongClient.json
- packages/contracts-core/deployments/sepolia/TestClient.json
- packages/contracts-core/deployments/synapse_sepolia/BondingManager.json
- packages/contracts-core/deployments/synapse_sepolia/Destination.json
- packages/contracts-core/deployments/synapse_sepolia/GasOracle.json
- packages/contracts-core/deployments/synapse_sepolia/Inbox.json
- packages/contracts-core/deployments/synapse_sepolia/Origin.json
- packages/contracts-core/deployments/synapse_sepolia/PingPongClient.json
- packages/contracts-core/deployments/synapse_sepolia/Summit.json
- packages/contracts-core/deployments/synapse_sepolia/TestClient.json
- packages/contracts-core/script/configs/Messaging003.dc.json
- packages/contracts-core/script/configs/Messaging003AgentRoot.dc.json
Files selected for processing (2)
- packages/contracts-core/script/DeployClients003.s.sol (5 hunks)
- packages/contracts-core/script/DeployMessaging003Base.s.sol (9 hunks)
Files skipped from review due to trivial changes (1)
- packages/contracts-core/script/DeployClients003.s.sol
Additional comments: 2
packages/contracts-core/script/DeployMessaging003Base.s.sol (2)
343-352: The
_checkMessagingBasefunction correctly checks thelocalDomainandowneragainst the expected values. This is a good practice for post-deployment validation.355-406: The getter check functions (
_checkGetterAgentManager,_checkGetterInbox, etc.) are thorough and ensure that the deployed contracts have the correct references. This is crucial for the integrity of the system.
| pragma solidity 0.8.17; | ||
|
|
||
| // ═════════════════════════════ CONTRACT IMPORTS ══════════════════════════════ | ||
| import {AgentSecured, MessagingBase} from "../contracts/base/AgentSecured.sol"; | ||
| import {BondingManager} from "../contracts/manager/BondingManager.sol"; | ||
| import {Destination} from "../contracts/Destination.sol"; | ||
| import {GasOracle} from "../contracts/GasOracle.sol"; | ||
| import {Origin} from "../contracts/Origin.sol"; | ||
| import {Summit} from "../contracts/Summit.sol"; | ||
| import { AgentSecured, MessagingBase } from "../contracts/base/AgentSecured.sol"; | ||
| import { BondingManager } from "../contracts/manager/BondingManager.sol"; | ||
| import { Destination } from "../contracts/Destination.sol"; | ||
| import { GasOracle } from "../contracts/GasOracle.sol"; | ||
| import { Origin } from "../contracts/Origin.sol"; | ||
| import { Summit } from "../contracts/Summit.sol"; | ||
| // ═════════════════════════════ INTERNAL IMPORTS ══════════════════════════════ | ||
| import {DeployerUtils} from "./utils/DeployerUtils.sol"; | ||
| import { DeployerUtils } from "./utils/DeployerUtils.sol"; | ||
| // ═════════════════════════════ EXTERNAL IMPORTS ══════════════════════════════ | ||
| import {console, stdJson} from "forge-std/Script.sol"; | ||
| import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; | ||
| import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; | ||
| import { console, stdJson } from "forge-std/Script.sol"; | ||
| import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; | ||
| import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; | ||
|
|
||
| // solhint-disable no-console | ||
| // solhint-disable ordering |
There was a problem hiding this comment.
The pragma directive should be locked to a specific compiler version to prevent unexpected behavior in different compiler versions. If the code is intended to be flexible with the compiler version, this is fine, but it should be a conscious decision.
| constructor() { | ||
| setupPK("MESSAGING_DEPLOYER_PRIVATE_KEY"); | ||
| localDomain = uint32(block.chainid); | ||
| deploymentSalt = keccak256("Messaging003"); | ||
| deploymentSalt = keccak256("Messaging003-07"); | ||
| } |
There was a problem hiding this comment.
The deploymentSalt is hardcoded in the constructor, which could lead to predictability in contract deployment addresses. Consider parameterizing the salt or deriving it from less predictable data to enhance security.
| summit = predictFactoryDeployment(SUMMIT); | ||
| } | ||
| // Deploy and initialize contracts | ||
| _deployAndCheckAddress(agentManagerName(), _deployAgentManager, _initializeAgentManager, agentManager); | ||
| _deployAndCheckAddress(statementInboxName(), _deployStatementInbox, _initializeStatementInbox, statementInbox); | ||
| _deployAndCheckAddress(DESTINATION, _deployDestination, _initializeDestination, destination); | ||
| _deployAndCheckAddress( | ||
| agentManagerName(), | ||
| _deployAgentManager, | ||
| _initializeAgentManager, | ||
| agentManager | ||
| ); | ||
| _deployAndCheckAddress( | ||
| statementInboxName(), | ||
| _deployStatementInbox, | ||
| _initializeStatementInbox, | ||
| statementInbox | ||
| ); | ||
| _deployAndCheckAddress( | ||
| DESTINATION, | ||
| _deployDestination, | ||
| _initializeDestination, | ||
| destination | ||
| ); | ||
| _deployAndCheckAddress(GAS_ORACLE, _deployGasOracle, _initializeGasOracle, gasOracle); | ||
| _deployAndCheckAddress(ORIGIN, _deployOrigin, _initializeOrigin, origin); | ||
| if (isSynapseChain()) { |
There was a problem hiding this comment.
The isSynapseChain() function is called multiple times within the _deploy function. This could be optimized by storing the result in a local variable if the chain type does not change during the execution of the function.
| function(address) internal initFunc, | ||
| address predictedDeployment | ||
| ) internal { | ||
| (address deployment,) = deployContract(contractName, deployFunc, initFunc); | ||
| (address deployment, ) = deployContract(contractName, deployFunc, initFunc); | ||
| require(deployment == predictedDeployment, string.concat(contractName, ": wrong address")); |
There was a problem hiding this comment.
The _deployAndCheckAddress function uses a require statement to ensure that the deployed contract address matches the predicted address. This is a good practice for ensuring deployment consistency. However, the error message could include the expected and actual addresses to aid debugging.
| /// @dev Deploys Destination. | ||
| /// Note: requires AgentManager and StatementInbox addresses to have been set. | ||
| /// Note: requires AgentManager to have been deployed. | ||
| function _deployDestination() internal returns (address deployment, bytes memory constructorArgs) { | ||
| function _deployDestination() | ||
| internal | ||
| returns (address deployment, bytes memory constructorArgs) | ||
| { | ||
| // new Destination(domain, agentManager, statementInbox) | ||
| require(agentManager != address(0), "Agent Manager not set"); | ||
| require(statementInbox != address(0), "Statement Inbox not set"); |
There was a problem hiding this comment.
The _deployDestination function checks if the agentManager and statementInbox addresses are set but does not check if the contracts at those addresses have been deployed. This could lead to a situation where the deployment proceeds with uninitialized addresses. Consider adding checks for the code at those addresses.
| /// @dev Deploys GasOracle. | ||
| /// Note: requires Destination address to have been set. | ||
| function _deployGasOracle() internal returns (address deployment, bytes memory constructorArgs) { | ||
| function _deployGasOracle() | ||
| internal | ||
| returns (address deployment, bytes memory constructorArgs) | ||
| { | ||
| // new GasOracle(domain, destination) | ||
| require(destination != address(0), "Destination not set"); | ||
| constructorArgs = abi.encode(localDomain, destination); |
There was a problem hiding this comment.
Similar to the previous comment, the _deployGasOracle function checks if the destination address is set but does not check if the contract at that address has been deployed.
| } | ||
|
|
||
| /// @dev Transfers ownership of a given contract to a new owner. | ||
| function _transferOwnership(string memory name, address deployment, address newOwner) internal { | ||
| function _transferOwnership( | ||
| string memory name, | ||
| address deployment, | ||
| address newOwner | ||
| ) internal { | ||
| if (deployment == address(0)) { | ||
| console.log(" %s: skipped (not deployed)", name); | ||
| return; |
There was a problem hiding this comment.
The _transferOwnership function checks if the deployment address is zero before attempting to transfer ownership. This is a good safeguard against transferring ownership of a non-existent contract. However, it logs a message and returns without throwing an error, which could silently fail in a script execution context. Consider whether failing loudly (with a require or revert) would be more appropriate.
| function _logCheckedGetter( | ||
| string memory name, | ||
| string memory getter, | ||
| address deployment, | ||
| address expectedValue | ||
| ) internal view { | ||
| console.log(" [%s][%s]: checking .%s()", name, deployment, getter); | ||
| console.log(" expecting value: %s", expectedValue); | ||
| } | ||
|
|
||
| /// @dev Shortcut for logging | ||
| function _logCheckedGetter(string memory name, string memory getter, address deployment, uint32 expectedValue) | ||
| internal | ||
| view | ||
| { | ||
| function _logCheckedGetter( | ||
| string memory name, | ||
| string memory getter, | ||
| address deployment, | ||
| uint32 expectedValue | ||
| ) internal view { | ||
| console.log(" [%s][%s]: checking .%s()", name, deployment, getter); | ||
| console.log(" expecting value: %s", expectedValue); | ||
| } |
There was a problem hiding this comment.
The logging functions _logCheckedGetter are duplicated for different expected value types. This could be refactored to a single function that handles multiple types or uses Solidity's abi.encode to log the expected value in a generic way.
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Summary by CodeRabbit
The existing bullet-point list is still valid based on the provided instructions.