-
Notifications
You must be signed in to change notification settings - Fork 34
Add save command to the examples #874
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,29 @@ | ||
| use std::{fs, path::Path}; | ||
|
|
||
| use anyhow::Result; | ||
| use binius_core::constraint_system::{ValueVec, ValuesData}; | ||
| use binius_frontend::{compiler::CircuitBuilder, stat::CircuitStat}; | ||
| use binius_utils::serialization::SerializeBytes; | ||
| use clap::{Arg, Args, Command, FromArgMatches, Subcommand}; | ||
|
|
||
| use crate::{ExampleCircuit, prove_verify, setup}; | ||
|
|
||
| /// Serialize a value implementing `SerializeBytes` and write it to the given path. | ||
| fn write_serialized<T: SerializeBytes>(value: &T, path: &str) -> Result<()> { | ||
| if let Some(parent) = Path::new(path).parent() | ||
| && !parent.as_os_str().is_empty() | ||
| { | ||
| fs::create_dir_all(parent).map_err(|e| { | ||
| anyhow::anyhow!("Failed to create directory '{}': {}", parent.display(), e) | ||
| })?; | ||
| } | ||
| let mut buf: Vec<u8> = Vec::new(); | ||
| value.serialize(&mut buf)?; | ||
| fs::write(path, &buf) | ||
| .map_err(|e| anyhow::anyhow!("Failed to write serialized data to '{}': {}", path, e))?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// A CLI builder for circuit examples that handles all command-line parsing and execution. | ||
| /// | ||
| /// This provides a clean API for circuit examples where developers only need to: | ||
|
|
@@ -74,6 +94,27 @@ enum Commands { | |
| #[command(flatten)] | ||
| params: CommandArgs, | ||
| }, | ||
|
|
||
| /// Save constraint system, public witness, and non-public data to files if paths are provided | ||
| Save { | ||
| /// Output path for the constraint system binary | ||
| #[arg(long = "cs-path")] | ||
| cs_path: Option<String>, | ||
|
|
||
| /// Output path for the public witness binary | ||
| #[arg(long = "pub-witness-path")] | ||
| pub_witness_path: Option<String>, | ||
|
|
||
| /// Output path for the non-public data (witness + internal) binary | ||
| #[arg(long = "non-pub-data-path")] | ||
| non_pub_data_path: Option<String>, | ||
|
|
||
| #[command(flatten)] | ||
| params: CommandArgs, | ||
|
|
||
| #[command(flatten)] | ||
| instance: CommandArgs, | ||
|
Comment on lines
+115
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There appears to be a duplicate command argument flattening issue in the Since both fields serve the same purpose (collecting command arguments), the /// Save constraint system, public witness, and non-public data to files if paths are provided
Save {
/// Output path for the constraint system binary
#[arg(long = "cs-path")]
cs_path: Option<String>,
/// Output path for the public witness binary
#[arg(long = "pub-witness-path")]
pub_witness_path: Option<String>,
/// Output path for the non-public data (witness + internal) binary
#[arg(long = "non-pub-data-path")]
non_pub_data_path: Option<String>,
// Remove this duplicate field
// #[command(flatten)]
// params: CommandArgs,
#[command(flatten)]
instance: CommandArgs,
}Spotted by Diamond |
||
| }, | ||
| } | ||
|
|
||
| /// Wrapper for dynamic command arguments | ||
|
|
@@ -102,13 +143,15 @@ where | |
| let composition_cmd = Self::build_composition_subcommand(); | ||
| let check_snapshot_cmd = Self::build_check_snapshot_subcommand(); | ||
| let bless_snapshot_cmd = Self::build_bless_snapshot_subcommand(); | ||
| let save_cmd = Self::build_save_subcommand(); | ||
|
|
||
| let command = command | ||
| .subcommand(prove_cmd) | ||
| .subcommand(stat_cmd) | ||
| .subcommand(composition_cmd) | ||
| .subcommand(check_snapshot_cmd) | ||
| .subcommand(bless_snapshot_cmd); | ||
| .subcommand(bless_snapshot_cmd) | ||
| .subcommand(save_cmd); | ||
|
|
||
| // Also add top-level args for default prove behavior | ||
| let command = command.arg( | ||
|
|
@@ -171,6 +214,34 @@ where | |
| E::Params::augment_args(cmd) | ||
| } | ||
|
|
||
| fn build_save_subcommand() -> Command { | ||
| let mut cmd = Command::new("save").about( | ||
| "Save constraint system, public witness, and non-public data to files if paths are provided", | ||
| ); | ||
| cmd = cmd | ||
| .arg( | ||
| Arg::new("cs_path") | ||
| .long("cs-path") | ||
| .value_name("PATH") | ||
| .help("Output path for the constraint system binary"), | ||
| ) | ||
| .arg( | ||
| Arg::new("pub_witness_path") | ||
| .long("pub-witness-path") | ||
| .value_name("PATH") | ||
| .help("Output path for the public witness binary"), | ||
| ) | ||
| .arg( | ||
| Arg::new("non_pub_data_path") | ||
| .long("non-pub-data-path") | ||
| .value_name("PATH") | ||
| .help("Output path for the non-public data (witness + internal) binary"), | ||
| ); | ||
| cmd = E::Params::augment_args(cmd); | ||
| cmd = E::Instance::augment_args(cmd); | ||
| cmd | ||
| } | ||
|
|
||
| /// Set the about/description text for the command. | ||
| /// | ||
| /// This appears in the help output. | ||
|
|
@@ -212,6 +283,7 @@ where | |
| Some(("bless-snapshot", sub_matches)) => { | ||
| Self::run_bless_snapshot_impl(sub_matches.clone(), circuit_name) | ||
| } | ||
| Some(("save", sub_matches)) => Self::run_save(sub_matches.clone()), | ||
| Some((cmd, _)) => anyhow::bail!("Unknown subcommand: {}", cmd), | ||
| None => { | ||
| // No subcommand - default to prove behavior for backward compatibility | ||
|
|
@@ -319,6 +391,54 @@ where | |
| Ok(()) | ||
| } | ||
|
|
||
| fn run_save(matches: clap::ArgMatches) -> Result<()> { | ||
| // Extract optional output paths | ||
| let cs_path = matches.get_one::<String>("cs_path").cloned(); | ||
| let pub_witness_path = matches.get_one::<String>("pub_witness_path").cloned(); | ||
| let non_pub_data_path = matches.get_one::<String>("non_pub_data_path").cloned(); | ||
|
|
||
| // If nothing to save, exit early | ||
| if cs_path.is_none() && pub_witness_path.is_none() && non_pub_data_path.is_none() { | ||
| tracing::info!("No output paths provided; nothing to save"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Parse Params and Instance | ||
| let params = E::Params::from_arg_matches(&matches)?; | ||
| let instance = E::Instance::from_arg_matches(&matches)?; | ||
|
|
||
| // Build circuit | ||
| let mut builder = CircuitBuilder::new(); | ||
| let example = E::build(params, &mut builder)?; | ||
| let circuit = builder.build(); | ||
|
|
||
| // Generate witness | ||
| let mut filler = circuit.new_witness_filler(); | ||
| example.populate_witness(instance, &mut filler)?; | ||
| circuit.populate_wire_witness(&mut filler)?; | ||
| let witness: ValueVec = filler.into_value_vec(); | ||
|
|
||
| // Conditionally write artifacts | ||
| if let Some(path) = cs_path.as_deref() { | ||
| write_serialized(circuit.constraint_system(), path)?; | ||
| tracing::info!("Constraint system saved to '{}'", path); | ||
| } | ||
|
|
||
| if let Some(path) = pub_witness_path.as_deref() { | ||
| let data = ValuesData::from(witness.public()); | ||
| write_serialized(&data, path)?; | ||
| tracing::info!("Public witness saved to '{}'", path); | ||
| } | ||
|
|
||
| if let Some(path) = non_pub_data_path.as_deref() { | ||
| let data = ValuesData::from(witness.non_public()); | ||
| write_serialized(&data, path)?; | ||
| tracing::info!("Non-public witness saved to '{}'", path); | ||
| } | ||
|
|
||
|
GraDKh marked this conversation as resolved.
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Parse arguments and run the circuit example. | ||
| /// | ||
| /// This orchestrates the entire flow: | ||
|
|
||
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.
The
if letcombined with&&creates a syntax issue that could lead to unexpected behavior. The current structure:This attempts to use
parentin the second condition, but the variable binding fromif letdoesn't properly extend through the&&operator in this context.Consider restructuring with either nested conditionals:
Or with proper parentheses to clarify the intent:
Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.