fix(utils): return AmountTooLarge instead of ParseU64 on u64 overflow - #2506
fix(utils): return AmountTooLarge instead of ParseU64 on u64 overflow#2506Sertug17 wants to merge 4 commits into
Conversation
… in tokens_to_base_units When the combined integer+fractional string exceeds u64::MAX, the previous code returned TokenParseError::ParseU64 wrapping a ParseIntError, which misleadingly suggested a digit-format problem rather than an overflow. Changes: - Add TokenParseError::AmountTooLarge variant with a clear error message - Map the final combined.parse::<u64>() failure to AmountTooLarge - Add a regression test: "184467440737095516.16" with 2 decimals produces "18446744073709551616" (u64::MAX + 1) and must return AmountTooLarge The per-part pre-validation loop is kept as-is to catch non-numeric characters (ParseU64 remains correct for those cases). Fixes 0xMiden#2505
There was a problem hiding this comment.
I think this is only used by the CLI, so maybe we should move this function to the utils mod in the CLI crate (bin/miden-cli/src/utils.rs). Could you do that?
| // Validate that the parts contain only valid digits (catches non-numeric chars). | ||
| // Note: each part is validated individually, so a large combined value is NOT | ||
| // caught here — that case is handled by the AmountTooLarge error below. | ||
| for part in &parts { | ||
| part.parse::<u64>().map_err(TokenParseError::ParseU64)?; | ||
| } |
There was a problem hiding this comment.
This still reports overflow as ParseU64 when the integer part itself is too large. For example, a very large number fails here and never reaches the AmountTooLarge mapping below. I think we should check the error kind here too: map IntErrorKind::PosOverflow to AmountTooLarge, and keep ParseU64(err) for everything else.
| #[error("Amount is too large to fit in a u64")] | ||
| AmountTooLarge, |
There was a problem hiding this comment.
I would avoid mentioning u64 here and instead we should just say the amount is too large. InvalidAmount already covers the asset amount not being representable
There was a problem hiding this comment.
I don't fully get why we would need to add this error type, considering ParseU64(#[source] ParseIntError) already has in its inner error the specifics of the failure (overflow in this case). If we are to add it because we don't want the user to see u64 then we should take the extra effort and remove ParseU64 altogether.
…I crate Per review feedback from igamigo: these functions are only used by the CLI, so they belong in bin/miden-cli rather than the client library. Changes: - Move TokenParseError, tokens_to_base_units, base_units_to_tokens from crates/rust-client/src/utils.rs to bin/miden-cli/src/utils.rs - Add miden-protocol and miden-standards as CLI Cargo.toml dependencies - Fix TokenParseError::AmountTooLarge message (remove 'u64' mention) - Fix per-part validation loop: map IntErrorKind::PosOverflow to AmountTooLarge so an oversized integer part also returns AmountTooLarge, not ParseU64 - Update account.rs to import base_units_to_tokens from crate::utils
|
Thanks for the feedback. The motivation for keeping AmountTooLarge separate is that callers can distinguish between two semantically different failures: a formatting error (non-numeric characters → ParseU64) vs. a value that is structurally valid but too large to represent. Without the split, both cases return the same variant and callers that want to give users a meaningful message ("you typed a non-number" vs. "that amount is too large") can't tell them apart. That said, I agree the ParseU64 name exposes an implementation detail. I'm happy to rename it to InvalidFormat or ParseError if that addresses the concern. Would that work, or would you prefer I collapse everything into a single error variant? @igamigo @juan518munoz |
3 participants
… in tokens_to_base_units
When the combined integer+fractional string exceeds u64::MAX, the previous code returned TokenParseError::ParseU64 wrapping a ParseIntError, which misleadingly suggested a digit-format problem rather than an overflow.
Changes:
The per-part pre-validation loop is kept as-is to catch non-numeric characters (ParseU64 remains correct for those cases).
Fixes #2505