Skip to content

fix: reject malformed numeric CLI arguments instead of crashing or coercing to 0 - #24

Open
ayushsingh82 wants to merge 1 commit into
zkp2p:mainfrom
ayushsingh82:fix/asbigint-validation
Open

fix: reject malformed numeric CLI arguments instead of crashing or coercing to 0#24
ayushsingh82 wants to merge 1 commit into
zkp2p:mainfrom
ayushsingh82:fix/asbigint-validation

Conversation

@ayushsingh82

@ayushsingh82 ayushsingh82 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

While looking at how numeric CLI arguments get parsed, I hit two spots in src/utils/ that fail in ways the caller can't see coming.

asBigInt handles deposit ids, intent amounts, maxCost and durations across the deposit / intent / stake / vault / delegate commands. Pass it a hex, fractional, or non-numeric string like "0x10", "1.5" or "abc" and it throws a bare SyntaxError; a non-integer number throws a bare RangeError. Neither goes through the VALIDATION_ERROR envelope the rest of the CLI uses, so the user just gets a stack trace. Worse, BigInt("") is 0n, so a blank --deposit-id quietly becomes deposit 0 with no error at all.

amountToUnits has a similar sharp edge. An amount smaller than one base unit gets rounded down to 0n by parseUnits, so a transfer or approve silently turns into a no-op. And Number#toString switches to exponential notation for very small or very large magnitudes (1e-7, 1e+21), which parseUnits rejects outright.

The change keeps valid input behaving exactly as before:

  • asBigInt now accepts only a plain decimal-integer string or a safe-integer number. Anything else returns a VALIDATION_ERROR that names the field and shows the value it choked on. The bigint passthrough is untouched.
  • amountToUnits rejects a sub-base-unit amount and an implausibly large one with a clear message, and formats through toFixed(decimals) so parseUnits always sees plain decimal notation.

Checked:

  • asBigInt"", "0x10", "1.5", "abc", 1.5, NaN and 2**53 are each rejected with a VALIDATION_ERROR; 42, "42" and " -7 " still parse fine.
  • amountToUnits0.0000001, "0.0000004" (would have rounded to 0n) and 1e21 are each rejected; 0.000001 still returns 1n.
  • npm run check is green: lint, typecheck, 234 tests, and coverage with parsing.ts and validation.ts at 100% lines.

…ercing to 0

asBigInt (deposit ids, intent amounts, maxCost, durations) threw a raw
SyntaxError/RangeError on a hex, fractional, or non-numeric string and on a
non-integer number, and silently returned 0n for "" — so a blank --deposit-id
resolved to deposit 0. It now returns a VALIDATION_ERROR and accepts only a
plain decimal integer string or a safe-integer number.

amountToUnits rounded a sub-base-unit amount to 0n (turning a transfer or
approve into a no-op) and handed exponential notation ('1e-7', '1e+21')
straight to parseUnits, which throws. Both are now rejected with a clear error.
@ayushsingh82

Copy link
Copy Markdown
Contributor Author

@ADWilkinson small correctness-only fix — no behaviour change on valid input, just turns two silent-failure paths (blank --deposit-id → deposit 0, sub-unit amount → 0n no-op) and two raw-exception paths into clean validation errors. npm run check green. Happy to adjust scope.

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.

1 participant