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 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.

[thepastaclaw]

Code Review

Verified at head 7b057b8. The latest delta is a single scoped change adding #[serde(tag = "$type")] to StoredAssetLockInfo; wire-shape pin tests cover FullyConsumed, NotPresent, and PartiallyConsumed (the inner AssetLockValue's $formatVersion flattens cleanly to the same level), and storage is unaffected because persistence flows through PlatformSerialize/bincode on the inner AssetLockValueV0. Prior finding prior-1 (TokenPricingSchedule u64 JS-safety) is STILL VALID — re-confirmed in the current file at lines 34-44 and pinned to bare-number wire shape by tests at lines 100-158. One additional in-scope gap of the same class is present on TokenConfigurationChangeItem::MaxSupply(Option<TokenAmount>), which is freshly exposed to canonical JSON by this PR.

Reviewed commit: 7b057b8

🟡 2 suggestion(s)

Verified findings

suggestion: TokenPricingSchedule emits raw u64 JSON for Credits/TokenAmount, breaking the PR's JS-safe rollout

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

STILL VALID at 7b057b8. SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) are tuple/newtype variants over raw u64 and the canonical JsonConvertible impl at line 48 emits them as bare JSON numbers — the pin tests at lines 100-158 explicitly assert json!({ "SinglePrice": 1234 }) and json!({ "SetPrices": { "5": 50, "10": 80 } }). Sibling u64-bearing types touched in this same conversion sweep already opt into the helper (e.g. TokenDistributionInfo::PreProgrammed, AssetLockValueV0::initial_credit_value/remaining_credit_value). Once a price exceeds Number.MAX_SAFE_INTEGER (2^53 − 1), JS consumers in wasm-dpp2/wasm-sdk that round-trip this JSON will silently round the value — exactly the bug class this PR is supposed to eliminate. The serde_json::Value paths used by token direct-purchase pricing, proof results, and token events all cross this JS-facing boundary. The cleanest fix is field-level serde(with = ...) annotations on the variant fields; for the map, use the safe_integer_map::json_safe_u64_u64_map helper that other map-bearing variants already use.

suggestion: TokenConfigurationChangeItem::MaxSupply loses precision for token supplies above 2^53−1 in canonical JSON

packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs (line 39)

This PR adds a canonical JsonConvertible for TokenConfigurationChangeItem impl at line 756, newly exposing the MaxSupply(Option<TokenAmount>) tuple variant to the JSON wire. TokenAmount is a u64 alias, the tuple-variant field is not field-annotated, and the round-trip fixtures assert a raw JSON number for maxSupply. JS clients consuming proof results, configuration update previews, or TokenEvent::ConfigUpdate payloads will silently round any supply cap above Number.MAX_SAFE_INTEGER, mirroring the gap on TokenPricingSchedule. Annotate the variant field with serde(with = "crate::serialization::json_safe_option_u64") under feature = "json-conversion" and update the pinned wire-shape test to assert the string form for above-safe-integer values. This is in scope because the canonical JsonConvertible impl is introduced by this PR.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3573 7b057b8 --dry-run
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 857, in
result = post_review(
File ), so I posted the same verified findings as a top-level review body._

[thepastaclaw]

Code Review

Verified at head fff0f5c. Latest delta 7b057b8..fff0f5c is test-only — strengthening BlockInfo and ValidatorSet wire-shape fixtures in packages/rs-dpp/src/block/block_info/mod.rs and packages/rs-dpp/src/core_types/validator_set/mod.rs — no production code changed, no new findings. Prior findings prior-1 (TokenPricingSchedule raw u64 JSON) and prior-2 (TokenConfigurationChangeItem::MaxSupply raw u64 JSON) re-validated by direct file read at fff0f5c: both STILL VALID and carried forward. The flagged files were not touched in the delta; enum variants still derive serde Serialize/Deserialize over u64 aliases with no json-safe annotation, and the pin tests still assert raw-number JSON shapes.

Reviewed commit: fff0f5c

🟡 2 suggestion(s)

Verified findings

suggestion: TokenPricingSchedule emits raw u64 JSON for Credits/TokenAmount, bypassing the PR's JS-safe rollout

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

Verified at fff0f5c: SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) remain tuple/newtype variants over raw u64 aliases. The canonical JsonConvertible impl at line 48 is the empty blanket impl, so wire shape is fully derived from #[derive(Serialize, Deserialize)] (line 28). The pin tests at lines 105 and 120 explicitly assert raw-number JSON: {"SinglePrice": 1234} and {"SetPrices": {"5": 50, "10": 80}}. No #[json_safe_fields] or per-field serde(with) stringifier is applied.

This PR's stated goal is JS-safe large-integer protection at every canonical JSON serialization site (~25 V0/V1 leaves carry #[json_safe_fields]). This type is on the canonical JSON wire and is reachable from wasm-dpp2 through impl_wasm_conversions_inner! → JsonConvertible::to_json → json_to_js_value (which uses serde_wasm_bindgen::Serializer::json_compatible() without serialize_large_number_types_as_strings), so any Credits value above 2^53−1 (≈9.007e15) loses precision on the JS side. Credit values legitimately exceed that range (1 DASH = 1e11 credits), making this exploitable in pricing UIs where a high-value token deal could be misrepresented to a user before approval.

The value getter on TokenPricingScheduleWasm carefully uses bigint_from_str to avoid this, but the canonical toJSON path defeats that protection. #[json_safe_fields] only applies to named struct leaves, so the fix likely requires either promoting these variants to named-field structs with the attribute, or adding a custom serde adapter for the u64 payloads. The pin tests at lines 100–158 would need to be updated to the string-shaped JSON as part of the fix.

suggestion: TokenConfigurationChangeItem::MaxSupply loses precision for token supplies above 2^53−1 in canonical JSON

packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs (line 39)

Verified at fff0f5c: MaxSupply(Option<TokenAmount>) (line 39) is a tuple variant over the u64 alias TokenAmount. The enum derives Serialize, Deserialize with only rename_all = "camelCase" (lines 28–32); no #[json_safe_fields] or serde(with = "…") is applied. The canonical JsonConvertible impl is the blanket empty impl, so the wire shape is fully serde-derived and emits a bare JSON number. The existing round-trip test (elsewhere in the file) pins the shape as {"maxSupply": 123456789}.

Governance over a token's max supply is exactly the kind of value that can legitimately exceed 2^53−1 (low-decimal, high-supply fungible tokens or large NFT-style enumerations). When this enum is the payload of a token-configuration change vote and is rendered or re-parsed by a JS wallet/admin UI through wasm-dpp2's toJSON/fromJSON, the displayed/approved cap can silently differ from the on-chain change — a signing-integrity hazard for multi-sig token administrators.

Same class of issue as the TokenPricingSchedule finding: tuple/newtype enum variant payloads over u64 aliases bypass the PR's #[json_safe_fields] convention. The fix is either a named-field representation for the variant or a custom serde adapter for the Option<TokenAmount> payload, plus an updated pin test asserting the string-shaped JSON.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3573 fff0f5c --dry-run
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 857, in
result = post_review(
File ), so I posted the same verified findings as a top-level review body._

[thepastaclaw]

Code Review

Latest delta (validator_set test fixture refactor at ad2d37f) is test-only with no production impact. Re-verified both prior fff0f5c findings against current source: TokenPricingSchedule still derives serde over u64 aliases with the blanket JsonConvertible impl (token_pricing_schedule.rs:28-51), and TokenConfigurationChangeItem::MaxSupply still uses Option with the blanket impl at token_configuration_item.rs:756. Both are STILL VALID, carried forward as convergent suggestions (claude+codex).

🟡 2 suggestion(s)

Prior-finding reconciliation

• Prior #1 (TokenPricingSchedule raw Credits/TokenAmount JSON): STILL VALID / carried forward as an active suggestion below.
• Prior #2 (TokenConfigurationChangeItem::MaxSupply raw TokenAmount JSON): STILL VALID / carried forward as an active suggestion below.

New findings in the latest delta

No new defects were introduced by fff0f5cf..ad2d37f8; the latest delta is limited to test fixture refactoring for validator-set BLS keys.

Verified active findings

suggestion: TokenPricingSchedule canonical JSON emits raw u64 for Credits/TokenAmount, bypassing the PR's JS-safe rollout

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

Verified at ad2d37f. SinglePrice(Credits) (line 34) and SetPrices(BTreeMap<TokenAmount, Credits>) (line 44) are tuple/newtype variants over the u64 aliases Credits and TokenAmount. The enum derives serde with no field attributes (line 28), and the canonical JsonConvertible/ValueConvertible impls at lines 48 and 51 are the empty blanket impls, so the wire shape is derived entirely from serde. The wire-shape tests at lines 100-123 pin the bare-integer form ({"SinglePrice": 1234}, {"SetPrices": {"5": 50, "10": 80}}), confirming there is no #[json_safe_fields] and no serde(with = ...) u64-string adapter.

This is in scope for the PR: wasm-dpp2's TokenPricingScheduleWasm.toJSON() (wired through impl_wasm_conversions_inner! in packages/wasm-dpp2/src/state_transitions/batch/token_pricing_schedule.rs:115-119) routes through this canonical path, while the hand-written value() getter on the same wrapper already uses bigint_from_str(...) for both SinglePrice and SetPrices keys/values — proving the team treats raw u64 as unsafe across this boundary on the hand-written path. Credits and TokenAmount are u64; values above 2^53−1 silently lose precision when round-tripped through JS JSON.parse. The same gap propagates through BatchTransitionWasm.toJSON() for direct-purchase price transitions.

Apply the same #[json_safe_fields] (or serde(with = json_safe_u64 / json_safe_u64_u64_map)) treatment used on the ~25 V0/V1 leaves in this PR, and update the wire-shape tests to assert the string-encoded form.

suggestion: TokenConfigurationChangeItem::MaxSupply canonical JSON loses precision for token supplies above 2^53−1

packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs (line 28-39)

Verified at ad2d37f. MaxSupply(Option<TokenAmount>) (line 39) is a tuple variant over the u64 alias TokenAmount. The enum derives Serialize, Deserialize with only serde(rename_all = "camelCase") (lines 28-32); no #[json_safe_fields] and no serde(with = ...) adapter. The canonical JsonConvertible impl at line 756 is the empty blanket impl, so to_json falls through to plain serde_json::to_value, emitting the inner u64 as a JSON Number.

This variant exists precisely to express full-u64 token supplies, which is the canonical case where JS Number rounding bites. The variant-internal round-trip test at lines 779-793 pins {"maxSupply": 123_456_789u64} — a 9-digit value well under 2^53 — so the convention doesn't exercise the JS-safe boundary. The same JsonConvertible path is reachable from JS through TokenConfigurationChangeItemWasm::max_supply_item (packages/wasm-dpp2/src/tokens/configuration_change_item/items/max_supply.rs) and through nested emission via BatchTransitionWasm.toJSON() for TokenConfigUpdateTransitionV0.update_token_configuration_item.

Apply #[json_safe_fields] (or serde(with = json_safe_option_u64)) on the variant payload, and update the test to exercise a value above 2^53−1 to lock in the safe wire shape — matching the convention this PR rolls out on V0/V1 leaves.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR scope and should be handled separately.

• BLS pubkey HR/non-HR deserialize divergence in blstrs_plus leaves a ValidatorSet round-trip test permanently #[ignore] — The latest delta's value-round-trip test for ValidatorSet (packages/rs-dpp/src/core_types/validator_set/mod.rs) remains #[ignore] because BlsPublicKey<Bls12381G2Impl> routes through the local bls_pubkey_serde wrapper while the inner blstrs_plus Deserialize borrows a &str and fails through the non-HR (Value) path. The current commit improves the test by baking BLS pubkey bytes from shared hex consts (removing circular interpolation), but the underlying upstream blstrs_plus deserialize fix is outside this PR's scope.
  Follow-up: Track the upstream blstrs_plus dual-shape deserialize fix; once the dep is bumped, drop the #[ignore] and remove the local bls_pubkey_serde wrapper in a separate maintenance PR.

🤖 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:29-51`: TokenPricingSchedule canonical JSON emits raw u64 for Credits/TokenAmount, bypassing the PR's JS-safe rollout
  Verified at ad2d37f8. `SinglePrice(Credits)` (line 34) and `SetPrices(BTreeMap<TokenAmount, Credits>)` (line 44) are tuple/newtype variants over the `u64` aliases `Credits` and `TokenAmount`. The enum derives serde with no field attributes (line 28), and the canonical `JsonConvertible`/`ValueConvertible` impls at lines 48 and 51 are the empty blanket impls, so the wire shape is derived entirely from serde. The wire-shape tests at lines 100-123 pin the bare-integer form (`{"SinglePrice": 1234}`, `{"SetPrices": {"5": 50, "10": 80}}`), confirming there is no `#[json_safe_fields]` and no `serde(with = ...)` u64-string adapter.
  
  This is in scope for the PR: wasm-dpp2's `TokenPricingScheduleWasm.toJSON()` (wired through `impl_wasm_conversions_inner!` in packages/wasm-dpp2/src/state_transitions/batch/token_pricing_schedule.rs:115-119) routes through this canonical path, while the hand-written `value()` getter on the same wrapper already uses `bigint_from_str(...)` for both `SinglePrice` and `SetPrices` keys/values — proving the team treats raw u64 as unsafe across this boundary on the hand-written path. Credits and TokenAmount are u64; values above 2^53−1 silently lose precision when round-tripped through JS `JSON.parse`. The same gap propagates through `BatchTransitionWasm.toJSON()` for direct-purchase price transitions.
  
  Apply the same `#[json_safe_fields]` (or `serde(with = json_safe_u64 / json_safe_u64_u64_map)`) treatment used on the ~25 V0/V1 leaves in this PR, and update the wire-shape tests to assert the string-encoded form.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs:28-39`: TokenConfigurationChangeItem::MaxSupply canonical JSON loses precision for token supplies above 2^53−1
  Verified at ad2d37f8. `MaxSupply(Option<TokenAmount>)` (line 39) is a tuple variant over the `u64` alias `TokenAmount`. The enum derives `Serialize, Deserialize` with only `serde(rename_all = "camelCase")` (lines 28-32); no `#[json_safe_fields]` and no `serde(with = ...)` adapter. The canonical `JsonConvertible` impl at line 756 is the empty blanket impl, so `to_json` falls through to plain `serde_json::to_value`, emitting the inner u64 as a JSON `Number`.
  
  This variant exists precisely to express full-u64 token supplies, which is the canonical case where JS `Number` rounding bites. The variant-internal round-trip test at lines 779-793 pins `{"maxSupply": 123_456_789u64}` — a 9-digit value well under 2^53 — so the convention doesn't exercise the JS-safe boundary. The same `JsonConvertible` path is reachable from JS through `TokenConfigurationChangeItemWasm::max_supply_item` (packages/wasm-dpp2/src/tokens/configuration_change_item/items/max_supply.rs) and through nested emission via `BatchTransitionWasm.toJSON()` for `TokenConfigUpdateTransitionV0.update_token_configuration_item`.
  
  Apply `#[json_safe_fields]` (or `serde(with = json_safe_option_u64)`) on the variant payload, and update the test to exercise a value above 2^53−1 to lock in the safe wire shape — matching the convention this PR rolls out on V0/V1 leaves.

[thepastaclaw]

Code Review

Latest delta at 51d3193 cleanly migrates DistributionFunction and RewardDistributionType to internally-tagged serde with consistent test/fixture updates and no new issues introduced. Both prior suggestion-level findings from ad2d37f remain STILL VALID: TokenPricingSchedule and TokenConfigurationChangeItem::MaxSupply derive plain serde over u64 aliases without the PR's #[json_safe_fields] adapter that is applied to 10+ other DPP types (including files in this delta), so they emit raw u64 on the canonical JSON wire and lose precision in JS consumers above 2^53-1. Categorized as bugs/suggestions rather than security since signing/consensus paths use bincode, not JSON.

🟡 2 suggestion(s)

Note: review_poster dry-run could not map inline comments (pay/platform failed: 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/dashpay/platform/pulls/3573)
PullRequest.diff too_large
), so I posted the same verified findings as a body-only review.

Verified active findings

suggestion: TokenPricingSchedule canonical JSON emits raw u64 for Credits/TokenAmount, missing the PR's JS-safe rollout

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

Verified STILL VALID at 51d3193. SinglePrice(Credits) (line 34) and SetPrices(BTreeMap<TokenAmount, Credits>) (line 44) are tuple/newtype variants over the u64 aliases Credits and TokenAmount. The enum derives serde at line 28 with no field-level adapters, and the canonical JsonConvertible (line 48) / ValueConvertible (line 51) impls are empty blanket impls — so the wire shape is derived directly from serde. The in-tree tests pin the unsafe shape: json_round_trip_single_price asserts {"SinglePrice": 1234} (line 105) and json_round_trip_set_prices asserts {"SetPrices": {"5": 50, "10": 80}} (line 120) — values land as JSON Numbers and lose precision above 2^53-1 in any JS-side parser. This contradicts the PR's stated convention applied to 10+ other DPP types via #[json_safe_fields] (including reward_distribution_type/mod.rs and distribution_function/mod.rs modified in this same delta). Credits is u64 in the smallest unit (1 Dash = 1e11 credits), so values above 2^53-1 are realistically reachable. Apply serde(with = "crate::serialization::json_safe_u64") to SinglePrice and serde(with = "crate::serialization::json::safe_integer_map::json_safe_u64_u64_map") to SetPrices (or add #[json_safe_fields]) and update the round-trip tests to assert string-encoded integers.

suggestion: TokenConfigurationChangeItem::MaxSupply canonical JSON loses precision for supplies above 2^53-1

packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs (line 28-39)

Verified STILL VALID at 51d3193. MaxSupply(Option<TokenAmount>) at line 39 is a tuple variant over the u64 alias TokenAmount. The enum derives Serialize, Deserialize at lines 28-32 with only serde(rename_all = "camelCase") — no #[json_safe_fields] and no per-field serde(with = ...) adapter. The canonical JsonConvertible / ValueConvertible impls are empty blanket impls, so canonical JSON falls through to serde's default u64-as-Number encoding. The round-trip test at line 790 explicitly pins the unsafe shape: assert_eq!(json, json!({"maxSupply": 123_456_789u64})). Token supply caps are routinely chosen as round powers of 10 (10^18 for ERC-20-style 18-decimal tokens), so this boundary is realistically reachable and would silently truncate the cap in JS consumers via wasm-dpp2 / wasm-sdk. Same gap as TokenPricingSchedule — the PR's #[json_safe_fields] rollout did not reach this enum. Apply serde(with = "crate::serialization::json_safe_option_u64") to the MaxSupply payload and update the round-trip test to assert the string-encoded shape.

🤖 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`:28-51: TokenPricingSchedule canonical JSON emits raw u64 for Credits/TokenAmount, missing the PR's JS-safe rollout
  Verified STILL VALID at 51d31930. `SinglePrice(Credits)` (line 34) and `SetPrices(BTreeMap<TokenAmount, Credits>)` (line 44) are tuple/newtype variants over the `u64` aliases `Credits` and `TokenAmount`. The enum derives serde at line 28 with no field-level adapters, and the canonical `JsonConvertible` (line 48) / `ValueConvertible` (line 51) impls are empty blanket impls — so the wire shape is derived directly from serde. The in-tree tests pin the unsafe shape: `json_round_trip_single_price` asserts `{"SinglePrice": 1234}` (line 105) and `json_round_trip_set_prices` asserts `{"SetPrices": {"5": 50, "10": 80}}` (line 120) — values land as JSON Numbers and lose precision above 2^53-1 in any JS-side parser. This contradicts the PR's stated convention applied to 10+ other DPP types via `#[json_safe_fields]` (including `reward_distribution_type/mod.rs` and `distribution_function/mod.rs` modified in this same delta). `Credits` is u64 in the smallest unit (1 Dash = 1e11 credits), so values above 2^53-1 are realistically reachable. Apply `serde(with = "crate::serialization::json_safe_u64")` to `SinglePrice` and `serde(with = "crate::serialization::json::safe_integer_map::json_safe_u64_u64_map")` to `SetPrices` (or add `#[json_safe_fields]`) and update the round-trip tests to assert string-encoded integers.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs`:28-39: TokenConfigurationChangeItem::MaxSupply canonical JSON loses precision for supplies above 2^53-1
  Verified STILL VALID at 51d31930. `MaxSupply(Option<TokenAmount>)` at line 39 is a tuple variant over the `u64` alias `TokenAmount`. The enum derives `Serialize, Deserialize` at lines 28-32 with only `serde(rename_all = "camelCase")` — no `#[json_safe_fields]` and no per-field `serde(with = ...)` adapter. The canonical `JsonConvertible` / `ValueConvertible` impls are empty blanket impls, so canonical JSON falls through to serde's default u64-as-Number encoding. The round-trip test at line 790 explicitly pins the unsafe shape: `assert_eq!(json, json!({"maxSupply": 123_456_789u64}))`. Token supply caps are routinely chosen as round powers of 10 (10^18 for ERC-20-style 18-decimal tokens), so this boundary is realistically reachable and would silently truncate the cap in JS consumers via wasm-dpp2 / wasm-sdk. Same gap as TokenPricingSchedule — the PR's `#[json_safe_fields]` rollout did not reach this enum. Apply `serde(with = "crate::serialization::json_safe_option_u64")` to the `MaxSupply` payload and update the round-trip test to assert the string-encoded shape.