Skip to content

fix(web-client): js_u64_to_u64 throws instead of panicking on out-of-range BigInt - #342

Open
ygd58 wants to merge 2 commits into
0xMiden:mainfrom
ygd58:fix-js-u64-panic
Open

ygd58 wants to merge 2 commits into
0xMiden:mainfrom
ygd58:fix-js-u64-panic

Conversation

@ygd58

@ygd58 ygd58 commented Aug 27, 2026

Copy link
Copy Markdown

js_u64_to_u64 (the Node.js-side JsU64 -> u64 conversion, used at every JS-facing bigint parameter in this crate) panicked whenever the caller passed a BigInt that was negative or >= 2^64 - napi's BigInt has no such constraint at the type level, so this is ordinary, easily-reachable JS input, not a contrived edge case.

Scope

This was a single, widely-shared helper - 33 call sites across 11 files - so a negative or oversized amount/index/timestamp anywhere in this SDK's surface (Felt, Word, FungibleAsset, TransactionStatus.committed, TransactionProver.newRemoteProver's timeout, or any mint/send/swap transaction request builder's amount) crashed the whole module instead of throwing a catchable error.

Fix

Changes js_u64_to_u64 to return Result<u64, JsErr>. Every call site already sat inside a function returning Result<_, JsErr>, so most just gained a ?. Two call sites (TransactionProver::new_remote_prover, TransactionStatus::committed) previously returned their value directly (no Result) and needed their own signature changed - verified neither has another Rust-side caller in this crate. MerklePath::verify and SparseMerklePath::verify return plain bool and already collapse the native call's own Result into false on failure; an invalid index now takes the same false path rather than becoming a Result-returning API change.

Test plan

Added crates/web-client/test/js_u64_out_of_range.test.ts (6 cases) covering Felt, Word, FungibleAsset and TransactionStatus with negative and >=2^64 BigInt inputs, plus one confirming valid input is unaffected.

Ran locally, not just CI:

cargo build -p miden-client-web --no-default-features --features nodejs,testing --release
SKIP_WEB_SERVER=1 pnpm exec playwright test --project=nodejs --workers=1

Build: clean. Full suite: 176 passed, 25 skipped, 0 failed (201 total) - all 6 new cases included, no regressions.

ygd58 added 2 commits August 28, 2026 00:22
…range BigInt

js_u64_to_u64 (the Node.js-side JsU64 -> u64 conversion, used at every
JS-facing bigint parameter in this crate) panicked whenever the caller
passed a BigInt that was negative or >= 2^64 - napi's BigInt has no such
constraint at the type level, so this is ordinary, easily-reachable JS
input, not a contrived edge case.

This was a single, widely-shared helper (33 call sites across 11 files),
so a negative or oversized amount/index/timestamp anywhere in this SDK's
surface (Felt, Word, FungibleAsset, TransactionStatus.committed,
TransactionProver.newRemoteProver's timeout, or any mint/send/swap
transaction request builder's amount) crashed the whole module instead of
throwing a catchable error.

Changes js_u64_to_u64 to return Result<u64, JsErr>. Every call site
already sat inside a function returning Result<_, JsErr>, so most just
gained a `?`. Two call sites (TransactionProver::new_remote_prover,
TransactionStatus::committed) previously returned their value directly
(no Result) and needed their own signature changed to Result<_, JsErr> -
verified neither has another Rust-side caller in this crate.
MerklePath::verify and SparseMerklePath::verify return plain bool and
already collapse the native call's own Result into false on failure; an
invalid index now takes the same false path rather than becoming a
Result-returning API change.

Adds crates/web-client/test/js_u64_out_of_range.test.ts (6 cases) covering
Felt, Word, FungibleAsset and TransactionStatus with negative and
>=2^64 BigInt inputs, plus one confirming valid input is unaffected.

I don't have a matching Rust/WASM toolchain in my current environment, so
this needs local build + `make test-web-client-nodejs` (or the
SKIP_WEB_SERVER playwright invocation) verification before being
considered ready, same as my prior PRs in this repo (0xMiden#294, 0xMiden#339).
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