Skip to content
Open
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
96 changes: 93 additions & 3 deletions cmd/soroban-cli/src/commands/contract/arg_parsing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,10 @@ fn parse_single_argument(
config,
)?);
Ok(())
} else if matches!(input.type_, ScSpecTypeDef::Option(_)) {
parsed_args.push(ScVal::Void);
Ok(())
} else if let Some(arg_path) = matches_.get_one::<PathBuf>(&fmt_arg_file_name(&name)) {
// Check the file arg before the `Option` fallback: an explicitly
// provided `--<arg>-file-path` must not be silently ignored for
// `Option<T>` parameters.
parsed_args.push(parse_file_argument(
&name,
arg_path,
Expand All @@ -260,6 +260,9 @@ fn parse_single_argument(
config,
)?);
Ok(())
} else if matches!(input.type_, ScSpecTypeDef::Option(_)) {
parsed_args.push(ScVal::Void);
Ok(())
} else {
Err(Error::MissingArgument {
arg: name,
Expand Down Expand Up @@ -1522,4 +1525,91 @@ mod tests {
"invoke help contains unexpected control characters {bad_chars:?}:\n{help:?}"
);
}

fn one_arg_fn_spec(fn_name: &str, type_: ScSpecTypeDef) -> Spec {
use stellar_xdr::{ScSpecEntry, ScSpecFunctionInputV0, ScSpecFunctionV0};
Spec(Some(vec![ScSpecEntry::FunctionV0(ScSpecFunctionV0 {
doc: "".try_into().unwrap(),
name: fn_name.try_into().unwrap(),
inputs: vec![ScSpecFunctionInputV0 {
doc: "".try_into().unwrap(),
name: "i".try_into().unwrap(),
type_,
}]
.try_into()
.unwrap(),
outputs: vec![ScSpecTypeDef::U32].try_into().unwrap(),
})]))
}

fn parse_i_arg(spec: &Spec, fn_name: &str, argv: &[&str]) -> Vec<ScVal> {
let matches = build_custom_cmd(fn_name, spec)
.expect("command should build")
.try_get_matches_from(argv)
.expect("args should parse");
let input = match spec.find_function(fn_name).unwrap().inputs.first() {
Some(input) => input.clone(),
None => panic!("function should have an input"),
};
let mut signers = Vec::new();
let mut parsed_args = Vec::new();
parse_single_argument(
&input,
&matches,
spec,
&config::Args::default(),
&mut signers,
&mut parsed_args,
)
.expect("argument should parse");
parsed_args
}

// Regression test for https://github.com/stellar/stellar-cli/issues/2432:
// a `--<arg>-file-path` for an `Option<T>` parameter was silently ignored
// and the function invoked with `None`.
#[test]
fn optional_arg_reads_value_from_file_path() {
use std::io::Write;
let spec = one_arg_fn_spec(
"opt",
ScSpecTypeDef::Option(Box::new(ScSpecTypeOption {
value_type: Box::new(ScSpecTypeDef::U32),
})),
);
let mut file = tempfile::NamedTempFile::new().unwrap();
write!(file, "127").unwrap();
let path = file.path().to_str().unwrap().to_string();
Comment on lines +1580 to +1582

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for flagging this, but I don't think these tests are flaky. NamedTempFile delegates its Write impl to a bare std::fs::File, which is unbuffered in userspace (there's no BufWriter here) — write! goes straight through write_all/write(2) to the kernel, and write!(...).unwrap() only returns Ok once every byte has been accepted by the kernel. parse_file_argument then reads the same path back with std::fs::read_to_string in the same process, so the read is served from the page cache and sees the full contents (standard POSIX read-after-write coherency). No userspace buffer sits between the two.

For the same reason, adding file.flush() wouldn't change anything: File's flush is a no-op (there's nothing to flush). sync_all() would force durability to physical disk, but that only matters for crash-safety, not for reading the bytes back within the running test. The buffering concern would apply if the write went through a BufWriter, which isn't the case here. I'm happy to add an explicit flush if the team wants it as documentation of intent, but functionally it's a no-op, so I'd lean toward keeping the tests as-is.


let parsed = parse_i_arg(&spec, "opt", &["--i-file-path", &path]);

assert_eq!(parsed, vec![ScVal::U32(127)]);
}

#[test]
fn optional_arg_omitted_defaults_to_void() {
let spec = one_arg_fn_spec(
"opt",
ScSpecTypeDef::Option(Box::new(ScSpecTypeOption {
value_type: Box::new(ScSpecTypeDef::U32),
})),
);

let parsed = parse_i_arg(&spec, "opt", &[]);

assert_eq!(parsed, vec![ScVal::Void]);
}

#[test]
fn required_arg_reads_value_from_file_path() {
use std::io::Write;
let spec = one_arg_fn_spec("req", ScSpecTypeDef::U32);
let mut file = tempfile::NamedTempFile::new().unwrap();
write!(file, "127").unwrap();
let path = file.path().to_str().unwrap().to_string();

let parsed = parse_i_arg(&spec, "req", &["--i-file-path", &path]);

assert_eq!(parsed, vec![ScVal::U32(127)]);
}
}