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

[fix]: VerifyCmd Flag Collision #1513

Open
wants to merge 2 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 37 additions & 8 deletions miden/src/cli/verify.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::{path::PathBuf, time::Instant};
use std::path::Path;

use assembly::diagnostics::{IntoDiagnostic, Report, WrapErr};
use clap::Parser;
Expand All @@ -17,33 +18,39 @@ pub struct VerifyCmd {
output_file: Option<PathBuf>,
/// Path to proof file
#[clap(short = 'p', long = "proof", value_parser)]
proof_file: PathBuf,
proof_file: Option<PathBuf>,
Comment on lines 20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep proof_file as required and try to infer input/output files based on it (rather than using input file to infer proof/output file).

/// Program hash (hex)
#[clap(short = 'h', long = "program-hash")]
#[clap(short = 'x', long = "hash")]
program_hash: String,
Comment on lines +23 to 24
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably keep the long name as program-hash as it is more self-describing.

}

impl VerifyCmd {
pub fn execute(&self) -> Result<(), Report> {
pub fn execute(&mut self) -> Result<(), Report> {

let (proof_file,output_file)=self.infer_defaults();

self.proof_file=Some(proof_file);
self.output_file=Some(output_file);
Comment on lines +28 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need to make self mutable here. Once we get the inferred files we can probably use them directly rather than reading them from self.


println!("===============================================================================");
println!("Verifying proof: {}", self.proof_file.display());
println!("Verifying proof: {}", self.proof_file.as_ref().unwrap().display());
println!("-------------------------------------------------------------------------------");

// read program hash from input
let program_hash = ProgramHash::read(&self.program_hash).map_err(Report::msg)?;

// load input data from file
let input_data = InputFile::read(&self.input_file, &self.proof_file)?;
let input_data = InputFile::read(&self.input_file, &self.proof_file.as_ref().unwrap().clone())?;

// fetch the stack inputs from the arguments
let stack_inputs = input_data.parse_stack_inputs().map_err(Report::msg)?;

// load outputs data from file
let outputs_data =
OutputFile::read(&self.output_file, &self.proof_file).map_err(Report::msg)?;
OutputFile::read(&self.output_file, &self.proof_file.as_ref().unwrap()).map_err(Report::msg)?;

// load proof from file
let proof = ProofFile::read(&Some(self.proof_file.clone()), &self.proof_file)
let proof = ProofFile::read(&Some(self.proof_file.as_ref().unwrap().to_path_buf()), &self.proof_file.as_ref().unwrap())
.map_err(Report::msg)?;

let now = Instant::now();
Expand All @@ -62,4 +69,26 @@ impl VerifyCmd {

Ok(())
}
}

pub fn infer_defaults(&self)->(PathBuf,PathBuf){
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in another comment, I'd probably infer input/output files based on the proof file rather than the other way around.

Also, I don't think this function needs to be public, right?

let input_file = self.input_file.clone().unwrap_or_else(|| {
PathBuf::from("default_input_file.txt")
});

let base_name = input_file.file_stem().expect("Invalid input file").to_str().unwrap();

let proof_file = self.proof_file.clone().unwrap_or_else(|| {
let mut proof_path = input_file.parent().unwrap_or_else(|| Path::new(".")).to_path_buf();
proof_path.push(format!("{}.proof", base_name));
proof_path
});

let output_file = self.output_file.clone().unwrap_or_else(|| {
let mut output_path = input_file.parent().unwrap_or_else(|| Path::new(".")).to_path_buf();
output_path.push(format!("{}.outputs", base_name));
output_path
});

return (proof_file,output_file);
}
}
6 changes: 3 additions & 3 deletions miden/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ pub enum Actions {

/// CLI entry point
impl Cli {
pub fn execute(&self) -> Result<(), Report> {
match &self.action {
pub fn execute(&mut self) -> Result<(), Report> {
match &mut self.action {
Actions::Analyze(analyze) => analyze.execute(),
Actions::Compile(compile) => compile.execute(),
Actions::Bundle(compile) => compile.execute(),
Expand All @@ -55,7 +55,7 @@ impl Cli {
/// Executable entry point
pub fn main() -> Result<(), Report> {
// read command-line args
let cli = Cli::parse();
let mut cli = Cli::parse();

initialize_diagnostics();

Expand Down
Loading