Pedro/debug hh deployer #1710
Pedro/debug hh deployer #1710
Conversation
otherview
requested review from
tudor-malene,
StefanIliev545 and
BedrockSquirrel
WalkthroughThe recent updates involve enhancing the Changes
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 X ? 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: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- testnet/launcher/cmd/cli.go (2 hunks)
- testnet/launcher/cmd/cli_flags.go (2 hunks)
- testnet/launcher/cmd/main.go (1 hunks)
- testnet/launcher/config.go (2 hunks)
- testnet/launcher/docker.go (3 hunks)
- testnet/launcher/l1contractdeployer/config.go (2 hunks)
- testnet/launcher/l1contractdeployer/docker.go (2 hunks)
- testnet/launcher/l2contractdeployer/config.go (2 hunks)
Additional comments: 14
testnet/launcher/cmd/cli.go (2)
10-15: The addition of
contractDeployerDockerImageandcontractDeployerDebugfields to theTestnetConfigCLIstruct aligns with the PR's objective to support remote debugging for the contract deployer. This change is well-structured and follows the existing pattern of the codebase.27-37: The parsing of the new CLI flags
contractDeployerDockerImageandcontractDeployerDebugis consistent with the existing flag parsing logic. However, ensure that the default values provided for these flags are appropriate and that they are documented in the same way as other flags.testnet/launcher/cmd/cli_flags.go (2)
9-11: The new flags
contractDeployerDockerImageFlagandcontractDeployerDebugFlagare added correctly. It's important to ensure that these flags are documented and communicated to the developers who will use this CLI, as they are part of the new feature.22-23: The descriptions for the new flags are clear and concise. It's good practice to keep the flag descriptions helpful for the end-users, as seen here.
testnet/launcher/cmd/main.go (1)
- 20-21: The integration of the new contract deployer settings into the
mainfunction is done correctly. The use of option functions to pass configuration is a clean approach that keeps the code modular and easy to read.testnet/launcher/config.go (2)
14-17: The restructuring of the
Configstruct to include the newcontractDeployerDockerImageandcontractDeployerDebugfields, as well as the repositioning ofisSGXEnabled, is logical and maintains the clarity of the configuration's purpose.60-70: The addition of
WithContractDeployerDockerImageandWithContractDeployerDebugfunctions follows the established pattern for configuration option functions. This is a good use of the options pattern for dependency injection.testnet/launcher/docker.go (2)
- 33-39: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [36-43]
The conditional logic to append debug commands and ports based on the
debugEnabledflag is implemented correctly. It's important to ensure that the debug port (9229) does not conflict with other services and that it is properly secured.
- 187-193: The method
deployL1Contractsnow correctly references the configuration from theTestnetinstance. This change encapsulates the deployment logic within theTestnettype, which is a good practice for maintainability.testnet/launcher/l1contractdeployer/config.go (2)
8-11: The addition of the
debugEnabledfield to theConfigstruct is consistent with the feature requirements. It's important to validate that this new configuration is handled correctly wherever theConfigis used.42-46: The
WithDebugEnabledfunction is added to allow the configuration of the debug mode. This is a straightforward and clean implementation of the option pattern.testnet/launcher/l1contractdeployer/docker.go (1)
- 34-43: The changes to the command slice to include the node inspector for debugging are correct. However, it's crucial to ensure that exposing the debug port does not introduce security vulnerabilities, especially if the deployer runs in a shared or production-like environment.
testnet/launcher/l2contractdeployer/config.go (2)
19-19: The
debugEnabledfield is added to theConfigstruct to support the new debugging feature. This change is consistent with the rest of the codebase and the feature's requirements.100-104: The
WithDebugEnabledfunction is implemented correctly, following the established pattern for configuration options. This addition will facilitate the enabling of debugging for the contract deployer.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- contracts/deployment_scripts/core/001_deploy_management_contract.ts (1 hunks)
- testnet/launcher/l1contractdeployer/docker.go (4 hunks)
Files skipped from review due to trivial changes (1)
- contracts/deployment_scripts/core/001_deploy_management_contract.ts
Additional comments: 3
testnet/launcher/l1contractdeployer/docker.go (3)
61-64: The changes to the
StartNewContainerfunction call correctly include theportsslice to handle the new debugging feature.82-90: The logic to adjust the
tailSizebased on the debug flag is correct and ensures that the correct amount of log output is retrieved.113-116: The conditional logic to remove debugger lines from the output when debugging is enabled is correct and ensures that only relevant lines are processed.
| cmds := []string{"npx"} | ||
| var ports []int | ||
|
|
||
| // inspect stops operation until debugger is hooked on port 9229 if debug is enabled | ||
| if n.cfg.debugEnabled { | ||
| cmds = append(cmds, "--node-options=\"--inspect-brk=0.0.0.0:9229\"") | ||
| ports = append(ports, 9229) | ||
| } | ||
|
|
||
| cmds = append(cmds, "hardhat", "deploy", "--network", "layer1") | ||
|
|
There was a problem hiding this comment.
The addition of debug options to the cmds slice and the ports slice is correct and follows the intended feature implementation. However, the debug option string is not properly escaped, which could lead to command execution issues.
- cmds = append(cmds, "--node-options=\"--inspect-brk=0.0.0.0:9229\"")
+ cmds = append(cmds, "--node-options=--inspect-brk=0.0.0.0:9229")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.
| cmds := []string{"npx"} | |
| var ports []int | |
| // inspect stops operation until debugger is hooked on port 9229 if debug is enabled | |
| if n.cfg.debugEnabled { | |
| cmds = append(cmds, "--node-options=\"--inspect-brk=0.0.0.0:9229\"") | |
| ports = append(ports, 9229) | |
| } | |
| cmds = append(cmds, "hardhat", "deploy", "--network", "layer1") | |
| cmds := []string{"npx"} | |
| var ports []int | |
| // inspect stops operation until debugger is hooked on port 9229 if debug is enabled | |
| if n.cfg.debugEnabled { | |
| cmds = append(cmds, "--node-options=--inspect-brk=0.0.0.0:9229") | |
| ports = append(ports, 9229) | |
| } | |
| cmds = append(cmds, "hardhat", "deploy", "--network", "layer1") | |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- testnet/launcher/l2contractdeployer/docker.go (2 hunks)
Additional comments: 3
testnet/launcher/l2contractdeployer/docker.go (3)
- 82-85: The
docker.StartNewContainerfunction is called with the newportsparameter. Ensure that theStartNewContainerfunction has been updated accordingly to handle this new parameter without causing any regressions.Verification successful
The
StartNewContainerfunction has been successfully located in thego/common/docker/docker.gofile, and it does indeed include aports []intparameter in its signature. This confirms that the function is designed to handle theportsparameter, which aligns with the changes made in thedocker.gofile of thel2contractdeployerdirectory.Final response:
The
StartNewContainerfunction in thedocker.gofile has been verified to accept aports []intparameter, which is consistent with the changes made in thel2contractdeployerdirectory. No regressions related to this parameter addition should be expected based on this information.* 27-43: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [27-85]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the StartNewContainer function handles the new 'ports' parameter correctly. ast-grep --lang go --pattern $'func StartNewContainer($_, $_, $_, ports []int, $_, $_, $_) { $$$ }'Length of output: 102
Script:
#!/bin/bash # Search for the StartNewContainer function to verify its parameters. ast-grep --lang go --pattern $'func StartNewContainer($$$) { $$$ }'Length of output: 70
Script:
#!/bin/bash # Use ripgrep to search for the StartNewContainer function in the codebase. rg "func StartNewContainer"Length of output: 196
The
Startfunction has been refactored to include debugging logic. Ensure that the introduction of the debugging feature does not introduce any security concerns, such as exposing sensitive information through the logs or opening unnecessary network ports.
- 27-43: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [27-85]
The
Startfunction setsn.containerIDafter starting the container. Ensure that this container ID is used appropriately throughout the codebase and that there are no race conditions or synchronization issues when accessing this field.
| func (n *ContractDeployer) Start() error { | ||
| fmt.Printf("Starting L2 contract deployer with config: \n%s\n\n", litter.Sdump(*n.cfg)) | ||
|
|
||
| cmds := []string{ | ||
| "npx", "hardhat", "obscuro:deploy", | ||
| "--network", "layer2", | ||
| cmds := []string{"npx"} | ||
| var ports []int | ||
|
|
||
| // inspect stops operation until debugger is hooked on port 9229 if debug is enabled | ||
| if n.cfg.debugEnabled { | ||
| cmds = append(cmds, "--node-options=\"--inspect-brk=0.0.0.0:9229\"") | ||
| ports = append(ports, 9229) | ||
| } | ||
|
|
||
| cmds = append(cmds, "hardhat", "obscuro:deploy", "--network", "layer2") | ||
|
|
||
| envs := map[string]string{ | ||
| "PREFUND_FAUCET_AMOUNT": n.cfg.faucetPrefundAmount, | ||
| "MGMT_CONTRACT_ADDRESS": n.cfg.managementContractAddress, |
There was a problem hiding this comment.
The Start function has been updated to conditionally append command-line arguments and ports for debugging. This is a good use of conditional logic to enable optional features. However, the debug port 9229 is hardcoded. Consider defining it as a constant or configuration parameter to avoid magic numbers and facilitate changes if needed in the future.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [27-85]
The Start function prints the configuration and starts the container. However, there is no error handling for the configuration print step. If printing the configuration is critical, consider adding error handling or logging for this step.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [27-85]
The Start function's error handling for docker.StartNewContainer is straightforward, but it might be beneficial to add more context to the error before returning it, such as what was being attempted when the error occurred.
Why this change is needed
https://github.com/ten-protocol/ten-internal/issues/2643
Adds a
contract-deployer-debugflag that allows to remotely debug the hh contract deployer.What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks