-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: reorganize cli params #432
feat: reorganize cli params #432
Conversation
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.
Thanks Marko, much cleaner! I just have some comments (non-blocking) open to discussion, other LGTM.
@@ -20,9 +22,11 @@ use tracing::info; | |||
use zero_bin_common::fs::generate_block_proof_file_name; | |||
|
|||
#[derive(Debug, Clone, Copy)] | |||
pub struct ProverParams { | |||
pub struct ProverConfig { |
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.
Do we actually need to have a ProverConfig
anymore, given that it is semantically identical to CliProverConfig
, and that all fields are guaranteed to be initialized?
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.
@Nashtare I don't much like the idea of clap
structure (CliProverConfig
) being passed as an argument deep in the stack, but yeah, we can remove ProverConfig
.
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.
Yeah I guessed you'd answer something along these lines 🙃
I'm not strongly opinionated, we can keep the distinction.
387c328
to
098fe75
Compare
e09916f
into
feat/optimize-segment-proof-aggregation
Resolves #426