Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contract deployment: store important addresses #1653

Merged
merged 3 commits into from
Nov 23, 2023
Merged

Contract deployment: store important addresses #1653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
38110e5
Contract deployment: store important addresses
BedrockSquirrel Nov 20, 2023
26e46ed
Merge remote-tracking branch 'origin/main' into matt/write-contract-a…
BedrockSquirrel Nov 22, 2023
d50a1fd
fix env var name
BedrockSquirrel Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/manual-deploy-testnet-l2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ jobs:
-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 }}
Comment on lines 309 to 315
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
10 changes: 10 additions & 0 deletions contracts/deployment_scripts/bridge/001_deploy_bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
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();
Comment on lines 12 to 18
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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();

Expand All @@ -27,6 +28,15 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
log: true,
});

// 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}`)
Comment on lines +31 to +38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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}`)


// We get the Cross chain messenger deployment on the layer 2 network.
const messengerL2 = await deployments.get("CrossChainMessenger");

Expand Down
14 changes: 12 additions & 2 deletions contracts/deployment_scripts/messenger/layer1/001_deploy_cross_chain_messenger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,26 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {

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");
}
Comment on lines +32 to +34
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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");
}

console.log(`L1CrossChainMessenger=${crossChainDeployment.address}`)
};

export default func;
Comment on lines 13 to 38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;

Expand Down
3 changes: 3 additions & 0 deletions testnet/launcher/l2contractdeployer/cmd/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ type L2ContractDeployerConfigCLI struct {
dockerImage string
l2Host string
l2WSPort int
managementContractAddr string
messageBusContractAddr string
l2PrivateKey string
l2HOCPrivateKey string
Expand All @@ -28,6 +29,7 @@ func ParseConfigCLI() *L2ContractDeployerConfigCLI {
dockerImage := flag.String(dockerImageFlag, "testnetobscuronet.azurecr.io/obscuronet/hardhatdeployer:latest", flagUsageMap[dockerImageFlag])
l2Host := flag.String(l2HostFlag, "", flagUsageMap[l2HostFlag])
l2WSPort := flag.Int(l2WSPortFlag, 9000, flagUsageMap[l2WSPortFlag])
managementContractAddr := flag.String(managementContractAddrFlag, "", flagUsageMap[managementContractAddrFlag])
messageBusContractAddr := flag.String(messageBusContractAddrFlag, "", flagUsageMap[messageBusContractAddrFlag])
l2PrivateKey := flag.String(l2privateKeyFlag, "", flagUsageMap[l2privateKeyFlag])
l2HOCPrivateKey := flag.String(l2HOCPrivateKeyFlag, "", flagUsageMap[l2HOCPrivateKeyFlag])
Expand All @@ -41,6 +43,7 @@ func ParseConfigCLI() *L2ContractDeployerConfigCLI {
cfg.dockerImage = *dockerImage
cfg.l2Host = *l2Host
cfg.l2WSPort = *l2WSPort
cfg.managementContractAddr = *managementContractAddr
cfg.messageBusContractAddr = *messageBusContractAddr
cfg.l2PrivateKey = *l2PrivateKey
cfg.l2HOCPrivateKey = *l2POCPrivateKey
Expand Down
2 changes: 2 additions & 0 deletions testnet/launcher/l2contractdeployer/cmd/cli_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const (
dockerImageFlag = "docker_image"
l2HostFlag = "l2_host"
l2WSPortFlag = "l2_ws_port"
managementContractAddrFlag = "management_contract_addr"
messageBusContractAddrFlag = "message_bus_contract_addr"
l2privateKeyFlag = "l2_private_key"
l2HOCPrivateKeyFlag = "l2_hoc_private_key"
Expand All @@ -23,6 +24,7 @@ func getFlagUsageMap() map[string]string {
dockerImageFlag: "Docker image to run",
l2HostFlag: "Layer 2 network host addr",
l2WSPortFlag: "Layer 2 network WebSocket port",
managementContractAddrFlag: "Management contract address",
messageBusContractAddrFlag: "Message bus contract address",
l2privateKeyFlag: "Layer 2 private key",
l2HOCPrivateKeyFlag: "Layer 2 HOC contract private key",
Expand Down
1 change: 1 addition & 0 deletions testnet/launcher/l2contractdeployer/cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ func main() {
l2cd.WithL2Host(cliConfig.l2Host), // "host"
l2cd.WithL2WSPort(cliConfig.l2WSPort), // 81
l2cd.WithL1PrivateKey(cliConfig.privateKey), // "f52e5418e349dccdda29b6ac8b0abe6576bb7713886aa85abea6181ba731f9bb"
l2cd.WithManagementContractAddress(cliConfig.managementContractAddr), //
l2cd.WithMessageBusContractAddress(cliConfig.messageBusContractAddr), // "0xFD03804faCA2538F4633B3EBdfEfc38adafa259B"
l2cd.WithL2PrivateKey(cliConfig.l2PrivateKey), // "8dfb8083da6275ae3e4f41e3e8a8c19d028d32c9247e24530933782f2a05035b"
l2cd.WithHocPKString(cliConfig.l2HOCPrivateKey), // "6e384a07a01263518a09a5424c7b6bbfc3604ba7d93f47e3a455cbdd7f9f0682"),
Expand Down
27 changes: 17 additions & 10 deletions testnet/launcher/l2contractdeployer/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,17 @@ type Option = func(c *Config)

// Config holds the properties that configure the package
type Config struct {
l1HTTPURL string
l1privateKey string
l2Port int
l2Host string
l2PrivateKey string
hocPKString string
pocPKString string
messageBusAddress string
dockerImage string
faucetPrefundAmount string
l1HTTPURL string
l1privateKey string
l2Port int
l2Host string
l2PrivateKey string
hocPKString string
pocPKString string
managementContractAddress string
messageBusAddress string
dockerImage string
faucetPrefundAmount string
}

func NewContractDeployerConfig(opts ...Option) *Config {
Expand Down Expand Up @@ -53,6 +54,12 @@ func WithL2Host(s string) Option {
}
}

func WithManagementContractAddress(s string) Option {
return func(c *Config) {
c.managementContractAddress = s
}
}

func WithMessageBusContractAddress(s string) Option {
return func(c *Config) {
c.messageBusAddress = s
Expand Down
1 change: 1 addition & 0 deletions testnet/launcher/l2contractdeployer/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ func (n *ContractDeployer) Start() error {

envs := map[string]string{
"PREFUND_FAUCET_AMOUNT": n.cfg.faucetPrefundAmount,
"MGMT_CONTRACT_ADDRESS": n.cfg.managementContractAddress,
"MESSAGE_BUS_ADDRESS": n.cfg.messageBusAddress,
"NETWORK_JSON": fmt.Sprintf(`
{
Expand Down
Loading