-
Notifications
You must be signed in to change notification settings - Fork 160
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
base: next
Are you sure you want to change the base?
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.
Thank you! And sorry for such a delayed review. I left some comments inline. Also, some code formatting seems to be off. You can run make lint
to fix this automatically.
miden/src/cli/verify.rs
Outdated
#[clap(short = 'x', long = "hash")] | ||
program_hash: String, |
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.
I would probably keep the long name as program-hash
as it is more self-describing.
miden/src/cli/verify.rs
Outdated
#[clap(short = 'p', long = "proof", value_parser)] | ||
proof_file: PathBuf, | ||
proof_file: Option<PathBuf>, |
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.
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).
miden/src/cli/verify.rs
Outdated
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); |
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.
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
.
miden/src/cli/verify.rs
Outdated
@@ -62,4 +69,26 @@ impl VerifyCmd { | |||
|
|||
Ok(()) | |||
} | |||
} | |||
|
|||
pub fn infer_defaults(&self)->(PathBuf,PathBuf){ |
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.
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?
Hi @varun-doshi - any interest in pushing this PR over the finish line? |
Hi yes apologies for the delay |
Describe your changes
Related #1421
This PR does the following:
program-hash
to-x
. No long forminfer_defaults
function which checks the directory specified by the input file passed(with-i
) to search for a.proof
and.outputs
file (only if these files are not already specified) . If these files are already specified by user, then it does not run this check.Hence the command can be called as such:
as well as such:
TODO:
Checklist before requesting a review
next
according to naming convention.