fix(cli): parse Vec<u64>, Vec<u128> and Vec<bool> arguments from comma-separated lists - #266
fix(cli): parse Vec<u64>, Vec<u128> and Vec<bool> arguments from comma-separated lists#266nodersteam wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new CSV vec parser currently silently drops empty elements (e.g. ",", "1,,2"), which can mask user input errors and alter the intended argument value.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends spel-cli’s IDL-aware CLI argument parsing and risc0-compatible serialization so that additional Vec<primitive> instruction arguments (Vec<u64>, Vec<u128>, Vec<bool>) can be provided as comma-separated lists and correctly serialized as length-prefixed sequences.
Changes:
- Added
ParsedValue::Seq(Vec<ParsedValue>)plus CSV parsing forVec<u64>,Vec<u128>, andVec<bool>inspel-cli/src/parse.rs. - Mapped
(Vec<T>, Seq(items))intoDynamicValue::Seqduring serialization inspel-cli/src/serialize.rs, with added serialization/round-trip tests. - Documented the new CLI formats in
README.md.
File summaries
| File | Description |
|---|---|
| spel-cli/src/parse.rs | Adds ParsedValue::Seq and parses Vec<u64>/Vec<u128>/Vec<bool> from comma-separated input. |
| spel-cli/src/serialize.rs | Converts ParsedValue::Seq into DynamicValue::Seq and adds risc0 serde shape/round-trip tests. |
| README.md | Documents the CLI input format for the newly supported vector types. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| U32Array(Vec<u32>), // [u32; N] / ProgramId | ||
| ByteArrayVec(Vec<Vec<u8>>), // Vec<[u8; 32]> | ||
| StringVec(Vec<String>), // Vec<String> | ||
| Seq(Vec<ParsedValue>), // Vec<T> for any other primitive T (u64, u128, bool) |
There was a problem hiding this comment.
Fair point. ParsedValue is reachable through the spel lib target, but nothing in the workspace consumes it outside spel-cli itself (the lib exists for the binary), and the crate is not published, so there is no downstream exhaustive match to break today. I have noted the new variant as an additive enum change in the PR description so it can be carried into the release notes; if you would rather guard the enum with #[non_exhaustive], happy to add that in this PR.
| // Vec<u64> / Vec<u128> / Vec<bool> — comma-separated values, parsed element by | ||
| // element with the primitive parser; an empty (or whitespace-only) string is an | ||
| // empty list. | ||
| IdlType::Primitive(p) if p == "u64" || p == "u128" || p == "bool" => { | ||
| let mut items = Vec::new(); | ||
| for (i, part) in raw | ||
| .split(',') | ||
| .map(str::trim) | ||
| .filter(|s| !s.is_empty()) | ||
| .enumerate() | ||
| { | ||
| let item = | ||
| parse_primitive(part, p).map_err(|e| format!("Element [{}]: {}", i, e))?; | ||
| items.push(item); | ||
| } | ||
| Ok(ParsedValue::Seq(items)) | ||
| }, |
There was a problem hiding this comment.
Agreed, that hid mistakes. de054bf changes it: a fully empty (or whitespace-only) string is still an empty list, but an empty element inside a non-empty list (",", "1,,2", "1,2,") is now an error that names the index, e.g. Element [1]: empty element in Vec<u64>. Test parse_vec_rejects_empty_elements covers u64 and bool; README row updated.
| U32Array(Vec<u32>), // [u32; N] / ProgramId | ||
| ByteArrayVec(Vec<Vec<u8>>), // Vec<[u8; 32]> | ||
| StringVec(Vec<String>), // Vec<String> | ||
| Seq(Vec<ParsedValue>), // Vec<T> for any other primitive T (u64, u128, bool) |
There was a problem hiding this comment.
Done in de054bf: the comment now reads Vec<u64> / Vec<u128> / Vec<bool>, one element per entry, matching what parse_vec actually constructs.
|
@nodersteam I think Copilot's comments are worth addressing, but otherwise it looks sensible |
|
Thanks for the look, @vpavlin. Addressed Copilot's three comments in de054bf: empty elements inside a list are now rejected with their index (fully empty string stays an empty list), the |
…a-separated lists Instruction arguments of these types fell through to ParsedValue::Raw and the transaction failed to build with a type mismatch; only Vec<u8> and Vec<u32> had list parsers. Parse the list element by element with the existing primitive parser into a new ParsedValue::Seq, serialize it as a length-prefixed sequence, and report a malformed element with its index. An empty string is an empty list. Covered by parse and serialize unit tests, including a risc0 serde round-trip through a Deserialize-derived instruction.
…ents
An empty element inside a non-empty comma-separated list ("1,,2", ",")
is now an error naming the element index instead of being silently
dropped. A fully empty string is still an empty list. Also tightens the
ParsedValue::Seq comment to the three element types that produce it.
de054bf to
f60680a
Compare
|
Rebased onto main (f60680a) to pick up the new @vpavlin the fresh CI run on the rebased head is waiting for workflow approval (fork PR) — could you hit "Approve and run" when you get a chance? Thanks. |
Five sequencer-backed jobs checked out
`github.event.pull_request.head.sha`, so they ran the PR's branch tip
while the workflow definition came from main. A PR opened before a new
test script lands therefore fails on a file it never could have had:
scripts/multisig-e2e-test.sh: No such file or directory
Process completed with exit code 127
That is #266's current red check, and every other open PR is one
commit-add away from the same thing — nine of them lack that script at
their head right now. The unit and E2E jobs never had the override, so
they went green on the same commit, which is what makes the failure
look like the contributor's fault.
The quieter half of the problem is that those jobs tested each PR
against whatever base it branched from, so a PR could pass while being
broken against current main.
`actions/checkout` with no `ref:` already does the right thing: the
merge ref on pull_request, the pushed commit on push — which is what
the `||` expression was hand-rolling, minus the staleness.
SPEL_REF has to move with it. It reaches `spel init --spel-rev` and
lands in the scaffolded guest manifest as
spel-framework = { git = "...", rev = "refs/pull/N/head" }
so it decides which framework the guest compiles against. Left at
/head while the checkout moved to /merge, a run would take its scripts
from the merged tree and its framework from the stale head — the two
halves disagreeing is worse than both being stale. Cargo resolves the
merge ref fine; verified it locks to the same commit `git ls-remote`
reports for refs/pull/N/merge.
One deliberate behaviour change: refs/pull/N/merge exists only while a
PR is conflict-free, so a conflicted PR now fails to resolve the ref
instead of testing against a base it no longer merges into. That seems
right — a green check on a conflicted PR is describing a tree nobody
will ever ship — but it is a change, not a fix.
Claude-Session: https://claude.ai/code/session_01S9qsH6Um6shweCPEN3Z6ph
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Description
spelonly had list parsers forVec<u8>andVec<u32>(parse_vecinspel-cli/src/parse.rs). Any otherVec<primitive>instruction argument fell through toParsedValue::Raw, andto_dynamic_valuethen failed withtype mismatch: expected Vec { vec: Primitive("u128") }, got Raw("...")- an instruction with, say,tranches: Vec<u128>could not be called from the CLI at all, not even with an empty list.This PR parses comma-separated lists for
Vec<u64>,Vec<u128>andVec<bool>element by element with the existing primitive parser and serializes them as a length-prefixed sequence, the same risc0 serde shape the guest deserializes. An empty (or whitespace-only) string is an empty list; a malformed element is reported with its index (Element [1]: Invalid u128 'seven'), and an empty element inside a non-empty list ("1,,2",",") is an error rather than being dropped.Verified end to end against a program whose instruction carries
tranches: Vec<u128>:spel --dry-runproduces instruction bytes identical torisc0_zkvm::serde::to_vecof the same RustInstructionvalue for both--tranches 300,700and--tranches "", and the same build created schedules on the public LEZ testnet whose on-chain state decodes to[300, 700].Changes
spel-cli/src/parse.rs: newParsedValue::Seq(Vec<ParsedValue>)(withDisplay→[300, 700]);parse_vecarm foru64/u128/boolelements.Vec<u8>/Vec<u32>keep their current representations and behaviour. Note for release notes:ParsedValueis reachable through thespellib target, so the new variant is an additive change to that enum (nothing in the workspace matches on it outsidespel-cliitself).spel-cli/src/serialize.rs:(Vec<T>, Seq(items))→DynamicValue::Seq.README.md: row in the argument-format table.parse.rs- CSV parsing foru128(incl.u128::MAX),u64,bool; empty / whitespace-only string → empty list; empty element in a non-empty list is rejected with its index; error names the offending element;Display;u8/u32unchanged.serialize.rs-Vec<u128>→ length prefix + four u32 words per element; empty vec →[0]; risc0 serde round-trip through aDeserialize-derived instruction withVec<u64>,Vec<u128>,Vec<bool>and an emptyVec<u128>.Checklist
cargo build --manifest-path spel-cli/Cargo.toml)cargo test --manifest-path spel-cli/Cargo.toml: 111 passed;cargo test --workspace --all-targets --exclude spel --exclude spel-ffi-compile-test: green;cargo fmt --all --check: clean;cargo clippyreports nothing in the changed hunks)mainReviewer note: the new
parse_vecarm returns an error on a malformed element instead of theRawfallback theu8/u32arms use (that fallback exists soserialize.rscan retry au32CSV, which the new types do not need). Happy to align either way.