-
Notifications
You must be signed in to change notification settings - Fork 95
refactor(l2): improve L2 deployer #4014
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.
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
base: main
Are you sure you want to change the base?
Conversation
Lines of code reportTotal lines added: Detailed view
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the L2 deployer to simplify its CLI interface and improve usability. The changes restructure the deployer command-line arguments, remove unused code, and standardize environment variable naming patterns.
- Restructures deployer CLI arguments for better organization and usability
- Removes unused test data I/O utilities and refactors code organization
- Updates environment variable naming for consistency across the codebase
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
cmd/ethrex/l2/deployer.rs | Major refactor of deployer CLI interface, argument structure, and contract deployment logic |
cmd/ethrex/l2/command.rs | Updates to use new Signer interface for contract calls |
cmd/ethrex/l2/options.rs | Adds default RPC URL value to eth options |
crates/l2/utils/test_data_io.rs | Completely removes unused test data I/O utilities |
crates/l2/utils/mod.rs | Removes reference to deleted test_data_io module |
crates/l2/sdk/src/sdk.rs | Updates call_contract function signature and removes unused initialize_contract function |
crates/l2/sequencer/utils.rs | Makes DEV_MODE_ADDRESS constant public |
Multiple config files | Updates environment variable names and path references for consistency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -677,7 +647,8 @@ fn deploy_tdx_contracts( | |||
Command::new("make") | |||
.arg("deploy-all") | |||
.env("PRIVATE_KEY", hex::encode(opts.private_key.as_ref())) | |||
.env("RPC_URL", &opts.rpc_url) | |||
// TODO: This can panic | |||
.env("RPC_URL", &opts.eth_options.rpc_url[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can panic if opts.eth_options.rpc_url
is empty. The TODO comment acknowledges this issue but it should be addressed. Consider using .get(0)
and handling the None case or adding validation earlier in the function.
.env("RPC_URL", &opts.eth_options.rpc_url[0]) | |
.env( | |
"RPC_URL", | |
opts.eth_options.rpc_url.get(0).ok_or_else(|| { | |
DeployerError::DeploymentSubtaskFailed( | |
"No RPC URL provided in eth_options.rpc_url".to_string(), | |
) | |
})?, | |
) |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
// Salt to use with CREATE2 deterministic deployer. Defaults to random. | ||
ETHREX_DEPLOYER_DETERMINISTIC_SALT=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the true
be something like <bytes>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks for the catch. 0b0d13c
Motivation
Current deployer is meant to be used in development only and is hard to use for people without much context.
Description
Simplify deployer CLI and review defaults. Refactor code to remove unused and unneeded code.
TDX deployment is still pending to refactor, will be addressed in another PR