feat(dpp): unify JSON/Value conversion traits + comprehensive round-trip tests

[shumkov]

Summary

Unification of JSON/Value conversion across rs-dpp + wasm-dpp2: canonical JsonConvertible / ValueConvertible traits on every domain type, ~80 trait impls + ~200 round-trip tests, and a coordinated deprecation sweep that removed all 5 documented Critical bugs and most legacy non-canonical conversion mechanisms.

What landed (high-level)

• Pass 1: canonical JsonConvertible / ValueConvertible impls on ~80 rs-dpp domain types.
• Pass 2: ~200 dedicated json_convertible_tests / value_convertible_tests modules with full wire-shape assertions per type.
• Phase D (steps 1–11): deprecation and deletion of non-canonical conversion mechanisms. Removed StateTransitionValueConvert/StateTransitionJsonConvert traits + 68 impl files, A6/A7/A8/A9 identity traits, A10/A11 document traits down to one helper each, AssetLockProof asymmetric methods, and the _versioned DataContract method family. Replaced with canonical + (for DataContract) from_*_validated(value, &pv) opt-in validation.
• All 5 Critical findings resolved (see table below).
• Upstream PRs landed — dashcore fix: Starcounter-Jack JSON-Patch Prototype Pollution vulnerability #708 + feat(drive): log number of refunded epochs #729 merged; this branch dropped the local outpoint_serde wrapper. The blstrs_plus PR is still pending (one ValidatorSet value-round-trip test remains #[ignore]).

Critical findings status

#	Finding	Resolution
Critical-1	is_human_readable HR/non-HR divergence	Documented on both canonical traits in serialization_traits.rs with the divergence table + ContentDeserializer caveat + pointer to canonical dual-shape visitor examples.
Critical-2	Silent array→bytes coercion in From<JsonValue> for Value	Heuristic removed (rs-platform-value/src/converter/serde_json.rs). Faithful conversion: JSON array → Value::Array. Pin tests added.
Critical-3	ExtendedDocument non-round-trippable	Replaced broken manual serde with #[serde(tag = "$extendedFormatVersion")] derive; round-trip tests added.
Critical-4	DataContract serde impurity	Platform-version coupling pinned in 3 regression tests; validation flipped to opt-in (Deserialize no longer hardcodes full_validation=true); KEEP-AS-EXCEPTION rationale documented at all 3 sites.
Critical-5	to_canonical_object sorts keys (assumed signature-load-bearing)	Falsified — signing uses bincode (PlatformSignable derive). Methods had zero production callers; deleted along with A1/A2.

DataContract API — final shape

The deprecation sweep collapsed the _versioned method family into a clear split by validation policy:

• No validation (the new default): canonical serde_json::from_value::<DataContract>(json) / platform_value::from_value::<DataContract>(v) / serde_json::to_value(&dc) / platform_value::to_value(&dc). Use for storage reads, internal round-trips.
• Opt-in validation: DataContract::from_json_validated(json, &pv) / from_value_validated(value, &pv). Use on trust boundaries (SDK ingest, fixture loaders). No bool param — name implies always-validates.
• Kept: to_validating_json(&pv) — different concept (produces JSON-Schema-compatible output with binary as u8 arrays).
• Deleted entirely: to_*_versioned, into_value_versioned, from_*_versioned(_, full_validation, _).

Test results

• rs-dpp: 3619/3619 lib tests pass, 0 failed, 7 ignored. Of the 7 ignored: 6 are pre-existing recursive_schema_validator ignores; 1 is ValidatorSet::value_round_trip_with_full_wire_shape (pending blstrs_plus upstream PR).
• rs-platform-value: 1036/1036 lib tests pass.
• rs-drive: 3040/3040 lib tests pass.
• rs-sdk: 117/117 lib tests pass.
• rs-sdk-ffi: 252/252 lib tests pass.
• Workspace cargo check --tests clean across dpp / drive / drive-abci / wasm-dpp / wasm-dpp2 / dash-sdk / rs-sdk-ffi.

Architectural conventions established

• Tag-shape rules: all versioned outer enums use #[serde(tag = "$formatVersion")]; all discriminated enums use a $-prefixed key ($type, $action, $transition); zero adjacent-tagged enums remain.
• #[json_safe_fields] rollout: 25+ V0/V1 struct leaves carry the attribute. JS-safe large-integer protection at every serialization site.
• Wire-shape test convention: json!{} / platform_value!{} literal assertions in every round-trip test (~85 tests on this convention).
• Wasm-side adapter pattern: impl_wasm_conversions_inner! (45 sites in wasm-dpp2) for rs-dpp domain types using canonical traits; impl_wasm_conversions_serde! (20 sites) for wasm-only DTOs without rs-dpp counterparts — pattern documented and re-audited.

Cross-package audit (just before shipping)

• wasm-sdk: 0 manual Serialize/Deserialize, 0 references to deleted legacy APIs, 38 impl_wasm_serde_conversions! applications. All DTOs follow canonical patterns.
• wasm-dpp2: 3 manual serde impls (IdentifierWasm, PoolingWasm, PlatformAddressWasm) — all documented production-required adapters for lenient JS-facing parsing; rest of crate flows through canonical helpers.
• rs-sdk / rs-sdk-ffi / swift-sdk: no breakage; no references to deleted APIs.

Out of scope (separate work)

• blstrs_plus BLS PublicKey dual-shape deserialize — pending upstream. One ValidatorSet value-round-trip test remains #[ignore] until it lands.
• Pass 5 (delete the legacy wasm-dpp crate) — blocked on team decision.

Docs

• docs/json-value-unification-plan.md — the live plan + status doc (regularly updated through the work).
• docs/json-value-conversion-inventory.md — pre-pass-1 structural inventory.
• docs/json-value-conversion-canonical-pattern.md — the canonical-trait usage pattern, kept up to date.

Test plan

• CI green (currently 3 success / 12 skipped / 0 failed).
• Reviewer runs cargo test -p dpp --features all_features_without_client --lib and sees 3619 pass / 0 fail.
• Reviewer skims docs/json-value-unification-plan.md to confirm the architectural decisions (validation-opt-in, KEEP-AS-EXCEPTION for DataContract, tag-shape conventions) match team intent.
• Spot-check a wasm-dpp2 wrapper migration (e.g., IdentityCreditWithdrawalTransitionWasm) round-trips correctly from the JS SDK perspective.

🤖 Generated with Claude Code

[thepastaclaw]

Code Review

Cumulative review at cf7affb. The only delta since 7672522 is a one-line test alignment in packages/rs-platform-value/tests/coverage_tests.rs (Critical-2 array→bytes heuristic removal); it introduces no new findings. All 7 prior findings remain present at the current head and are carried forward. Codex's claim that prior-6/prior-7 are now fixed is incorrect: platform_value's deserialize_byte_buf delegates to deserialize_bytes, which still rejects Value::Array. Two NaN/Inf coercion sites and three u16-truncation sites are consolidated where they share a root cause.

_Note: Inline posting failed (command failed (1): gh pr diff 3573 --repo dashpay/platform
STDOUT:

STDERR:
could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead. (https://api.github.com/repos/), so I posted the same verified findings as a top-level review body._

🔴 4 blocking | 🟡 3 suggestion(s)

7 additional finding(s)

blocking: Value→JSON conversion silently rewrites NaN/±∞ floats to JSON 0 on all three paths

packages/rs-platform-value/src/converter/serde_json.rs (line 44)

All three Value→JSON converters use Number::from_f64(...).unwrap_or(0.into()) for Value::Float: line 44 (try_into_validating_json), line 140 (try_to_validating_json by-reference), and line 289 (TryInto<JsonValue>). serde_json::Number::from_f64() returns None for NaN, +∞, and -∞, so these helpers silently mutate non-finite floats into the valid JSON number 0 instead of rejecting them. This is a trust-boundary defect because the validating-JSON helpers are explicitly used as a sanitization step before schema validation, signing, and WASM-bound JSON emission. Any caller that places a NaN/±∞ float into a Value (including via a permissive JS or f64 deserialization path) will see the forged value 0 downstream, defeating any range/finite check that would otherwise have rejected the input. The Critical-2 coverage test added at this commit additionally locks in the silent-coercion behavior. Return Error::Unsupported (or a dedicated non-finite variant) on None instead of unwrap_or(0.into()).

blocking: ExtendedDocumentWasm omits the public toObject()/toJSON() JS ABI

packages/wasm-dpp/src/document/extended_document.rs (line 27)

The exported #[wasm_bindgen(js_class=ExtendedDocument)] impl exposes constructor, getters/setters, toBuffer, hash, clone, validate, get, and set, but no #[wasm_bindgen(js_name=toObject)] or #[wasm_bindgen(js_name=toJSON)] (verified via grep — only getData returns the inner properties map, not the full extended-document shape). Peer wrappers in this crate (e.g. IdentityWasm in packages/wasm-dpp/src/identity/identity.rs:166-220) and most wasm-dpp2 wrappers expose this conversion ABI, and the entire PR centers on standardizing canonical object/JSON conversion. JS consumers that previously relied on extendedDocument.toObject()/toJSON() — including JSON.stringify(extendedDocument), which transparently invokes toJSON — get undefined from the wasm boundary. This is a public ABI regression for JS callers. Add the canonical toObject/toJSON methods on the exported class.

blocking: Query retry loop never reaches its advertised 1-second timeout (counter reset each iteration)

packages/rs-drive-abci/src/query/service.rs (line 129)

let mut counter = 0; is declared on line 133, inside the loop { ... } body started on line 129. Every iteration zeroes the counter, runs counter += 1 once, then evaluates if counter >= 100 — which is never true (counter is always 1 at that point). The advertised 1-second timeout / query_counter increment / needs_restart = true branch is unreachable, so a query whose required height never lands keeps sleeping in 10ms increments indefinitely instead of giving up and (eventually) returning NotServiceable. Compare with query_counter, which is correctly declared outside the loop. Beyond the latency bug, this is a denial-of-service primitive: untrusted gRPC callers can pin spawn_blocking worker threads during any state/drive height inconsistency window. Declare let mut counter = 0; immediately before the loop { on line 129.

blocking: Vote-query proof verifiers truncate u32 limits/offsets to u16, breaking proof binding

packages/rs-drive-proof-verifier/src/from_request.rs (line 89)

Six unchecked as u16 casts on wire-u32 pagination fields remain at lines 89, 199, 254, 324, 370, and 371 (verified via grep). The server-side drive-abci handlers reject overflow with u16::try_from(...), but the verifier wraps silently — so a malicious or buggy server can answer a client request for count = 70000 with a proof for count = 4464 (or any count & 0xFFFF) and the verifier will happily verify it against the wrapped query. The offset case at line 371 is the strongest: an attacker can shift the entire returned window without verification failing. This is a proof-soundness break at the network trust boundary for ContestedDocumentVotePollDriveQuery, ContestedResourceVotesGivenByIdentityQuery, ContestedDocumentVotePollVotesDriveQuery, VotePollsByDocumentTypeQuery, and VotePollsByEndDateDriveQuery. Replace each v as u16 with a checked u16::try_from(v).map_err(...)? so the verifier matches the server-side validation semantics.

suggestion: PartialIdentity key-ID parsing lossy-casts invalid JS numbers into different KeyIDs

packages/wasm-dpp2/src/identity/partial_identity.rs (line 320)

option_array_to_not_found() (lines 320-348) accepts any JsValue with as_f64(), validates only the upper bound (key_val > u32::MAX as f64), then casts key_val as KeyID. Rust's f64 as u32 is saturating/truncating: -1.0 → 0, NaN → 0, -Infinity → 0, 1.9 → 1. A JS caller passing [-1, 1.5, NaN, -Infinity] silently produces {KeyID(0)} instead of being rejected — conflating invalid inputs with the legitimate key id 0. The same lossy pattern is repeated in value_to_loaded_public_keys_from_object (line 363/374, key_str.parse::<f64>() → key_val as u32) and value_to_loaded_public_keys_from_json (lines 413/424), where it additionally accepts strings like "NaN", "Infinity", "-1", "1.5" as object keys. The crate already has a stricter try_to_u32() helper in packages/wasm-dpp2/src/utils.rs that rejects non-integer/negative/non-finite inputs; the partial-identity parsers should use it (or equivalent finite + non-negative + integral checks) before casting.

suggestion: Fixed-size byte deserializer rejects Value::Array of u8 on non-human-readable platform_value

packages/rs-dpp/src/serialization/serde_bytes.rs (line 90)

AnyShapeVisitor::<N> advertises (and implements) visit_byte_buf, visit_bytes, visit_str (base64), and visit_seq (sequence of u8). The HR branch (line 96) uses deserialize_any, but the non-HR branch (line 101) hard-wires deserialize_byte_buf(AnyShapeVisitor::<N>). On platform_value::Deserializer, deserialize_byte_buf delegates to deserialize_bytes (packages/rs-platform-value/src/value_serialization/de.rs:347-352), which only accepts Value::Bytes*/Value::Identifier and rejects Value::Array with invalid_type before visit_seq can run. After the Critical-2 fix in this PR (JSON integer arrays no longer coerce to Value::Bytes), JSON→platform_value→struct paths for fixed-size byte fields now fail on the array shape — even though the visitor was written to support it. Either implement an Array<u8> fallback in platform_value::Deserializer::deserialize_byte_buf, or call deserialize_any on the non-HR branch for sources that support it (and keep deserialize_byte_buf only for bincode). Note: codex flagged this as fixed; verification shows the platform_value path still rejects arrays here.

suggestion: Variable-length byte deserializer has the same Value::Array gap on non-human-readable input

packages/rs-dpp/src/serialization/serde_bytes_var.rs (line 68)

Same defect as serde_bytes.rs: AnyShapeVisitor implements visit_seq (collecting u8 elements into Vec<u8>), but the non-HR branch unconditionally calls deserialize_byte_buf(AnyShapeVisitor), which on platform_value::Deserializer delegates to deserialize_bytes and rejects Value::Array before the seq visitor runs. Variable-length byte fields read from JSON-derived Value objects fail this path despite the visitor being designed to accept them. Same fix shape as serde_bytes.rs.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-value/src/converter/serde_json.rs`:44-289: Value→JSON conversion silently rewrites NaN/±∞ floats to JSON 0 on all three paths
  All three Value→JSON converters use `Number::from_f64(...).unwrap_or(0.into())` for `Value::Float`: line 44 (`try_into_validating_json`), line 140 (`try_to_validating_json` by-reference), and line 289 (`TryInto<JsonValue>`). `serde_json::Number::from_f64()` returns `None` for `NaN`, `+∞`, and `-∞`, so these helpers silently mutate non-finite floats into the valid JSON number `0` instead of rejecting them. This is a trust-boundary defect because the validating-JSON helpers are explicitly used as a sanitization step before schema validation, signing, and WASM-bound JSON emission. Any caller that places a NaN/±∞ float into a `Value` (including via a permissive JS or `f64` deserialization path) will see the forged value `0` downstream, defeating any range/finite check that would otherwise have rejected the input. The Critical-2 coverage test added at this commit additionally locks in the silent-coercion behavior. Return `Error::Unsupported` (or a dedicated non-finite variant) on `None` instead of `unwrap_or(0.into())`.
- [BLOCKING] In `packages/wasm-dpp/src/document/extended_document.rs`:27-303: ExtendedDocumentWasm omits the public toObject()/toJSON() JS ABI
  The exported `#[wasm_bindgen(js_class=ExtendedDocument)]` impl exposes constructor, getters/setters, `toBuffer`, `hash`, `clone`, `validate`, `get`, and `set`, but no `#[wasm_bindgen(js_name=toObject)]` or `#[wasm_bindgen(js_name=toJSON)]` (verified via grep — only `getData` returns the inner properties map, not the full extended-document shape). Peer wrappers in this crate (e.g. `IdentityWasm` in packages/wasm-dpp/src/identity/identity.rs:166-220) and most wasm-dpp2 wrappers expose this conversion ABI, and the entire PR centers on standardizing canonical object/JSON conversion. JS consumers that previously relied on `extendedDocument.toObject()`/`toJSON()` — including `JSON.stringify(extendedDocument)`, which transparently invokes `toJSON` — get `undefined` from the wasm boundary. This is a public ABI regression for JS callers. Add the canonical `toObject`/`toJSON` methods on the exported class.
- [BLOCKING] In `packages/rs-drive-abci/src/query/service.rs`:129-148: Query retry loop never reaches its advertised 1-second timeout (counter reset each iteration)
  `let mut counter = 0;` is declared on line 133, *inside* the `loop { ... }` body started on line 129. Every iteration zeroes the counter, runs `counter += 1` once, then evaluates `if counter >= 100` — which is never true (`counter` is always 1 at that point). The advertised 1-second timeout / `query_counter` increment / `needs_restart = true` branch is unreachable, so a query whose required height never lands keeps sleeping in 10ms increments indefinitely instead of giving up and (eventually) returning `NotServiceable`. Compare with `query_counter`, which is correctly declared outside the loop. Beyond the latency bug, this is a denial-of-service primitive: untrusted gRPC callers can pin `spawn_blocking` worker threads during any state/drive height inconsistency window. Declare `let mut counter = 0;` immediately before the `loop {` on line 129.
- [BLOCKING] In `packages/rs-drive-proof-verifier/src/from_request.rs`:89-371: Vote-query proof verifiers truncate u32 limits/offsets to u16, breaking proof binding
  Six unchecked `as u16` casts on wire-`u32` pagination fields remain at lines 89, 199, 254, 324, 370, and 371 (verified via grep). The server-side drive-abci handlers reject overflow with `u16::try_from(...)`, but the verifier wraps silently — so a malicious or buggy server can answer a client request for `count = 70000` with a proof for `count = 4464` (or any `count & 0xFFFF`) and the verifier will happily verify it against the wrapped query. The `offset` case at line 371 is the strongest: an attacker can shift the entire returned window without verification failing. This is a proof-soundness break at the network trust boundary for ContestedDocumentVotePollDriveQuery, ContestedResourceVotesGivenByIdentityQuery, ContestedDocumentVotePollVotesDriveQuery, VotePollsByDocumentTypeQuery, and VotePollsByEndDateDriveQuery. Replace each `v as u16` with a checked `u16::try_from(v).map_err(...)?` so the verifier matches the server-side validation semantics.
- [SUGGESTION] In `packages/wasm-dpp2/src/identity/partial_identity.rs`:320-425: PartialIdentity key-ID parsing lossy-casts invalid JS numbers into different KeyIDs
  `option_array_to_not_found()` (lines 320-348) accepts any `JsValue` with `as_f64()`, validates only the upper bound (`key_val > u32::MAX as f64`), then casts `key_val as KeyID`. Rust's `f64 as u32` is saturating/truncating: `-1.0 → 0`, `NaN → 0`, `-Infinity → 0`, `1.9 → 1`. A JS caller passing `[-1, 1.5, NaN, -Infinity]` silently produces `{KeyID(0)}` instead of being rejected — conflating invalid inputs with the legitimate key id 0. The same lossy pattern is repeated in `value_to_loaded_public_keys_from_object` (line 363/374, `key_str.parse::<f64>()` → `key_val as u32`) and `value_to_loaded_public_keys_from_json` (lines 413/424), where it additionally accepts strings like `"NaN"`, `"Infinity"`, `"-1"`, `"1.5"` as object keys. The crate already has a stricter `try_to_u32()` helper in packages/wasm-dpp2/src/utils.rs that rejects non-integer/negative/non-finite inputs; the partial-identity parsers should use it (or equivalent finite + non-negative + integral checks) before casting.
- [SUGGESTION] In `packages/rs-dpp/src/serialization/serde_bytes.rs`:90-102: Fixed-size byte deserializer rejects Value::Array of u8 on non-human-readable platform_value
  `AnyShapeVisitor::<N>` advertises (and implements) `visit_byte_buf`, `visit_bytes`, `visit_str` (base64), and `visit_seq` (sequence of `u8`). The HR branch (line 96) uses `deserialize_any`, but the non-HR branch (line 101) hard-wires `deserialize_byte_buf(AnyShapeVisitor::<N>)`. On `platform_value::Deserializer`, `deserialize_byte_buf` delegates to `deserialize_bytes` (packages/rs-platform-value/src/value_serialization/de.rs:347-352), which only accepts `Value::Bytes*`/`Value::Identifier` and rejects `Value::Array` with `invalid_type` before `visit_seq` can run. After the Critical-2 fix in this PR (JSON integer arrays no longer coerce to `Value::Bytes`), JSON→platform_value→struct paths for fixed-size byte fields now fail on the array shape — even though the visitor was written to support it. Either implement an `Array<u8>` fallback in `platform_value::Deserializer::deserialize_byte_buf`, or call `deserialize_any` on the non-HR branch for sources that support it (and keep `deserialize_byte_buf` only for bincode). Note: codex flagged this as fixed; verification shows the platform_value path still rejects arrays here.
- [SUGGESTION] In `packages/rs-dpp/src/serialization/serde_bytes_var.rs`:68-76: Variable-length byte deserializer has the same Value::Array gap on non-human-readable input
  Same defect as serde_bytes.rs: `AnyShapeVisitor` implements `visit_seq` (collecting `u8` elements into `Vec<u8>`), but the non-HR branch unconditionally calls `deserialize_byte_buf(AnyShapeVisitor)`, which on `platform_value::Deserializer` delegates to `deserialize_bytes` and rejects `Value::Array` before the seq visitor runs. Variable-length byte fields read from JSON-derived `Value` objects fail this path despite the visitor being designed to accept them. Same fix shape as serde_bytes.rs.

[github-actions]

✅ DashSDKFFI.xcframework built for this PR.

• Workflow run: https://github.com/dashpay/platform/actions/runs/27264884388
• Artifacts: DashSDKFFI.xcframework (folder), DashSDKFFI.xcframework.zip, xc_checksum.txt

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "0ec62892b26b443f87af198c4de645829dc925e464ae3b0fb05941ad6248350f"
)

Xcode manual integration:

• Download 'DashSDKFFI.xcframework' artifact from the run link above.
• Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
• If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

[thepastaclaw]

Code Review

Latest delta (5ae8c26) is a doctest-only correction in rs-platform-value/src/value_serialization/mod.rs and introduces no new defects. Cumulative re-verification confirms all seven prior findings are STILL VALID on disk at the cited locations: NaN/Inf→0 silent coercion in three Value→JSON paths (serde_json.rs:44,140,289); ExtendedDocumentWasm still lacks toObject/toJSON wasm-bindgen exports; query service.rs:133 still declares the retry counter inside the loop body; vote-query proof verifiers still truncate u32 pagination to u16 at six sites in from_request.rs; PartialIdentity (wasm-dpp2) still lossy-casts f64→KeyID; and both serde_bytes deserializers still hard-wire deserialize_byte_buf on the non-human-readable branch. Four are merge-blocking correctness/security bugs; three are suggestion-level robustness issues. No new findings introduced by the delta.

_Note: Inline posting failed (command failed (1): gh pr diff 3573 --repo dashpay/platform
STDOUT:

STDERR:
could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead. (https://api.github.com/repos/), so I posted the same verified findings as a top-level review body._

🔴 4 blocking | 🟡 3 suggestion(s)

7 additional finding(s)

blocking: Value→JSON silently rewrites NaN/±∞ floats to JSON 0 across all three converters

packages/rs-platform-value/src/converter/serde_json.rs (line 44)

All three Value→JsonValue paths in this file — try_into_validating_json (line 44), the by-reference try_to_validating_json (line 140), and TryInto<JsonValue> for Value (line 289) — still use Number::from_f64(float).unwrap_or(0.into()). serde_json::Number::from_f64() returns None for NaN, +inf, and -inf, so any non-finite float is silently rewritten to the valid JSON number 0. This is a data-integrity defect at the Rust→JSON/JS boundary: callers cannot distinguish a real zero from an unrepresentable float, and the JSON-Schema validator that runs over the converted document (e.g. validate_document_v0() calls try_into_validating_json()) validates a different value than the platform-serialized state transition actually carries. The new test validating_json_float_nan_becomes_zero (lines 1135-1139) codifies the broken behavior. All three signatures already return Result<JsonValue, Error>, so the fix is to return Err(Error::Unsupported(...)) (or a new Error::NonFiniteFloat) when Number::from_f64 returns None, and update the test accordingly.

blocking: ExtendedDocumentWasm omits the public toObject()/toJSON() JS ABI

packages/wasm-dpp/src/document/extended_document.rs (line 31)

The exported #[wasm_bindgen(js_class=ExtendedDocument)] impl exposes constructor, getters/setters, toBuffer, hash, clone, validate, get, and set, but no #[wasm_bindgen(js_name=toObject)] or #[wasm_bindgen(js_name=toJSON)]. Grep confirms zero matches in this file. Other wasm wrappers (IdentityWasm, DataContractWasm, …) expose the canonical conversion ABI, and the generic JS-side conversion helper in packages/wasm-dpp2/src/serialization/conversions.rs looks for toJSON() before normalizing. Without these methods on ExtendedDocument, JSON.stringify(extendedDocument) and extendedDocument.toObject() both fail/throw, and the rest of this PR's canonical JsonConvertible/ValueConvertible unification is undermined for the one type that needs it most. Add toObject() and toJSON() wasm-bindgen methods that round-trip through the canonical Rust-side serializers.

blocking: Query retry loop never reaches its advertised 1-second timeout (counter reset every iteration)

packages/rs-drive-abci/src/query/service.rs (line 129)

Verified at line 133: let mut counter = 0; is declared inside the loop body. On every iteration the variable is recreated as 0, then either the loop breaks (heights match) or counter is incremented to 1 and the counter >= 100 check at line 143 is evaluated — which can never be true. The comment at line 142 ("We try for up to 1 second") is unreachable, and needs_restart is never set via this path. Operationally this means a wedged committed_block_height_guard (e.g. under load or during block-processing churn) causes the inner loop to sleep-and-retry forever, parking the spawn-blocking worker indefinitely and bypassing the query_counter > 3 saturation guard at line 150. Because queries are dispatched via spawn_blocking_task_with_name_if_supported("query", ...), an adversary that can induce or wait for height divergence can saturate the blocking pool. Fix: hoist let mut counter = 0; outside the inner loop so increments accumulate.

                let mut needs_restart = false;

                let mut counter = 0;
                loop {
                    let committed_block_height_guard = platform
                        .committed_block_height_guard
                        .load(Ordering::Relaxed);
                    if platform_state.last_committed_block_height() == committed_block_height_guard
                    {
                        break;
                    } else {
                        counter += 1;
                        sleep(Duration::from_millis(10))
                    }

                    // We try for up to 1 second
                    if counter >= 100 {
                        query_counter += 1;
                        needs_restart = true;
                        break;
                    }
                }

blocking: Vote-query proof verifiers truncate wire u32 pagination to u16, breaking proof binding

packages/rs-drive-proof-verifier/src/from_request.rs (line 89)

Verified at six sites: limit: v.count.map(|v| v as u16) (lines 89, 254, 324), limit: value.limit.map(|x| x as u16) (line 199), and limit: v.limit.map(|v| v as u16), offset: v.offset.map(|v| v as u16) (lines 370-371). Server-side handlers in rs-drive-abci's voting query module reject overflow with checked u16::try_from(...), but the verifier silently wraps any u32 > 65535 (e.g. 70000_u32 as u16 == 4464, 65536_u32 as u16 == 0). A malicious or buggy DAPI server can therefore answer a request whose wire count is 70000 with a proof bound to count=4464; the verifier reconstructs its query with the same truncation and "verifies" the proof against the wrong pagination window. For light-client consumers, authenticated proofs no longer guarantee the result corresponds to the requested page. Fix: replace each as u16 with u16::try_from(v).map_err(|_| Error::RequestError { error: format!("... exceeds u16::MAX") })? so the verifier rejects oversized wire values symmetrically with the server.

suggestion: PartialIdentity key-ID parsing lossy-casts invalid JS numbers into different KeyIDs

packages/wasm-dpp2/src/identity/partial_identity.rs (line 320)

Verified at three sites: option_array_to_not_found() (lines 320-348) takes key.as_f64(), checks only the upper bound (> u32::MAX as f64), then casts with key_val as KeyID (line 341). value_to_loaded_public_keys_from_object() (line 374) and value_to_loaded_public_keys_from_json() (line 424) parse object keys as f64 and cast with as u32 after the same one-sided check. Rust's f64 as u32 saturates negatives and NaN to 0, saturates +∞ to u32::MAX, and truncates fractional values. So JS inputs like -1, NaN, Infinity, 1.5, or "4294967296.5" are silently aliased to KeyIDs 0, 0, u32::MAX, 1, and 0 respectively — none rejected. This can collide identity keys at parse time, marking the wrong key index as "loaded" or "not found". The same crate already exposes a strict helper (packages/wasm-dpp2/src/utils.rs) and uses it on the non-object path here; the object/array paths should use it too, or validate is_finite() && >= 0.0 && fract() == 0.0 && <= u32::MAX as f64 before casting. For the string-key paths, parsing as u32 directly (key_str.parse::<u32>()) naturally rejects negatives, fractions, and out-of-range values.

suggestion: Fixed-size byte deserializer rejects Value::Array on non-human-readable platform_value

packages/rs-dpp/src/serialization/serde_bytes.rs (line 90)

The AnyShapeVisitor::<N> advertises and implements four shapes — visit_byte_buf, visit_bytes, visit_str (base64), and visit_seq (sequences of u8). The human-readable branch (line 96) routes through deserialize_any which can dispatch any of them, but the non-HR branch (line 101) hard-wires deserialize_byte_buf(AnyShapeVisitor::<N>). On platform_value::Deserializer, deserialize_byte_buf delegates to deserialize_bytes, which only accepts Value::Bytes*/Identifier and rejects Value::Array before visit_seq can run. The surrounding comment claims all four shapes are accepted regardless of is_human_readable, but that is not true for the platform_value deserializer. Since this PR's Critical-2 fix makes JSON arrays flow into Value::Array faithfully (rather than being heuristically coerced to Value::Bytes), the gap is now reachable in practice for byte fields that started life as JSON arrays. Fix: probe shape via deserialize_any on platform_value as well, or fall back to deserialize_seq on type errors.

suggestion: Variable-length byte deserializer has the same Value::Array gap on non-HR input

packages/rs-dpp/src/serialization/serde_bytes_var.rs (line 68)

Same defect as serde_bytes.rs, for Vec<u8> rather than [u8; N]: the visitor implements visit_seq() and can collect u8 elements, but the non-HR branch unconditionally calls deserialize_byte_buf(AnyShapeVisitor). On platform_value::Deserializer, that path rejects Value::Array before visit_seq is reachable. Resolve identically to the fixed-size case — dispatch through deserialize_any on platform_value or fall back to deserialize_seq for non-Bytes Value variants. Same Critical-2 interaction increases reachability.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-value/src/converter/serde_json.rs`:44-289: Value→JSON silently rewrites NaN/±∞ floats to JSON 0 across all three converters
  All three Value→JsonValue paths in this file — `try_into_validating_json` (line 44), the by-reference `try_to_validating_json` (line 140), and `TryInto<JsonValue> for Value` (line 289) — still use `Number::from_f64(float).unwrap_or(0.into())`. `serde_json::Number::from_f64()` returns `None` for `NaN`, `+inf`, and `-inf`, so any non-finite float is silently rewritten to the valid JSON number `0`. This is a data-integrity defect at the Rust→JSON/JS boundary: callers cannot distinguish a real zero from an unrepresentable float, and the JSON-Schema validator that runs over the converted document (e.g. `validate_document_v0()` calls `try_into_validating_json()`) validates a different value than the platform-serialized state transition actually carries. The new test `validating_json_float_nan_becomes_zero` (lines 1135-1139) codifies the broken behavior. All three signatures already return `Result<JsonValue, Error>`, so the fix is to return `Err(Error::Unsupported(...))` (or a new `Error::NonFiniteFloat`) when `Number::from_f64` returns `None`, and update the test accordingly.
- [BLOCKING] In `packages/wasm-dpp/src/document/extended_document.rs`:31-303: ExtendedDocumentWasm omits the public toObject()/toJSON() JS ABI
  The exported `#[wasm_bindgen(js_class=ExtendedDocument)]` impl exposes constructor, getters/setters, `toBuffer`, `hash`, `clone`, `validate`, `get`, and `set`, but no `#[wasm_bindgen(js_name=toObject)]` or `#[wasm_bindgen(js_name=toJSON)]`. Grep confirms zero matches in this file. Other wasm wrappers (IdentityWasm, DataContractWasm, …) expose the canonical conversion ABI, and the generic JS-side conversion helper in `packages/wasm-dpp2/src/serialization/conversions.rs` looks for `toJSON()` before normalizing. Without these methods on ExtendedDocument, `JSON.stringify(extendedDocument)` and `extendedDocument.toObject()` both fail/throw, and the rest of this PR's canonical JsonConvertible/ValueConvertible unification is undermined for the one type that needs it most. Add `toObject()` and `toJSON()` wasm-bindgen methods that round-trip through the canonical Rust-side serializers.
- [BLOCKING] In `packages/rs-drive-abci/src/query/service.rs`:129-148: Query retry loop never reaches its advertised 1-second timeout (counter reset every iteration)
  Verified at line 133: `let mut counter = 0;` is declared inside the `loop` body. On every iteration the variable is recreated as 0, then either the loop breaks (heights match) or counter is incremented to 1 and the `counter >= 100` check at line 143 is evaluated — which can never be true. The comment at line 142 ("We try for up to 1 second") is unreachable, and `needs_restart` is never set via this path. Operationally this means a wedged `committed_block_height_guard` (e.g. under load or during block-processing churn) causes the inner loop to sleep-and-retry forever, parking the spawn-blocking worker indefinitely and bypassing the `query_counter > 3` saturation guard at line 150. Because queries are dispatched via `spawn_blocking_task_with_name_if_supported("query", ...)`, an adversary that can induce or wait for height divergence can saturate the blocking pool. Fix: hoist `let mut counter = 0;` outside the inner loop so increments accumulate.
- [BLOCKING] In `packages/rs-drive-proof-verifier/src/from_request.rs`:89-371: Vote-query proof verifiers truncate wire u32 pagination to u16, breaking proof binding
  Verified at six sites: `limit: v.count.map(|v| v as u16)` (lines 89, 254, 324), `limit: value.limit.map(|x| x as u16)` (line 199), and `limit: v.limit.map(|v| v as u16), offset: v.offset.map(|v| v as u16)` (lines 370-371). Server-side handlers in rs-drive-abci's voting query module reject overflow with checked `u16::try_from(...)`, but the verifier silently wraps any u32 > 65535 (e.g. `70000_u32 as u16 == 4464`, `65536_u32 as u16 == 0`). A malicious or buggy DAPI server can therefore answer a request whose wire `count` is `70000` with a proof bound to `count=4464`; the verifier reconstructs its query with the same truncation and "verifies" the proof against the wrong pagination window. For light-client consumers, authenticated proofs no longer guarantee the result corresponds to the requested page. Fix: replace each `as u16` with `u16::try_from(v).map_err(|_| Error::RequestError { error: format!("... exceeds u16::MAX") })?` so the verifier rejects oversized wire values symmetrically with the server.
- [SUGGESTION] In `packages/wasm-dpp2/src/identity/partial_identity.rs`:320-425: PartialIdentity key-ID parsing lossy-casts invalid JS numbers into different KeyIDs
  Verified at three sites: `option_array_to_not_found()` (lines 320-348) takes `key.as_f64()`, checks only the upper bound (`> u32::MAX as f64`), then casts with `key_val as KeyID` (line 341). `value_to_loaded_public_keys_from_object()` (line 374) and `value_to_loaded_public_keys_from_json()` (line 424) parse object keys as `f64` and cast with `as u32` after the same one-sided check. Rust's `f64 as u32` saturates negatives and NaN to 0, saturates +∞ to `u32::MAX`, and truncates fractional values. So JS inputs like `-1`, `NaN`, `Infinity`, `1.5`, or `"4294967296.5"` are silently aliased to KeyIDs 0, 0, `u32::MAX`, 1, and 0 respectively — none rejected. This can collide identity keys at parse time, marking the wrong key index as "loaded" or "not found". The same crate already exposes a strict helper (`packages/wasm-dpp2/src/utils.rs`) and uses it on the non-object path here; the object/array paths should use it too, or validate `is_finite() && >= 0.0 && fract() == 0.0 && <= u32::MAX as f64` before casting. For the string-key paths, parsing as `u32` directly (`key_str.parse::<u32>()`) naturally rejects negatives, fractions, and out-of-range values.
- [SUGGESTION] In `packages/rs-dpp/src/serialization/serde_bytes.rs`:90-102: Fixed-size byte deserializer rejects Value::Array<u8> on non-human-readable platform_value
  The `AnyShapeVisitor::<N>` advertises and implements four shapes — `visit_byte_buf`, `visit_bytes`, `visit_str` (base64), and `visit_seq` (sequences of `u8`). The human-readable branch (line 96) routes through `deserialize_any` which can dispatch any of them, but the non-HR branch (line 101) hard-wires `deserialize_byte_buf(AnyShapeVisitor::<N>)`. On `platform_value::Deserializer`, `deserialize_byte_buf` delegates to `deserialize_bytes`, which only accepts `Value::Bytes*`/`Identifier` and rejects `Value::Array` before `visit_seq` can run. The surrounding comment claims all four shapes are accepted regardless of `is_human_readable`, but that is not true for the platform_value deserializer. Since this PR's Critical-2 fix makes JSON arrays flow into `Value::Array` faithfully (rather than being heuristically coerced to `Value::Bytes`), the gap is now reachable in practice for byte fields that started life as JSON arrays. Fix: probe shape via `deserialize_any` on platform_value as well, or fall back to `deserialize_seq` on type errors.
- [SUGGESTION] In `packages/rs-dpp/src/serialization/serde_bytes_var.rs`:68-76: Variable-length byte deserializer has the same Value::Array gap on non-HR input
  Same defect as `serde_bytes.rs`, for `Vec<u8>` rather than `[u8; N]`: the visitor implements `visit_seq()` and can collect `u8` elements, but the non-HR branch unconditionally calls `deserialize_byte_buf(AnyShapeVisitor)`. On `platform_value::Deserializer`, that path rejects `Value::Array` before `visit_seq` is reachable. Resolve identically to the fixed-size case — dispatch through `deserialize_any` on platform_value or fall back to `deserialize_seq` for non-Bytes Value variants. Same Critical-2 interaction increases reachability.

[thepastaclaw]

Code Review

Cumulative reconciliation at HEAD 47f99de. The 7 prior findings split cleanly: 4 target files this PR never touches (out of scope: service.rs retry counter, from_request.rs u32→u16, PartialIdentity lossy-cast lines, plus codex's factory skip_validation naming mismatch — all verified pre-existing on v3.1-dev), 2 are intentionally deferred / resolved by documented design choices in PR-touched files (ExtendedDocument toObject/toJSON deletion is deliberate; serde_bytes/serde_bytes_var non-HR design is documented and the wasm-dpp JS-array path is handled by the new lenient helper in f135b20), and 1 remains as a defensible in-scope concern: Value::Float → JSON silent NaN/±∞ → 0 substitution in all three Value→JsonValue converters in a file the PR rewrites. The new latest-delta commits (f135b20, c18c124, 47f99de) are small, well-scoped fixups with no new defects. The team explicitly added a pinning test asserting NaN → 0 as expected behavior, which signals deliberate acceptance — flagging as a suggestion so the divergence from the PR's canonical-round-trip thesis is consciously acknowledged.

Note: Inline posting failed because GitHub refuses this PR diff with PullRequest.diff too_large (>300 files), so I am posting the verifier-approved result as a top-level review body.

Reviewed commit: 47f99de

🟡 1 suggestion(s)

Prior-finding reconciliation

• Prior #1 (Value→JSON NaN/±∞→0): STILL VALID / carried forward as the active finding below, severity adjusted to suggestion because the current head explicitly pins the behavior as accepted but it still conflicts with canonical round-trip goals.
• Prior #2 (ExtendedDocument toObject()/toJSON() ABI): INTENTIONALLY DEFERRED / no longer active — current head deliberately removes those methods and the corresponding tests/callers; legacy wasm-dpp is marked for deletion.
• Prior #3 (query retry counter reset): verified present but out of scope for this PR head — unchanged from v3.1-dev, so not re-posted as an active review finding here.
• Prior #4 (vote-query proof u32→u16 truncation): verified present but out of scope for this PR head — unchanged from v3.1-dev, so not re-posted as an active review finding here.
• Prior #5 (PartialIdentity key-ID lossy casts): verified present but out of scope for this PR head — unchanged except unrelated canonical decode migration.
• Prior #6/#7 (serde_bytes / serde_bytes_var non-HR Value::Array gap): INTENTIONALLY DEFERRED — verifier found this matches the documented non-HR Value::Bytes canonical shape; the legacy JS-array path is handled by the new lenient helper.

New findings in the latest delta

No new defects were introduced by 5ae8c267..47f99dea; the latest commits are small targeted fixups.

Verified active findings

suggestion: Value→JSON silently rewrites NaN/±∞ to 0 in all three converters — contradicts the PR's canonical round-trip thesis

packages/rs-platform-value/src/converter/serde_json.rs (line 44-289)

All three Value→JsonValue paths in this file — try_into_validating_json (line 44), the by-reference try_to_validating_json (line 140), and TryInto<JsonValue> for Value (line 289) — use JsonValue::Number(Number::from_f64(float).unwrap_or(0.into())). serde_json::Number::from_f64() returns None for NaN/+∞/−∞, so non-finite floats are silently rewritten to JSON 0. The new test validating_json_float_nan_becomes_zero pins this for NaN, but the inverse path now produces Value::U64(0) — a genuine round-trip data loss rather than a documented restriction. This file is in-scope (the PR rewrote the From<JsonValue> for Value Array branch for Critical-2 and the overall stated goal is canonical, faithful JSON↔Value conversion); preserving silent 0-substitution here is the one remaining hole in the canonicalization sweep. Either return Err(Error::Unsupported("non-finite float not representable in JSON")) (matches Identifier/EnumU8 handling in the same match arm), or document the divergence next to the dual-shape visitor discussion in serialization_traits.rs and extend the pin test to ±∞ so the behavior is fully explicit.

Verifier notes for non-active prior/agent findings

• ExtendedDocumentWasm omits the public toObject()/toJSON() JS ABI — Intentionally deferred / resolved by design. Commit 123c00c in this PR deliberately removed the underlying ExtendedDocument::to_json / to_json_object_for_validation Rust methods together with the wasm-dpp toJSON()/toObject() bindings, and the corresponding describe('#toJSON') / describe('#toObject') JS test blocks were deleted. Grep across js-dash-sdk, js-dapi-client, wallet-lib, js-evo-sdk, and platform-test-suite shows zero remaining call sites. PR description confirms legacy wasm-dpp is slated for deletion.
• Query retry loop never reaches its advertised 1-second timeout (counter reset every iteration) — Out of scope — not introduced or touched by this PR. git diff origin/v3.1-dev...HEAD -- packages/rs-drive-abci/src/query/service.rs shows this PR only adds two new query handlers and imports; the inner let mut counter = 0; retry loop is unchanged. The defect originated in PR #1776, long predates this branch, and should be tracked as a separate v3.1-dev issue.
• Vote-query proof verifiers truncate wire u32 pagination to u16, breaking proof binding — Out of scope — git diff origin/v3.1-dev...HEAD -- packages/rs-drive-proof-verifier/src/from_request.rs returns zero lines. The six unchecked as u16 casts are pre-existing v3.1-dev code unrelated to JSON/Value unification. Track as a separate proof-binding correctness issue.
• PartialIdentity key-ID parsing lossy-casts invalid JS numbers into different KeyIDs — Out of scope — verified via git diff origin/v3.1-dev...HEAD -- packages/wasm-dpp2/src/identity/partial_identity.rs that this PR only changes the IdentityPublicKey decode path (legacy from_object(value, &pv) → canonical ValueConvertible::from_object(value)) and renames platform_version to _platform_version. The three lossy-cast helpers (option_array_to_not_found lines 330-341, value_to_loaded_public_keys_from_object lines 363-374, value_to_loaded_public_keys_from_json lines 413-424) are pre-existing v3.1-dev code, untouched by this PR. Worth a separate hardening pass.
• Fixed-size byte deserializer rejects Value::Array on non-human-readable platform_value — Intentionally deferred — file is heavily rewritten by this PR and the non-HR branch's choice of deserialize_byte_buf is documented as deliberate (bincode requires explicit shape hint; non-HR Value carries binary as Value::Bytes). The Critical-2 unification establishes Value::Bytes as canonical for non-HR; the legacy JS-array path is now handled by json_value_to_platform_value_lenient (added in f135b20) inside the deprecated wasm-dpp crate. Aligned with documented design intent.
• Variable-length byte deserializer has the same Value::Array gap on non-HR input — Intentionally deferred — same rationale and same documented design as the fixed-size helper. The non-HR branch's deserialize_byte_buf is the canonical shape hint matched by the non-HR design contract; the wasm-dpp JS-array path is handled by the lenient helper from f135b20.
• DataContract factory inverts skip_validation into full_validation — Out of scope and not newly introduced. The skip_validation/full_validation parameter-naming mismatch between DataContractFactory::create_from_object (outer) and DataContractFactoryV0::create_from_object (inner) exists verbatim in origin/v3.1-dev (verified via git show origin/v3.1-dev:packages/rs-dpp/src/data_contract/factory/{mod,v0/mod}.rs). This PR's diff in v0/mod.rs only switches between from_value_validated / raw platform_value::from_value while preserving the same full_validation semantics. The naming hazard is real but pre-existing and should be tracked separately.
• WASM fromObject cannot round-trip maps with typed Rust keys (Group.fromObject) — Dropped — speculative and unverified. The finding asserts that platform_value's non-HR Identifier deserializer rejects a base58 Value::Text key, but platform_value::from_value is invoked via a serde derive on GroupV0 whose members: BTreeMap<Identifier, GroupMemberPower> deserialization actually goes through Identifier's Deserialize impl, which accepts both bytes and base58 strings. The codex finding does not cite a failing test or a verified rejection trace; the cited Group.spec.ts round-trip is not shown to fail. Insufficient evidence to surface as a blocking item.

[thepastaclaw]

Code Review

Cumulative verification at e5efc91. Prior-1 (silent NaN/±∞ → JSON 0 coercion in three Value→JsonValue paths in rs-platform-value/src/converter/serde_json.rs) is STILL VALID — verified directly at lines 44, 140, and 289, none of which the delta touched. The latest delta (47f99dea..e5efc91c) only extends the #[json_safe_fields] rollout to AssetLockValueV0, AddressWitness, GroupStateTransitionInfo, and platform-value Bytes20/32/36 markers; it is additive, consistent with the canonical JSON thesis, and introduces no new in-scope defects. Carrying the one prior suggestion forward; no blockers.

_Note: Inline posting failed (command failed (1): gh pr diff 3573 --repo dashpay/platform
STDOUT:

STDERR:
could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead. (https://api.github.com/repos/), so I posted the same verified findings as a top-level review body._

🟡 1 suggestion(s)

1 additional finding(s)

suggestion: Value→JSON silently rewrites NaN/±∞ to 0 in all three converters

packages/rs-platform-value/src/converter/serde_json.rs (line 44)

All three Value→JsonValue paths in this file use JsonValue::Number(Number::from_f64(float).unwrap_or(0.into())): try_into_validating_json at line 44, the by-reference try_to_validating_json at line 140, and TryInto<JsonValue> for Value at line 289. serde_json::Number::from_f64 returns None for NaN, +∞, and −∞ (none are representable in JSON per RFC 8259), so the fallback substitutes the perfectly valid JSON number 0. A Value::Float(NaN) therefore round-trips as Value::U64(0) rather than producing an error — directly contradicting this PR's canonical Value↔JSON round-trip thesis. Although JSON inputs can't introduce NaN/Inf, CBOR, bincode, and direct Rust construction can, so any such Value routed through these converters for JSON-schema validation, storage, or JS-facing wasm output is silently corrupted (and validation may pass against the rewritten 0). The deterministic coercion does not split consensus, but it's still a data-integrity hole at exactly the boundary this PR claims to unify. Mirror the explicit IntegerSizeError/Unsupported arms that already exist for U128/I128 and the Enum* variants in the same match: return an error (e.g., Error::Unsupported("non-finite float not representable in JSON")) at all three sites instead of unwrap_or(0.into()).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [SUGGESTION] In `packages/rs-platform-value/src/converter/serde_json.rs`:44-44: Value→JSON silently rewrites NaN/±∞ to 0 in all three converters
  All three Value→JsonValue paths in this file use `JsonValue::Number(Number::from_f64(float).unwrap_or(0.into()))`: `try_into_validating_json` at line 44, the by-reference `try_to_validating_json` at line 140, and `TryInto<JsonValue> for Value` at line 289. `serde_json::Number::from_f64` returns `None` for NaN, +∞, and −∞ (none are representable in JSON per RFC 8259), so the fallback substitutes the perfectly valid JSON number `0`. A `Value::Float(NaN)` therefore round-trips as `Value::U64(0)` rather than producing an error — directly contradicting this PR's canonical Value↔JSON round-trip thesis. Although JSON inputs can't introduce NaN/Inf, CBOR, bincode, and direct Rust construction can, so any such `Value` routed through these converters for JSON-schema validation, storage, or JS-facing wasm output is silently corrupted (and validation may pass against the rewritten `0`). The deterministic coercion does not split consensus, but it's still a data-integrity hole at exactly the boundary this PR claims to unify. Mirror the explicit `IntegerSizeError`/`Unsupported` arms that already exist for U128/I128 and the Enum* variants in the same `match`: return an error (e.g., `Error::Unsupported("non-finite float not representable in JSON")`) at all three sites instead of `unwrap_or(0.into())`.

[thepastaclaw]

Code Review

Carried-forward verification at head a39740e. Prior finding prior-1 (NaN/±∞ → 0 silent coercion in Value→JsonValue at three sites in rs-platform-value/src/converter/serde_json.rs) is STILL VALID — lines 44, 140, and 289 are unchanged and a new pin test only documents NaN→0 (±∞ remain unpinned and undocumented). The same converter is reached on the attacker-facing document-validation path in rs-dpp (validate_document_properties_v0 calls try_into_validating_json on Document properties before JSON Schema validation), which strengthens the case but doesn't establish a concrete consensus-bypass exploit, so we keep this at suggestion consistent with the prior review. Two additional in-scope suggestions from the latest delta: PlatformAddressWasm's serde Visitor doesn't enforce the 21-byte length that the public fromBytes/fromHex/TryFrom paths now guard, and the new proof-result wrapper VerifiedAssetLockConsumedWithAddressInfosWasm::from_object silently coerces malformed JS credit fields to None. No blocking issues confirmed.

_Note: Inline posting failed (command failed (1): gh pr diff 3573 --repo dashpay/platform
STDOUT:

STDERR:
could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead. (https://api.github.com/repos/), so I posted the same verified findings as a top-level review body._

🟡 3 suggestion(s)

3 additional finding(s)

suggestion: Value→JSON silently rewrites NaN/±∞ to 0 in all three Float converters

packages/rs-platform-value/src/converter/serde_json.rs (line 44)

All three Value→JsonValue paths in this file still use JsonValue::Number(Number::from_f64(float).unwrap_or(0.into())):

• try_into_validating_json (line 44)
• by-reference try_to_validating_json (line 140)
• TryInto<JsonValue> for Value (line 289)

serde_json::Number::from_f64 returns None for NaN, +∞, and −∞, so any non-finite float in a Value is silently rewritten to the JSON number 0 rather than rejected or preserved. The new pin test validating_json_float_nan_becomes_zero (line 1136) only documents NaN at one of the three sites; ±∞ remain unpinned and undocumented at all three.

This contradicts the PR's stated faithful-canonical-conversion goal — Critical-2 in this same PR explicitly removed the analogous silent array→bytes coercion on the inverse path on the same principle. It also matters at a real trust boundary: validate_document_properties_v0 in packages/rs-dpp/src/data_contract/methods/validate_document/v0/mod.rs calls value.try_into_validating_json() on attacker-controlled document properties before JSON Schema validation, and binary deserialization formats can transmit NaN/±∞ floats. A Value::Float(NaN) from a malicious binary state transition will be schema-validated as JSON number 0 instead of being rejected, so numeric constraints (e.g. maximum, const, multipleOf) are not actually being enforced for non-finite floats.

Preferred fix: return Err(Error::Unsupported("non-finite float cannot be represented as JSON Number")) at all three sites so the lossy reshape fails fast, and update / remove the pin test accordingly. Failing that, at minimum extend pin tests to ±∞ at all three sites and document the lossy behavior so future callers don't trust the result.

suggestion: PlatformAddressWasm serde Visitor doesn't enforce the 21-byte length the public paths guard

packages/wasm-dpp2/src/platform_address/address.rs (line 202)

The public TryFrom<JsValue> (Uint8Array branch), TryFrom<&str> hex branch, fromBytes, and fromHex paths in this file all explicitly reject any payload that isn't exactly 21 bytes, with a comment naming the consensus hazard: surplus_output is signed as part of ShieldFromAssetLock, and PlatformAddress::from_bytes uses bincode::decode_from_slice, which silently ignores trailing bytes after a valid 21-byte prefix.

The serde PlatformAddressWasmVisitor paths newly added in this PR — visit_bytes (lines 202–209) and visit_seq (lines 211–222) — call PlatformAddress::from_bytes(...) directly with no length check, so any serde input that reaches the visitor (e.g. a non-human-readable codec round-trip, or a host-supplied Uint8Array routed through deserialize_any) can be silently truncated to a different signed value, reopening the hazard the public paths just closed. Add an explicit value.len() != 21 (and equivalent for the accumulated Vec<u8>) check at both sites before calling PlatformAddress::from_bytes, matching the public surface.

    fn visit_bytes<E>(self, value: &[u8]) -> Result<Self::Value, E>
    where
        E: de::Error,
    {
        if value.len() != 21 {
            return Err(E::custom(format!(
                "PlatformAddress must be exactly 21 bytes, got {}",
                value.len()
            )));
        }
        PlatformAddress::from_bytes(value)
            .map(PlatformAddressWasm)
            .map_err(|e| E::custom(e.to_string()))
    }

    fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
    where
        A: de::SeqAccess<'de>,
    {
        let mut bytes: Vec<u8> = Vec::new();
        while let Some(byte) = seq.next_element::<u8>()? {
            bytes.push(byte);
        }
        if bytes.len() != 21 {
            return Err(A::Error::custom(format!(
                "PlatformAddress must be exactly 21 bytes, got {}",
                bytes.len()
            )));
        }
        PlatformAddress::from_bytes(&bytes)
            .map(PlatformAddressWasm)
            .map_err(|e| A::Error::custom(e.to_string()))
    }

suggestion: from_object silently drops malformed initial/remainingCreditValue to None

packages/wasm-dpp2/src/state_transitions/proof_result/shielded.rs (line 181)

VerifiedAssetLockConsumedWithAddressInfosWasm::from_object reads initialCreditValue / remainingCreditValue with a closure that returns Option<u64> and uses s.parse::<u64>().ok() and an f64 fallback. A JS caller that passes a present-but-malformed value — a string that isn't a base-10 u64, a BigInt outside u64 range, an f64 above u64::MAX as f64 or non-integral, or a wrong-typed value — gets None instead of an error. That loses the distinction between “field absent” and “field corrupt” at the JS→Rust boundary, and lets a buggy JS host silently flip a PartiallyConsumed proof result into a no-credit one.

This was just introduced as part of the PR's canonical JSON/Value unification surface (the prior code dropped to None; the comment specifically calls out round-trip safety). Returning WasmDppResult<Option<u64>> and surfacing a structured error when the field is present but malformed keeps the round-trip path intact while preventing silent data loss. Confidence is moderate because the impact is bounded by JS-host trust on a proof-result consumer surface, not a consensus boundary.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [SUGGESTION] In `packages/rs-platform-value/src/converter/serde_json.rs`:44-44: Value→JSON silently rewrites NaN/±∞ to 0 in all three Float converters
  All three Value→JsonValue paths in this file still use `JsonValue::Number(Number::from_f64(float).unwrap_or(0.into()))`:
- `try_into_validating_json` (line 44)
- by-reference `try_to_validating_json` (line 140)
- `TryInto<JsonValue> for Value` (line 289)

`serde_json::Number::from_f64` returns `None` for NaN, +∞, and −∞, so any non-finite float in a `Value` is silently rewritten to the JSON number `0` rather than rejected or preserved. The new pin test `validating_json_float_nan_becomes_zero` (line 1136) only documents NaN at one of the three sites; ±∞ remain unpinned and undocumented at all three.

This contradicts the PR's stated faithful-canonical-conversion goal — Critical-2 in this same PR explicitly removed the analogous silent array→bytes coercion on the inverse path on the same principle. It also matters at a real trust boundary: `validate_document_properties_v0` in `packages/rs-dpp/src/data_contract/methods/validate_document/v0/mod.rs` calls `value.try_into_validating_json()` on attacker-controlled document properties before JSON Schema validation, and binary deserialization formats can transmit NaN/±∞ floats. A `Value::Float(NaN)` from a malicious binary state transition will be schema-validated as JSON number `0` instead of being rejected, so numeric constraints (e.g. `maximum`, `const`, `multipleOf`) are not actually being enforced for non-finite floats.

Preferred fix: return `Err(Error::Unsupported("non-finite float cannot be represented as JSON Number"))` at all three sites so the lossy reshape fails fast, and update / remove the pin test accordingly. Failing that, at minimum extend pin tests to ±∞ at all three sites and document the lossy behavior so future callers don't trust the result.
- [SUGGESTION] In `packages/wasm-dpp2/src/platform_address/address.rs`:202-222: PlatformAddressWasm serde Visitor doesn't enforce the 21-byte length the public paths guard
  The public `TryFrom<JsValue>` (Uint8Array branch), `TryFrom<&str>` hex branch, `fromBytes`, and `fromHex` paths in this file all explicitly reject any payload that isn't exactly 21 bytes, with a comment naming the consensus hazard: `surplus_output` is signed as part of `ShieldFromAssetLock`, and `PlatformAddress::from_bytes` uses `bincode::decode_from_slice`, which silently ignores trailing bytes after a valid 21-byte prefix.

The serde `PlatformAddressWasmVisitor` paths newly added in this PR — `visit_bytes` (lines 202–209) and `visit_seq` (lines 211–222) — call `PlatformAddress::from_bytes(...)` directly with no length check, so any serde input that reaches the visitor (e.g. a non-human-readable codec round-trip, or a host-supplied Uint8Array routed through `deserialize_any`) can be silently truncated to a different signed value, reopening the hazard the public paths just closed. Add an explicit `value.len() != 21` (and equivalent for the accumulated `Vec<u8>`) check at both sites before calling `PlatformAddress::from_bytes`, matching the public surface.
- [SUGGESTION] In `packages/wasm-dpp2/src/state_transitions/proof_result/shielded.rs`:181-198: from_object silently drops malformed initial/remainingCreditValue to None
  `VerifiedAssetLockConsumedWithAddressInfosWasm::from_object` reads `initialCreditValue` / `remainingCreditValue` with a closure that returns `Option<u64>` and uses `s.parse::<u64>().ok()` and an `f64` fallback. A JS caller that passes a present-but-malformed value — a string that isn't a base-10 u64, a BigInt outside u64 range, an `f64` above `u64::MAX as f64` or non-integral, or a wrong-typed value — gets `None` instead of an error. That loses the distinction between “field absent” and “field corrupt” at the JS→Rust boundary, and lets a buggy JS host silently flip a `PartiallyConsumed` proof result into a no-credit one.

This was just introduced as part of the PR's canonical JSON/Value unification surface (the prior code dropped to `None`; the comment specifically calls out round-trip safety). Returning `WasmDppResult<Option<u64>>` and surfacing a structured error when the field is present but malformed keeps the round-trip path intact while preventing silent data loss. Confidence is moderate because the impact is bounded by JS-host trust on a proof-result consumer surface, not a consensus boundary.

[thepastaclaw]

Code Review

Latest delta (89fb47c) is a pure clippy::type_complexity refactor introducing an AddressInfoMap alias in proof_result.rs with no behavioral change. All three prior findings remain STILL VALID at the current head and are carried forward. No new in-scope findings from the a39740e..89fb47c delta.

_Note: Inline posting failed (command failed (1): gh pr diff 3573 --repo dashpay/platform
STDOUT:

STDERR:
could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead. (https://api.github.com/repos/), so I posted the same verified findings as a top-level review body._

🟡 3 suggestion(s)

3 additional finding(s)

suggestion: PlatformAddressWasm serde Visitor bypasses the 21-byte length guard enforced on every public path

packages/wasm-dpp2/src/platform_address/address.rs (line 202)

All four user-facing entry points — TryFrom<JsValue> Uint8Array branch (116-128), TryFrom<&str> hex branch (165-172), fromBytes (336-348), and fromHex — explicitly reject any payload that is not exactly 21 bytes. The accompanying comments tie the guard to consensus-sensitive surplus_output signing in ShieldFromAssetLock: PlatformAddress::from_bytes decodes via bincode and does not require full-slice consumption, so an over-length buffer with a valid 21-byte prefix is silently truncated and would route funds to a different destination with a still-valid signature.

The serde Visitor::visit_bytes (202-209) and visit_seq (211-222) both call PlatformAddress::from_bytes directly without the length check. Any deserializer that delivers bytes (bincode, MessagePack, CBOR, serde_wasm_bindgen on a typed-array path) bypasses the invariant the rest of the type takes for granted. This PR's scope tightens canonical JSON/Value conversion boundaries; the byte-path Visitor is the remaining hole at the same trust boundary. Mirror the explicit if bytes.len() != 21 rejection in both visitor methods so every entry point shares the invariant.

suggestion: from_object silently drops malformed initialCreditValue/remainingCreditValue to None

packages/wasm-dpp2/src/state_transitions/proof_result/shielded.rs (line 181)

read_opt_u64 returns Option<u64> and swallows all parse failures: s.parse::<u64>().ok() on the string branch and an as_f64-with-bounds check on the numeric branch both convert any unparseable input into None. A JS caller that passes a present-but-malformed value (initialCreditValue: "not_a_number", a negative number, a fractional number, Infinity, NaN) gets a successfully constructed object with None instead of an error.

That conflates field absent (the documented wire convention for no surplus) with field present but garbage, hiding bad proof-result input at the WASM boundary. The leniency justified in the comment above (line 177-180) is only needed for undefined/null and for the BigInt/string/number triple — it does not require swallowing parse errors. Change the helper to WasmDppResult<Option<u64>>: return Ok(None) only for undefined/null, and surface a WasmDppError when a present value cannot be cleanly interpreted as u64.

suggestion: Value::Float NaN/±∞ silently rewritten to JSON 0 in all three Value→JSON converters

packages/rs-platform-value/src/converter/serde_json.rs (line 44)

All three Value→JsonValue paths still use JsonValue::Number(Number::from_f64(float).unwrap_or(0.into())): try_into_validating_json (line 44), the by-ref try_to_validating_json (line 140), and TryInto<JsonValue> for Value (line 289). serde_json::Number::from_f64 returns None for NaN, +∞, and -∞ (RFC 8259 forbids them in JSON), so non-finite floats are silently coerced to JSON 0 instead of being rejected.

This PR's stated goal is canonical, lossless JSON/Value conversion (other size-overflow paths in the same functions already return Error::IntegerSizeError). Silently rewriting NaN/∞ to 0 contradicts that intent and makes Value::Float(f64::NAN) indistinguishable from a real 0 to every JSON consumer — including the new StateTransitionProofResult::to_json path established in this PR. Either return Error::Unsupported/an Error::IntegerSizeError-equivalent, or emit JsonValue::Null. The existing test that pins this behavior (validating_json_float_nan_becomes_zero) should be updated accordingly.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [SUGGESTION] In `packages/wasm-dpp2/src/platform_address/address.rs`:202-222: PlatformAddressWasm serde Visitor bypasses the 21-byte length guard enforced on every public path
  All four user-facing entry points — `TryFrom<JsValue>` Uint8Array branch (116-128), `TryFrom<&str>` hex branch (165-172), `fromBytes` (336-348), and `fromHex` — explicitly reject any payload that is not exactly 21 bytes. The accompanying comments tie the guard to consensus-sensitive `surplus_output` signing in `ShieldFromAssetLock`: `PlatformAddress::from_bytes` decodes via bincode and does not require full-slice consumption, so an over-length buffer with a valid 21-byte prefix is silently truncated and would route funds to a different destination with a still-valid signature.

The serde `Visitor::visit_bytes` (202-209) and `visit_seq` (211-222) both call `PlatformAddress::from_bytes` directly without the length check. Any deserializer that delivers bytes (bincode, MessagePack, CBOR, `serde_wasm_bindgen` on a typed-array path) bypasses the invariant the rest of the type takes for granted. This PR's scope tightens canonical JSON/Value conversion boundaries; the byte-path Visitor is the remaining hole at the same trust boundary. Mirror the explicit `if bytes.len() != 21` rejection in both visitor methods so every entry point shares the invariant.
- [SUGGESTION] In `packages/wasm-dpp2/src/state_transitions/proof_result/shielded.rs`:181-198: from_object silently drops malformed initialCreditValue/remainingCreditValue to None
  `read_opt_u64` returns `Option<u64>` and swallows all parse failures: `s.parse::<u64>().ok()` on the string branch and an `as_f64`-with-bounds check on the numeric branch both convert any unparseable input into `None`. A JS caller that passes a present-but-malformed value (`initialCreditValue: "not_a_number"`, a negative number, a fractional number, `Infinity`, `NaN`) gets a successfully constructed object with `None` instead of an error.

That conflates `field absent` (the documented wire convention for `no surplus`) with `field present but garbage`, hiding bad proof-result input at the WASM boundary. The leniency justified in the comment above (line 177-180) is only needed for `undefined`/`null` and for the BigInt/string/number triple — it does not require swallowing parse errors. Change the helper to `WasmDppResult<Option<u64>>`: return `Ok(None)` only for `undefined`/`null`, and surface a `WasmDppError` when a present value cannot be cleanly interpreted as `u64`.
- [SUGGESTION] In `packages/rs-platform-value/src/converter/serde_json.rs`:44-44: Value::Float NaN/±∞ silently rewritten to JSON 0 in all three Value→JSON converters
  All three Value→JsonValue paths still use `JsonValue::Number(Number::from_f64(float).unwrap_or(0.into()))`: `try_into_validating_json` (line 44), the by-ref `try_to_validating_json` (line 140), and `TryInto<JsonValue> for Value` (line 289). `serde_json::Number::from_f64` returns `None` for `NaN`, `+∞`, and `-∞` (RFC 8259 forbids them in JSON), so non-finite floats are silently coerced to JSON `0` instead of being rejected.

This PR's stated goal is canonical, lossless JSON/Value conversion (other size-overflow paths in the same functions already return `Error::IntegerSizeError`). Silently rewriting NaN/∞ to `0` contradicts that intent and makes `Value::Float(f64::NAN)` indistinguishable from a real `0` to every JSON consumer — including the new `StateTransitionProofResult::to_json` path established in this PR. Either return `Error::Unsupported`/an `Error::IntegerSizeError`-equivalent, or emit `JsonValue::Null`. The existing test that pins this behavior (`validating_json_float_nan_becomes_zero`) should be updated accordingly.

[thepastaclaw]

Code Review

All three prior findings are fixed on the current head: PlatformAddress serde visitors now enforce the 21-byte guard, VerifiedAssetLockConsumedWithAddressInfos errors on malformed present credit values, and Value::Float NaN/±∞ now returns Error::Unsupported in all three Value→JSON converters with pinned regression tests. New delta validation: codex flagged one consistency gap in the JsonConvertible rollout — TokenPricingSchedule's enum variants hold u64 Credits/TokenAmount but do not apply the json_safe_u64 / json_safe_u64_u64_map helpers that other tuple-variant u64 fields in this PR (e.g. TokenDistributionInfo::PreProgrammed) explicitly opt into. The gap is real but non-blocking: the WASM value() getter already exposes prices as BigInt safely, the consensus binary path is unaffected, and realistic token prices won't exceed Number.MAX_SAFE_INTEGER. Downgraded from blocking to suggestion.

_Note: Inline posting failed (command failed (1): gh pr diff 3573 --repo dashpay/platform
STDOUT:

STDERR:
could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead. (https://api.github.com/repos/), so I posted the same verified findings as a top-level review body._

🟡 1 suggestion(s)

1 additional finding(s)

suggestion: TokenPricingSchedule variants miss the json_safe_u64 rollout for Credits / BTreeMap

packages/rs-dpp/src/tokens/token_pricing_schedule.rs (line 34)

Both SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) hold u64 values that go through the new JsonConvertible impl unwrapped. Other tuple-variant u64 fields touched by this PR explicitly opt into the JS-safe helper (see TokenDistributionInfo::PreProgrammed in data_contract/associated_token/token_distribution_key.rs:54-61, which uses serde(with = "crate::serialization::json_safe_u64") precisely because #[json_safe_fields] can't auto-annotate tuple variants). Credits or per-tier prices above 2^53-1 round-trip through to_json as raw serde_json::Number and lose precision in JS clients, breaking the canonical JS-safe invariant the rest of the rollout enforces. Non-blocking in practice because the TokenPricingScheduleWasm.value getter already returns BigInt and realistic prices stay well below MAX_SAFE_INTEGER, but the gap should be closed for rollout consistency. The existing JSON round-trip tests only cover small values (1234, 50, 80) so they will not catch this.

    SinglePrice(
        #[cfg_attr(
            feature = "json-conversion",
            serde(with = "crate::serialization::json_safe_u64")
        )]
        Credits,
    ),

    /// A tiered pricing model where specific token amounts map to credit prices.
    ///
    /// This allows for more complex pricing structures, such as
    /// volume discounts or progressive pricing. The map keys
    /// represent token amount thresholds, and the values are the
    /// corresponding credit prices.
    /// If the first token amount is greater than 1 this means that the user can only
    /// purchase that amount as a minimum at a time.
    SetPrices(
        #[cfg_attr(
            feature = "json-conversion",
            serde(with = "crate::serialization::json::safe_integer_map::json_safe_u64_u64_map")
        )]
        BTreeMap<TokenAmount, Credits>,
    ),

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [SUGGESTION] In `packages/rs-dpp/src/tokens/token_pricing_schedule.rs`:34-44: TokenPricingSchedule variants miss the json_safe_u64 rollout for Credits / BTreeMap<u64,u64>
  Both `SinglePrice(Credits)` and `SetPrices(BTreeMap<TokenAmount, Credits>)` hold u64 values that go through the new JsonConvertible impl unwrapped. Other tuple-variant u64 fields touched by this PR explicitly opt into the JS-safe helper (see `TokenDistributionInfo::PreProgrammed` in `data_contract/associated_token/token_distribution_key.rs:54-61`, which uses `serde(with = "crate::serialization::json_safe_u64")` precisely because `#[json_safe_fields]` can't auto-annotate tuple variants). Credits or per-tier prices above 2^53-1 round-trip through `to_json` as raw `serde_json::Number` and lose precision in JS clients, breaking the canonical JS-safe invariant the rest of the rollout enforces. Non-blocking in practice because the `TokenPricingScheduleWasm.value` getter already returns BigInt and realistic prices stay well below MAX_SAFE_INTEGER, but the gap should be closed for rollout consistency. The existing JSON round-trip tests only cover small values (1234, 50, 80) so they will not catch this.