Introduce CLI command #5
Conversation
This is first step towards CLI command of state reconstruction tool.
There was a problem hiding this comment.
I choose the builder style since I don't anticipate that many sub-commands or arguments and they are pretty self-descriptive this way.
There was a problem hiding this comment.
Gotcha, to be honest I'm not a huge fan of the builder style in general since I think it's a lot harder to understand how everything relates even when we don't have a lot of commands. However, if it's what you prefer I'm fine with it - I know that I'll be integrating database source arguments, but other than that I can't imagine many more arguments/sub-commands for now.
| let start_block = args.get_one::<u64>("start-block").expect("required"); | ||
| let block_step = args.get_one::<u64>("block-step").expect("required"); | ||
| println!("import from L1, starting from block number {}, processing {} blocks at a time", start_block, block_step); | ||
| // TODO(tuommaki): Implement block fetch logic. |
There was a problem hiding this comment.
I'll build this in small steps 🙂
There was a problem hiding this comment.
Gotcha, to be honest I'm not a huge fan of the builder style in general since I think it's a lot harder to understand how everything relates even when we don't have a lot of commands. However, if it's what you prefer I'm fine with it - I know that I'll be integrating database source arguments, but other than that I can't imagine many more arguments/sub-commands for now.
src/main.rs
Outdated
| .arg( | ||
| arg!(--"start-block" <START_BLOCK>) | ||
| .help("Ethereum block number to start state import from") | ||
| .default_value("16627460") |
There was a problem hiding this comment.
| .default_value("16627460") | |
| .default_value(ZKSYNC_GENESIS_BLOCK) |
The default value should be a constant.
src/main.rs
Outdated
| .arg( | ||
| arg!(--"block-step" <BLOCK_STEP>) | ||
| .help("Number of blocks to filter & process in one step") | ||
| .default_value("128") |
There was a problem hiding this comment.
| .default_value("128") | |
| .default_value(DEFAULT_BLOCK_STEP) |
src/main.rs
Outdated
| Command::new("import") | ||
| .about("Import state") |
There was a problem hiding this comment.
| Command::new("import") | |
| .about("Import state") | |
| Command::new("reconstruct") | |
| .about("reconstruct state") |
Nit: maybe "reconstruct" since we are doing a lot more than just importing the L1 state.
There was a problem hiding this comment.
Initially my thinking was that there would be export as well and in that case, I don't think deconstruct is necessarily that sound expression for the behaviour 😄
...but, revisiting requirements, I think officially there won't be export functionality so I'm ok with this. I'm thinking of hidden export command though, because we do need it for development purposes.
src/main.rs
Outdated
| let matches = cli().get_matches(); | ||
|
|
||
| match matches.subcommand() { | ||
| Some(("import", sub_matches)) => { |
There was a problem hiding this comment.
| Some(("import", sub_matches)) => { | |
| Some((Command::Reconstruct, sub_matches)) => { |
Do you know if there's any way to have the commands and sub-commands as Enums, like we can with the #[derive(Parser)] method? Would make it easier to maintain in the future.
There was a problem hiding this comment.
I briefly looked at this while writing the code. You can create an enum and workaround it here, but I personally would lean towards not having that boilerplate since I don't think these few string occurrences can cause any issues here.
This is first step towards CLI command of state reconstruction tool.