Conversation
…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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
js_u64_to_u64(the Node.js-sideJsU64->u64conversion, used at every JS-facingbigintparameter in this crate) panicked whenever the caller passed aBigIntthat was negative or>= 2^64- napi'sBigInthas 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_u64to returnResult<u64, JsErr>. Every call site already sat inside a function returningResult<_, JsErr>, so most just gained a?. Two call sites (TransactionProver::new_remote_prover,TransactionStatus::committed) previously returned their value directly (noResult) and needed their own signature changed - verified neither has another Rust-side caller in this crate.MerklePath::verifyandSparseMerklePath::verifyreturn plainbooland already collapse the native call's ownResultintofalseon failure; an invalid index now takes the samefalsepath rather than becoming aResult-returning API change.Test plan
Added
crates/web-client/test/js_u64_out_of_range.test.ts(6 cases) coveringFelt,Word,FungibleAssetandTransactionStatuswith negative and>=2^64BigIntinputs, plus one confirming valid input is unaffected.Ran locally, not just CI:
Build: clean. Full suite:
176 passed, 25 skipped, 0 failed(201 total) - all 6 new cases included, no regressions.