Contract deploy: record L2 contract addresses #1667
Conversation
WalkthroughThe changes across the deployment scripts for a blockchain project focus on improving code clarity and extending functionality. Variable names have been made more descriptive, additional operations for setting contract addresses have been included, and there is now logging for the deployment of a Layer 2 bridge. Additionally, a minor formatting fix was applied to a console log statement, and new logic was added to interact with a management contract during the deployment of a cross-chain messenger. Changes
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: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- contracts/deployment_scripts/bridge/001_deploy_bridge.ts (2 hunks)
- contracts/deployment_scripts/messenger/layer1/001_deploy_cross_chain_messenger.ts (1 hunks)
- contracts/deployment_scripts/messenger/layer2/001_deploy_cross_chain_messenger.ts (2 hunks)
Files skipped from review due to trivial changes (2)
- contracts/deployment_scripts/bridge/001_deploy_bridge.ts
- contracts/deployment_scripts/messenger/layer1/001_deploy_cross_chain_messenger.ts
Additional comments: 1
contracts/deployment_scripts/messenger/layer2/001_deploy_cross_chain_messenger.ts (1)
- 28-47: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [11-47]
The
busAddressis currently hardcoded, as noted by the TODO comment. Ensure that this is addressed before production deployment to avoid potential issues with address mismatches in different environments.
| } = hre; | ||
| // Use the contract addresses from the management contract deployment. | ||
| const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS!! |
There was a problem hiding this comment.
The non-null assertion operator !! is used without checking if MGMT_CONTRACT_ADDRESS is actually set, which could lead to runtime errors if the environment variable is not provided. Consider adding a check to ensure the variable is set before using it.
- const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS!!
+ const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS;
+ if (!mgmtContractAddress) {
+ throw new Error('MGMT_CONTRACT_ADDRESS is not set');
+ }Committable 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 mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS!! | |
| const mgmtContractAddress = process.env.MGMT_CONTRACT_ADDRESS; | |
| if (!mgmtContractAddress) { | |
| throw new Error('MGMT_CONTRACT_ADDRESS is not set'); | |
| } |
| const tx = await mgmtContract.SetImportantContractAddress("L2CrossChainMessenger", crossChainDeployment.address); | ||
| const receipt = await tx.wait(); | ||
| if (receipt.status !== 1) { | ||
| console.log("Failed to set L2CrossChainMessenger in management contract"); |
There was a problem hiding this comment.
The error message in the console.log statement could be more informative by including the transaction hash or additional details to aid in debugging.
- console.log("Failed to set L2CrossChainMessenger in management contract");
+ console.log(`Failed to set L2CrossChainMessenger in management contract, transaction hash: ${tx.hash}`);Committable 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.
| console.log("Failed to set L2CrossChainMessenger in management contract"); | |
| console.log(`Failed to set L2CrossChainMessenger in management contract, transaction hash: ${tx.hash}`); |
| const crossChainDeployment = await deployments.deploy('CrossChainMessenger', { | ||
| from: deployer, | ||
| args: [ busAddress ], | ||
| log: true, | ||
| }); |
There was a problem hiding this comment.
There is no error handling for the deployment of CrossChainMessenger. If the deployment fails, the script will continue to execute, potentially leading to misleading logs or further errors. Consider adding error handling after the deployment attempt.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- contracts/deployment_scripts/bridge/001_deploy_bridge.ts (2 hunks)
- contracts/deployment_scripts/messenger/layer1/001_deploy_cross_chain_messenger.ts (1 hunks)
- contracts/deployment_scripts/messenger/layer2/001_deploy_cross_chain_messenger.ts (3 hunks)
Files skipped from review due to trivial changes (2)
- contracts/deployment_scripts/bridge/001_deploy_bridge.ts
- contracts/deployment_scripts/messenger/layer1/001_deploy_cross_chain_messenger.ts
Additional comments: 1
contracts/deployment_scripts/messenger/layer2/001_deploy_cross_chain_messenger.ts (1)
- 15-15: Verify whether the
companionNetworksvariable is used elsewhere in the code. If not, consider removing it to avoid confusion and maintain clean code.
BedrockSquirrel
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
BedrockSquirrel
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
Why this change is needed
We should record and serve the contract addresses for L2 bridge and cross-chain messenger.
What changes were made as part of this PR
Deployer writes new contract addresses to mgmt contract for reference.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks