feat: accept _ digit separators in integer arguments - #2685
Conversation
Integer contract-invoke arguments (i32/u32/i64/u64/i128/u128/i256/u256) now accept `_` as a digit separator, mirroring Rust numeric literals, e.g. `--amount 1_000_000_000`. Previously `100_0000000` failed to parse. Stripping happens once at the `from_string` choke-point, gated to integer types only, so `_` is preserved in Symbol/String args (negative-control tests included). Closes stellar#2447
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds support for using _ as a digit separator in integer contract-argument inputs (e.g., 1_000_000), matching Rust-style readability while ensuring underscores remain intact for Symbol and String inputs.
Changes:
- Strip
_separators before JSON parsing when the targetScTypeis an integer type. - Introduce an
is_integer_typehelper to gate separator stripping to integer types only. - Add unit tests covering separator parsing for multiple integer types and ensuring underscore preservation for
Symbol/String.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let stripped; | ||
| let s = if is_integer_type(t) && s.contains('_') { | ||
| stripped = s.replace('_', ""); | ||
| stripped.as_str() | ||
| } else { | ||
| s | ||
| }; |
There was a problem hiding this comment.
Deliberate choice, documented in the PR description under design notes. At a type-committed CLI-arg position an underscore is a pure separator carrying no numeric information, so stripping can never produce a different number than the digit sequence typed — only the intended one; there's no aliasing of two distinct values. Worth noting two of the listed examples are actually valid Rust: the integer-literal grammar is DEC_DIGIT (DEC_DIGIT | _)*, so 1__2 (== 12) and 1_ (== 1) are legal literals with consecutive/trailing underscores. The genuinely non-Rust cases (__1, -_1, 1_x) either strip to an unambiguous digit sequence or still fail downstream with the existing clean type error — e.g. 1_x -> 1x -> the current parse error. Placement validation would reject inputs that have a single unambiguous meaning with no correctness or safety gain, so I'd keep strip-all as a deliberate superset of valid-Rust placements.
| let stripped; | ||
| let s = if is_integer_type(t) && s.contains('_') { | ||
| stripped = s.replace('_', ""); | ||
| stripped.as_str() | ||
| } else { | ||
| s | ||
| }; |
There was a problem hiding this comment.
Correct and fully compiler-enforced: stripped lives in the same scope as the shadowing let s, so the borrow is valid for the rest of the function, and a refactor that narrowed its scope would fail to compile (borrow of dropped value) rather than dangle — the borrow checker rules that out. The deferred-init idiom is deliberate: zero allocation on the common path (non-integer types, and integers without _) while keeping s: &str so none of the downstream consumers change. Moving to Cow<'_, str> ripples through all of those sites and changes s.to_owned() semantics (Cow::to_owned yields a Cow, not a String), so it's more churn for a stylistic gain. If readability is the concern I'm happy to extract a fn strip_int_separators<'a>(s: &'a str, t: &ScType) -> Cow<'a, str> helper that names the ownership explicitly while keeping downstream &str via .as_ref() — let me know if you'd prefer that.
| #[test] | ||
| fn test_u32_numeric_separator() { | ||
| assert_eq!( | ||
| from_string_primitive("1_000", &ScType::U32).unwrap(), | ||
| from_string_primitive("1000", &ScType::U32).unwrap() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_i64_numeric_separator() { | ||
| assert_eq!( | ||
| from_string_primitive("1_000_000", &ScType::I64).unwrap(), | ||
| from_string_primitive("1000000", &ScType::I64).unwrap() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_i128_numeric_separator() { | ||
| // The exact #2447 example. | ||
| assert_eq!( | ||
| from_string_primitive("100_0000000", &ScType::I128).unwrap(), | ||
| from_string_primitive("1000000000", &ScType::I128).unwrap() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_u128_numeric_separator() { | ||
| assert_eq!( | ||
| from_string_primitive("1_000", &ScType::U128).unwrap(), | ||
| from_string_primitive("1000", &ScType::U128).unwrap() | ||
| ); |
There was a problem hiding this comment.
Done in d31eb7a — folded the six per-type tests into a numeric_separator_tests! table macro that emits one named #[test] per row (same coverage, one line to add a type).
Copilot review: the six per-type tests were near-identical. Generate them from a numeric_separator_tests! table (one named #[test] per row) — same coverage, less duplication, easier to extend.
What
Integer contract-invoke arguments (
i32/u32/i64/u64/i128/u128/i256/u256) now accept_as a digit separator, mirroring Rust numeric literals — e.g.--amount 1_000_000_000.Why
Closes #2447. Large amounts like
1000000000are hard to read and error-prone (assets are often offset by 1e7). Today100_0000000fails withExpected type i128 … received: '100_0000000'.Design notes (pre-empting review)
from_stringchoke-point, gated to integer types only._is a legalSymbol/Stringcharacter and is preserved there (negative-control tests included)._only to disambiguate literals from identifiers in its lexer; at a type-committed CLI-arg position that constraint carries no information. Every valid-Rust separator placement works (this is a superset), and degenerate inputs like1_xstrip to1xand still fail with the existing clean error.[1_000]) are out of scope (JSON forbids_in numbers); can follow up if wanted.Tests
Separator parsing for u32/i64/i128 (the exact #2447 example)/u128/u256/i256;
_preserved forSymbolandString.cargo test -p soroban-spec-toolsandcargo clippy -p soroban-spec-tools --all-targetsare green.