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.

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/build all casper circuits #271

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

NikolayKostadinov21
Copy link
Contributor

No description provided.

Copy link
Contributor

@Owliie Owliie left a comment

Choose a reason for hiding this comment

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

Add README for usage

let command_line_arguments: Vec<String> = args().skip(1).collect();

if command_line_circuit.name != Circuits::all {
for (_, arg) in command_line_arguments.iter().enumerate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

enumerate is useless if you are not using the index

for (_, arg) in command_line_arguments.iter().enumerate() {
let arg_as_circuit = Circuits::from_str(&arg);
build_circuit(arg_as_circuit.unwrap());
let path = format!("build/{}", arg_as_circuit.unwrap().to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

handle the None case

Comment on lines +83 to +86
let command_line_circuit: CommandLineCircuit = CommandLineCircuit::parse();
let command_line_arguments: Vec<String> = args().skip(1).collect();

if command_line_circuit.name != Circuits::all {
Copy link
Contributor

Choose a reason for hiding this comment

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

You are only using command_line_circuit to check if it's all and are not using it to determine which circuits you should build. You could just as well check if args()[1] is "all"

circuit.save(&path, &gate_serializer, &hint_serializer);
}
} else {
for _circuit in Circuits::iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove underscore

#[derive(Debug, Eq, Hash, PartialEq, Copy, Clone, EnumString, Display, EnumIter)]
#[allow(non_camel_case_types)]
enum Circuits {
compute_shuffled_index_mainnet,
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename these to Pascal case

}

impl Circuits {
fn from_str(circuit_as_str: &str) -> Option<Circuits> {
Copy link
Contributor

Choose a reason for hiding this comment

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

EnumString already creates this function for you so this implementation is redundant. Moreover, you can provide casing in it to match the Pascal case of the enum corectly

let circuit = builder.build();
let hint_serializer = HintRegistry::<L, D>::new();
let gate_serializer = GateRegistry::<L, D>::new();
let command_line_circuit: CommandLineCircuit = CommandLineCircuit::parse();
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename CommandLineCircuit to CommandLineOptions

Lazy::force(&circuit_weigh_justification_and_finalization);
}))
}
Circuits::all => OneOrAllCircuits::AllCircuits(vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

This is never called

Comment on lines +48 to +60
fn build_circuit(circuit: Circuits) -> OneOrAllCircuits {
match circuit {
Circuits::compute_shuffled_index_mainnet => OneOrAllCircuits::OneCircuit(Box::new(|| {
Lazy::force(&circuit_mainnet);
})),
Circuits::compute_shuffled_index_minimal => OneOrAllCircuits::OneCircuit(Box::new(|| {
Lazy::force(&circuit_minimal);
})),
Circuits::weigh_justification_and_finalization => {
OneOrAllCircuits::OneCircuit(Box::new(|| {
Lazy::force(&circuit_weigh_justification_and_finalization);
}))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Return type of build_circuit is never used so it can be removed. Then OneOrAllCircuits becomes obsolete and the Box::new expression can be removed. Simply match the enum value and Lazy::force in the body of the matched value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants