Skip to content

feat(cli): parse and serialize Vec<String> args#189

Open
Thompsonmina wants to merge 1 commit into
logos-co:mainfrom
Thompsonmina:cli-vec-string
Open

feat(cli): parse and serialize Vec<String> args#189
Thompsonmina wants to merge 1 commit into
logos-co:mainfrom
Thompsonmina:cli-vec-string

Conversation

@Thompsonmina

@Thompsonmina Thompsonmina commented May 11, 2026

Copy link
Copy Markdown

Problem

spel-cli cannot parse Vec<String> arguments from the command line. The macro already emits the correct IDL shape (Vec { vec: Primitive("string") }) for any guest instruction parameter typed Vec<String>, and DynamicValue::Str already exists, so single-string args work today. But parse_vec in spel-cli/src/parse.rs has no arm for Primitive("string"), so the input falls through to ParsedValue::Raw and to_dynamic_value in spel-cli/src/serialize.rs rejects it:

❌ Serialization error: type mismatch:
   expected Vec { vec: Primitive("string") },
   got Raw("bafybeiTestSingleCid000000000000000000000001")

This blocks any LEZ program whose instruction takes a list of identifiers, names, or other string-keyed inputs.

Solution

Three small additive changes — no existing behaviour is altered:

  1. spel-cli/src/parse.rs — add a StringVec(Vec<String>) variant to ParsedValue, plus the matching Display arm and a new arm in parse_vec for Primitive("string") / Primitive("String"). Format is comma-separated, mirroring how Vec<u8>, Vec<u32>, and Vec<[u8; 32]> are already specified.
  2. spel-cli/src/serialize.rs — add an arm to to_dynamic_value that turns ParsedValue::StringVec(strs) into DynamicValue::Seq(strs.map(DynamicValue::Str)). DynamicValue::Str already serialises to risc0 serde via serialize_str, so the wire format follows automatically.
  3. spel-cli/src/cli.rs — one extra line in the TYPE FORMATS help block.

Verification

cargo test -p spel
# ... 83 passed; 0 failed

New tests:

  • parse::tests::vec_string_parses_csv — CSV → StringVec.
  • parse::tests::vec_string_empty_input_yields_empty_vec.
  • parse::tests::vec_string_single_element_yields_singleton.
  • serialize::tests::to_dynamic_value_vec_string_emits_seq_of_strStringVecDynamicValue::Seq[Str, …].
  • serialize::tests::serde_roundtrip_vec_stringthe critical contract test: CLI-emitted bytes deserialise back through risc0's Deserializer as Vec<String>, the same code the guest runs.

Known limitations

Elements containing commas are not supported (CSV format limitation). A follow-up PR can add quoting for general-purpose string vectors.

Checklist

  • Builds cleanly (cargo build)
  • Tests pass (cargo test -p spel — 83 passing)
  • README updated — N/A (no user-facing surface change beyond one help-text line, included in this PR)
  • New public methods have doc comments — N/A (no new public items)
  • Branch is off main

Update — flag repetition (per review thread)

Switched from CSV (--cids a,b,c) to flag repetition (--cid a --cid b --cid c) per the
discussion above. The on-the-wire format is unchanged — ParsedValue::StringVec
DynamicValue::Seq(Str…) → risc0 serde, same bytes as before. What changed is the entry
path:

  • parse_instruction_args now returns HashMap<String, Vec<String>>. Each --key value
    appends; scalar callers read .last() (same last-write-wins semantics as before), and
    Vec<String> args consume the whole vec.
  • New parse_string_vec(values: &[String]) -> ParsedValue is the typed entry point for
    repeated flags. The CSV arm for Primitive("string"|"String") is removed from
    parse_vec; the StringVec variant and its Display/serialize paths are unchanged.
  • tx.rs dispatches on Vec<String> shape at the parse call site (is_vec_string
    helper); every other type goes through the unchanged scalar parse_value path.

Test changes

The three CSV-format unit tests in parse.rs (vec_string_parses_csv,
vec_string_empty_input_yields_empty_vec, vec_string_single_element_yields_singleton)
are replaced with four repetition-flow tests:

  • parse_string_vec_multiple_values
  • parse_string_vec_empty_input_yields_empty_vec
  • parse_string_vec_single_element_yields_singleton
  • parse_string_vec_preserves_commas_in_elements

to_dynamic_value_vec_string_emits_seq_of_str and serde_roundtrip_vec_string in
serialize.rs updated to construct input via parse_string_vec(&[…]) instead of
parse_value("foo,bar,baz", …). Wire-format assertions unchanged.

cargo test -p spel
# 84 passed; 0 failed

@vpavlin

vpavlin commented May 12, 2026

Copy link
Copy Markdown
Collaborator

Question is - is it better to have comma-separated list in a single cli flag, or figure out and allow repetition?

--numbers 1,2,3

vs.

--number 1 --number 2 --number 3

The latter is more verbose a feels a bit cleaner

Thoughs @Thompsonmina @0x-r4bbit ?

@0x-r4bbit

Copy link
Copy Markdown
Contributor

Prefer the latter as well.

@Thompsonmina

Thompsonmina commented May 14, 2026

Copy link
Copy Markdown
Author

yeah, i agree latter is a bit clearer intention wise, although i personally prefer former as its a bit more succinct . But the latter also avoids any clunkiness with commas so i agree that we should probably go with that.

@vpavlin

vpavlin commented May 18, 2026

Copy link
Copy Markdown
Collaborator

Hey @Thompsonmina — the direction is clear from the thread: repeated flags (--number 1 --number 2 --number 3) rather than CSV. Could you update the PR to use that approach? Once updated, CI should be quick to re-run and we can merge.

Thompsonmina added a commit to Thompsonmina/spel that referenced this pull request May 18, 2026
Per review feedback on logos-co#189: --foo a --foo b --foo c rather than
--foos a,b,c. parse_instruction_args now returns Vec<String> per key;
scalar callers take .last(), Vec<String> args take the whole vec.
Elements may now contain commas.
@Thompsonmina

Copy link
Copy Markdown
Author

Done! should probably rebase onto the new main right?

@Thompsonmina Thompsonmina changed the title feat(cli): parse and serialize Vec<String> args from CSV input feat(cli): parse and serialize Vec<String> args May 18, 2026
@vpavlin

vpavlin commented May 25, 2026

Copy link
Copy Markdown
Collaborator

Yeah, this seems to have diverged quite a bit:)

warfield2016 added a commit to warfield2016/whistleblower that referenced this pull request May 27, 2026
Inside the Docker dev container:
- lgs new probe --template lez-framework (instant, LEZ cached)
- lgs setup (~3 min compile of LEZ + spel in container)
- lgs doctor: 19 PASS / 5 WARN / 0 FAIL — all WARNs expected
- sequencer (24.7MB) + wallet (21.2MB) binaries present

SPEL pin resolves cleanly in container (different pin than the native
attempt — scaffold's resolution may differ by lgs version or container
git context).

Also: competitor PR analysis updated with active review intel:
- weboko left 3 concrete blockers 2026-05-26
- Thompsonmina responded 2026-05-27
- Issue 1 (SPEL fork dependency) requires upstream merge of
  logos-co/spel#189 — outside their control
- Issue 2 (hardcoded program_id) is the exact clean-clone failure
  mode our phase 7.1 defends against
- Issue 3 (CIDs parsing bug) was real, just patched

Their submission has weeks of overhead minimum. Window remains real
but narrower than the prior 'stalled 6 days' read.

Next: verify localnet start + build + deploy inside container (phase 1.7).
warfield2016 added a commit to warfield2016/whistleblower that referenced this pull request May 27, 2026
Adopts path C (hand-rolled nssa_core) after discovering both other
framework options have issues:
- lez-framework: canonical scaffold template doesn't compile against
  current LEZ pin (6 errors including missing write_nssa_outputs_with_chained_call,
  E0282 type-inference failures)
- SPEL: depends on competitor's open upstream PR (logos-co/spel#189)

The hand-rolled path mirrors LEZ's own canonical examples
(examples/program_deployment/methods/guest/src/bin/hello_world.rs)
which definitely compile against the same nssa_core pin.

Added:
- methods/Cargo.toml — separate Cargo workspace (excluded from root)
  with edition 2024, resolver 3, nssa/nssa_core git pinned to LEZ
  rev 35d8df0d... (the scaffold's current default)
- methods/build.rs — risc0_build::embed_methods()
- methods/src/lib.rs — re-exports CHRONICLE_REGISTRY_ELF + _ID for
  host crates that want compile-time program ID
- methods/guest/Cargo.toml — guest crate with nssa_core, borsh, risc0-zkvm
- methods/guest/src/bin/chronicle_registry.rs — the actual program:
  * read_nssa_inputs::<Instruction>() for I/O
  * BorshDeserialize::try_from_slice for state load
  * Match dispatch on Instruction (InitRegistry, IndexBatch)
  * AccountPostState::new_claimed_if_default(account, Claim::Authorized)
  * ProgramOutput::new(...).write() for commit
  * Wire types inlined (Instruction, EntryRequest, RegistryEntry) because
    cargo-risczero's docker build context can't see registry-core via
    path = '../../crates/registry-core' — docs/INTEGRATION_NOTES.md
    explains this constraint
- Root Cargo.toml: exclude 'methods' (and 'web-demo') so root workspace
  stays edition 2021 / resolver 2

Phase 1 marked completed (Docker toolchain works). Phase 2 in progress
— guest binary written, debugging Rust version skew on transitive
deps (risc0's bundled rustc 1.88-dev vs ruint 1.18's required 1.90).
Comment thread spel-cli/src/cli.rs
println!(" --bin-<NAME> <FILE> Additional program binary (auto-fills --<NAME>-program-id)");
println!();
println!("COMMANDS:");
println!(" program-id <FILE> [--format hex|json] Extract ProgramId from ELF binary(ies)");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is wrong - we want to keep the program-id flag:)

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.

Good catch, wasn't intentional. When I rebased onto current main must have carried that from the old head. Will fix now

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds first-class CLI support for instruction args typed as Vec<String> by collecting repeated --flag value occurrences, parsing them into a typed ParsedValue::StringVec, and serializing them to risc0 serde as a sequence of strings.

Changes:

  • Introduces ParsedValue::StringVec plus parse_string_vec(values: &[String]) as the typed entry point for repeated flags.
  • Extends risc0 serialization to convert Vec<String> args into DynamicValue::Seq(DynamicValue::Str(..)) (with contract tests).
  • Updates tx execution flow to accept HashMap<String, Vec<String>> args and dispatch Vec<String> parsing based on the IDL type.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
spel-cli/src/tx.rs Switches instruction arg collection to multi-value map and routes Vec<String> through repeated-flag parsing.
spel-cli/src/serialize.rs Adds serialization support + tests for ParsedValue::StringVec → risc0 sequence of strings.
spel-cli/src/parse.rs Adds StringVec variant + parse_string_vec helper and tests for repeated-flag behavior.
spel-cli/src/cli.rs Changes instruction arg parsing to accumulate repeated flags and updates help text (needs correction).
Comments suppressed due to low confidence (1)

spel-cli/src/cli.rs:110

  • parse_instruction_args checks map.contains_key("h"), but the parser only recognizes args starting with --, so -h will be ignored and won’t trigger per-instruction help. Either parse -h explicitly, or drop the "h" check so the behavior matches what the parser can actually produce.
    while i < args.len() {
        if args[i].starts_with("--") {
            let key = args[i][2..].to_string();
            if i + 1 < args.len() && !args[i + 1].starts_with("--") {
                map.entry(key).or_default().push(args[i + 1].clone());
                i += 2;
            } else {
                map.entry(key).or_default().push("true".to_string());
                i += 1;
            }
        } else {
            i += 1;
        }
    }

    if map.contains_key("help") || map.contains_key("h") {
        print_instruction_help(ix);
        std::process::exit(0);
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread spel-cli/src/cli.rs Outdated
println!("COMMANDS:");
println!(" program-id <FILE> [--format hex|json] Extract ProgramId from ELF binary(ies)");
println!(" inspect <ACCOUNT-ID> --type <TYPE> Decode account data");
println!(" inspect <FILE> [FILE...] Print ProgramId for ELF binary(ies)");
Comment thread spel-cli/src/cli.rs
Comment on lines 93 to 101
if args[i].starts_with("--") {
let key = args[i][2..].to_string();
if i + 1 < args.len() && !args[i + 1].starts_with("--") {
map.insert(key, args[i + 1].clone());
map.entry(key).or_default().push(args[i + 1].clone());
i += 2;
} else {
map.insert(key, "true".to_string());
map.entry(key).or_default().push("true".to_string());
i += 1;
}
parse_instruction_args now returns HashMap<String, Vec<String>>: each
--key value appends. Scalar callers take .last() (last-write-wins
semantics unchanged); Vec<String> args take the whole vec via the new
parse_string_vec entry point. Elements may contain commas.

Other Vec types (Vec<u8>, Vec<u32>, Vec<[u8;32]>) keep the existing
CSV grammar.

cargo test -p spel — 84 passing.
@weboko

weboko commented Jun 6, 2026

Copy link
Copy Markdown

browsed through and looks good to me
@vpavlin anything blocking on your side?

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.

5 participants