Contract deployment: store important addresses #1653
Conversation
|
The changes across various files indicate an update to a blockchain-related codebase, where a management contract address is now dynamically passed as a command-line argument or environment variable. This allows for better flexibility and integration with the deployment and configuration of blockchain contracts, particularly for a bridge and a cross-chain messenger within a testnet environment. Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (8)
- .github/workflows/manual-deploy-testnet-l2.yml (1 hunks)
- contracts/deployment_scripts/bridge/001_deploy_bridge.ts (2 hunks)
- contracts/deployment_scripts/messenger/layer1/001_deploy_cross_chain_messenger.ts (1 hunks)
- testnet/launcher/l2contractdeployer/cmd/cli.go (3 hunks)
- testnet/launcher/l2contractdeployer/cmd/cli_flags.go (2 hunks)
- testnet/launcher/l2contractdeployer/cmd/main.go (1 hunks)
- testnet/launcher/l2contractdeployer/config.go (2 hunks)
- testnet/launcher/l2contractdeployer/docker.go (1 hunks)
Files skipped from review due to trivial changes (3)
- testnet/launcher/l2contractdeployer/cmd/cli.go
- testnet/launcher/l2contractdeployer/cmd/cli_flags.go
- testnet/launcher/l2contractdeployer/docker.go
Additional comments: 4
.github/workflows/manual-deploy-testnet-l2.yml (1)
- 312-315: The changes made to pass the management contract address and message bus contract address as command-line arguments to the L2 contract deployment process are correct and align with the pull request's goal of centralizing contract address storage. This ensures that the deployment scripts use the latest addresses stored in the management contract, which is crucial for consistency and reliability in the deployment process.
testnet/launcher/l2contractdeployer/cmd/main.go (1)
- 16-22: The addition of
l2cd.WithManagementContractAddressandl2cd.WithMessageBusContractAddressto theNewContractDeployerConfigfunction indicates that the deployment process now includes setting up a management contract address and a message bus contract address. This is consistent with the pull request summary, which mentions the introduction of a management contract to store important contract addresses. Ensure that themanagementContractAddrandmessageBusContractAddrare properly validated and sanitized before being used in the deployment process to prevent any security vulnerabilities. Additionally, verify that the corresponding environment variables or command-line arguments are being correctly passed to the deployment scripts and that they are documented for the operators of the deployment process.testnet/launcher/l2contractdeployer/config.go (2)
7-19: The new field
managementContractAddresshas been added to theConfigstruct to store the address of the management contract. This is a critical update for the deployment process to ensure that the management contract address is available to the deployment scripts. Make sure that all instances where theConfigstruct is used are updated to handle this new field appropriately.57-61: The function
WithManagementContractAddresshas been correctly added to allow setting themanagementContractAddressin theConfigstruct. This follows the established pattern of using option functions to configure theConfigobject. Ensure that this function is used wherever theConfigobject is created or modified to include the management contract address.
| deployments, | ||
| getNamedAccounts | ||
| } = hre; | ||
| const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS!! | ||
|
|
||
| // L2 address of a prefunded deployer account to be used in smart contracts | ||
| const accountsL2 = await getNamedAccounts(); |
There was a problem hiding this comment.
The use of the non-null assertion operator (!!) in line 15 is potentially unsafe. If MGMT_CONTRACT_ADDRESS is not set in the environment variables, this will lead to a runtime error. It would be better to handle the potential absence of this environment variable gracefully, with proper error handling to inform the user of the misconfiguration.
- const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS!!
+ const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS;
+ if (!mgmtContractAddress) {
+ throw new Error('MGMT_CONTRACT_ADDRESS must be set in the environment variables.');
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| deployments, | |
| getNamedAccounts | |
| } = hre; | |
| const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS!! | |
| // L2 address of a prefunded deployer account to be used in smart contracts | |
| const accountsL2 = await getNamedAccounts(); | |
| deployments, | |
| getNamedAccounts | |
| } = hre; | |
| const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS; | |
| if (!mgmtContractAddress) { | |
| throw new Error('MGMT_CONTRACT_ADDRESS must be set in the environment variables.'); | |
| } | |
| // L2 address of a prefunded deployer account to be used in smart contracts | |
| const accountsL2 = await getNamedAccounts(); |
|
|
||
| const { deployer } = await hre.companionNetworks.layer1.getNamedAccounts(); | ||
|
|
||
| // Read the message bus address from the management contract deployment. | ||
| // Use the contract addresses from the management contract deployment. | ||
| const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS!! | ||
| const messageBusAddress : string = process.env.MESSAGE_BUS_ADDRESS!! | ||
| console.log(`Message Bus address ${messageBusAddress}`); | ||
|
|
||
| // Setup the cross chain messenger and point it to the message bus from the management contract to be used for validation | ||
| await deployments.deploy('CrossChainMessenger', { | ||
| const crossChainDeployment = await deployments.deploy('CrossChainMessenger', { | ||
| from: deployer, | ||
| args: [ messageBusAddress ], | ||
| log: true, | ||
| }); | ||
|
|
||
| // get management contract and write the cross chain messenger address to it | ||
| const mgmtContract = (await hre.ethers.getContractFactory('ManagementContract')).attach(mgmtContractAddress) | ||
| const tx = await mgmtContract.SetImportantContractAddress("L1CrossChainMessenger", crossChainDeployment.address); | ||
| const receipt = await tx.wait(); | ||
| if (receipt.status !== 1) { | ||
| console.log("Failed to set L1CrossChainMessenger in management contract"); | ||
| } | ||
| console.log(`L1CrossChainMessenger=${crossChainDeployment.address}`) | ||
| }; | ||
|
|
||
| export default func; |
There was a problem hiding this comment.
The script uses non-null assertion operators (!!) to retrieve environment variables, which could lead to runtime errors if the variables are not set. It would be safer to check for the existence of these variables and handle the potential undefined case gracefully.
- const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS!!
- const messageBusAddress : string = process.env.MESSAGE_BUS_ADDRESS!!
+ const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS;
+ const messageBusAddress = process.env.MESSAGE_BUS_ADDRESS;
+ if (!mgmtContractAddress || !messageBusAddress) {
+ throw new Error('Environment variables MGMT_CONTRACT_ADDRESS or MESSAGE_BUS_ADDRESS are not set.');
+ }Additionally, the script assumes that the deployment will always be successful and does not handle the case where crossChainDeployment might be undefined if the deployment fails. It's important to add error handling for the deployment step.
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const { deployer } = await hre.companionNetworks.layer1.getNamedAccounts(); | |
| // Read the message bus address from the management contract deployment. | |
| // Use the contract addresses from the management contract deployment. | |
| const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS!! | |
| const messageBusAddress : string = process.env.MESSAGE_BUS_ADDRESS!! | |
| console.log(`Message Bus address ${messageBusAddress}`); | |
| // Setup the cross chain messenger and point it to the message bus from the management contract to be used for validation | |
| await deployments.deploy('CrossChainMessenger', { | |
| const crossChainDeployment = await deployments.deploy('CrossChainMessenger', { | |
| from: deployer, | |
| args: [ messageBusAddress ], | |
| log: true, | |
| }); | |
| // get management contract and write the cross chain messenger address to it | |
| const mgmtContract = (await hre.ethers.getContractFactory('ManagementContract')).attach(mgmtContractAddress) | |
| const tx = await mgmtContract.SetImportantContractAddress("L1CrossChainMessenger", crossChainDeployment.address); | |
| const receipt = await tx.wait(); | |
| if (receipt.status !== 1) { | |
| console.log("Failed to set L1CrossChainMessenger in management contract"); | |
| } | |
| console.log(`L1CrossChainMessenger=${crossChainDeployment.address}`) | |
| }; | |
| export default func; | |
| const { deployer } = await hre.companionNetworks.layer1.getNamedAccounts(); | |
| // Use the contract addresses from the management contract deployment. | |
| const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS; | |
| const messageBusAddress = process.env.MESSAGE_BUS_ADDRESS; | |
| if (!mgmtContractAddress || !messageBusAddress) { | |
| throw new Error('Environment variables MGMT_CONTRACT_ADDRESS or MESSAGE_BUS_ADDRESS are not set.'); | |
| } | |
| console.log(`Message Bus address ${messageBusAddress}`); | |
| // Setup the cross chain messenger and point it to the message bus from the management contract to be used for validation | |
| const crossChainDeployment = await deployments.deploy('CrossChainMessenger', { | |
| from: deployer, | |
| args: [ messageBusAddress ], | |
| log: true, | |
| }); | |
| // get management contract and write the cross chain messenger address to it | |
| const mgmtContract = (await hre.ethers.getContractFactory('ManagementContract')).attach(mgmtContractAddress) | |
| const tx = await mgmtContract.SetImportantContractAddress("L1CrossChainMessenger", crossChainDeployment.address); | |
| const receipt = await tx.wait(); | |
| if (receipt.status !== 1) { | |
| console.log("Failed to set L1CrossChainMessenger in management contract"); | |
| } | |
| console.log(`L1CrossChainMessenger=${crossChainDeployment.address}`) | |
| }; | |
| export default func; |
| // get management contract and write the L1 bridge address to it | ||
| const mgmtContract = (await hre.ethers.getContractFactory('ManagementContract')).attach(mgmtContractAddress) | ||
| const tx = await mgmtContract.SetImportantContractAddress("L1Bridge", layer1BridgeDeployment.address); | ||
| const receipt = await tx.wait(); | ||
| if (receipt.status !== 1) { | ||
| console.log("Failed to set L1BridgeAddress in management contract"); | ||
| } | ||
| console.log(`L1BridgeAddress=${layer1BridgeDeployment.address}`) |
There was a problem hiding this comment.
The transaction receipt status check is using a strict equality comparison against the number 1. It is important to ensure that the status codes being returned by the Ethereum client conform to this expectation. If the status codes are boolean, this check will always fail. Additionally, the error message should be more descriptive and possibly throw an error to halt the deployment process if setting the address in the management contract is critical.
- if (receipt.status !== 1) {
+ if (!receipt.status) {
console.log("Failed to set L1BridgeAddress in management contract");
+ throw new Error("Failed to set L1BridgeAddress in management contract");
}Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // get management contract and write the L1 bridge address to it | |
| const mgmtContract = (await hre.ethers.getContractFactory('ManagementContract')).attach(mgmtContractAddress) | |
| const tx = await mgmtContract.SetImportantContractAddress("L1Bridge", layer1BridgeDeployment.address); | |
| const receipt = await tx.wait(); | |
| if (receipt.status !== 1) { | |
| console.log("Failed to set L1BridgeAddress in management contract"); | |
| } | |
| console.log(`L1BridgeAddress=${layer1BridgeDeployment.address}`) | |
| // get management contract and write the L1 bridge address to it | |
| const mgmtContract = (await hre.ethers.getContractFactory('ManagementContract')).attach(mgmtContractAddress) | |
| const tx = await mgmtContract.SetImportantContractAddress("L1Bridge", layer1BridgeDeployment.address); | |
| const receipt = await tx.wait(); | |
| if (!receipt.status) { | |
| console.log("Failed to set L1BridgeAddress in management contract"); | |
| throw new Error("Failed to set L1BridgeAddress in management contract"); | |
| } | |
| console.log(`L1BridgeAddress=${layer1BridgeDeployment.address}`) |
| if (receipt.status !== 1) { | ||
| console.log("Failed to set L1CrossChainMessenger in management contract"); | ||
| } |
There was a problem hiding this comment.
The error handling for the transaction receipt only logs a message to the console but does not halt the execution or throw an error. This could lead to silent failures in a deployment pipeline. Consider throwing an error or exiting the process with a non-zero status code to ensure that deployment failures are properly signaled.
- if (receipt.status !== 1) {
- console.log("Failed to set L1CrossChainMessenger in management contract");
- }
+ if (receipt.status !== 1) {
+ throw new Error("Failed to set L1CrossChainMessenger in management contract");
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (receipt.status !== 1) { | |
| console.log("Failed to set L1CrossChainMessenger in management contract"); | |
| } | |
| if (receipt.status !== 1) { | |
| throw new Error("Failed to set L1CrossChainMessenger in management contract"); | |
| } |
…ddresses-on-deploy
| -l2_private_key=8dfb8083da6275ae3e4f41e3e8a8c19d028d32c9247e24530933782f2a05035b \ | ||
| -l2_hoc_private_key=6e384a07a01263518a09a5424c7b6bbfc3604ba7d93f47e3a455cbdd7f9f0682 \ | ||
| -l2_poc_private_key=4bfe14725e685901c062ccd4e220c61cf9c189897b6c78bd18d7f51291b2b8f8 \ | ||
| -management_contract_addr=${{ needs.build.outputs.MGMT_CONTRACT_ADDR }} \ | ||
| -message_bus_contract_addr=${{ needs.build.outputs.MSG_BUS_CONTRACT_ADDR }} \ | ||
| -docker_image=${{ vars.L2_HARDHATDEPLOYER_DOCKER_BUILD_TAG }} \ | ||
| -faucet_funds=${{ vars.FAUCET_INITIAL_FUNDS }} |
There was a problem hiding this comment.
The addition of the -management_contract_addr and -message_bus_contract_addr flags to the l2contractdeployer command is a good implementation to pass the management contract address and message bus contract address to the deployment script. However, ensure that the l2contractdeployer command-line interface and the corresponding configuration have been updated to support these new flags, as mentioned in the pull request summary.
Also, verify that the hardcoded private keys (-l2_private_key, -l2_hoc_private_key, -l2_poc_private_key) are placeholders and that the actual deployment uses securely managed secrets. Storing private keys in the code, especially in a public repository, poses a significant security risk.
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- testnet/launcher/l2contractdeployer/docker.go (1 hunks)
Additional comments: 1
testnet/launcher/l2contractdeployer/docker.go (1)
- 34-40: The environment variables
PREFUND_FAUCET_AMOUNT,MGMT_CONTRACT_ADDRESS, andMESSAGE_BUS_ADDRESSare being set correctly. However, ensure that themanagementContractAddressandmessageBusAddressfields are properly validated before use to prevent potential issues with the deployment process.
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
Why this change is needed
When L1Bridge and L1CrossChainMessenger contracts are deployed we want to write their addresses to the mgmt contract so they are stored in an accessible source of truth (useful for automated testing with often short-lived testnets)
What changes were made as part of this PR
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks