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

Verified at head 417114c: latest delta correctly fixes prior-2 by introducing TokenConfigurationChangeItemRepr with json_safe_option_u64 on MaxSupply (line 122) plus a >MAX_SAFE_INTEGER regression test (line 894). Prior-1 (TokenPricingSchedule still emits raw u64 Credits/TokenAmount in canonical JSON, line 28) and prior-3 (TokenDistributionInfo::Perpetual and TokenDistributionTypeWithResolvedRecipient lack direct round-trip tests) are STILL VALID at current head and carried forward. No new in-scope defects introduced by the latest delta.

Reviewed commit: 417114c

🟡 2 suggestion(s)

Verified findings

suggestion: TokenPricingSchedule canonical JSON emits raw u64 for Credits/TokenAmount — JS clients silently lose precision above 2^53−1

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

Re-verified at head 417114c: TokenPricingSchedule still derives plain Serialize, Deserialize (line 28) with tuple variants SinglePrice(Credits) (line 34) and SetPrices(BTreeMap<TokenAmount, Credits>) (line 44). Both Credits and TokenAmount are u64 aliases. JsonConvertible (line 48) uses plain serde with no #[json_safe_fields], no per-field serde(with = json_safe_u64), and no Repr adapter.

Impact: any JS/browser SDK consuming canonical JSON via wasm-dpp2/wasm-sdk silently rounds Credits or TokenAmount values above 9_007_199_254_740_991 (Number.MAX_SAFE_INTEGER) to the nearest IEEE-754 double. Because credits use 100_000_000_000 credits per Dash, prices above ~90,073 Dash already exceed the safe range. A JS client could compute the wrong purchase tier, or do a to_json → from_json that the Rust side disagrees with — a silent precision bug at the Rust↔JS trust boundary.

This PR establishes json_safe_u64 / json_safe_option_u64 as the canonical pattern, including in this very delta where TokenConfigurationChangeItemRepr::MaxSupply gains json_safe_option_u64 (token_configuration_item.rs:122) with a >MAX_SAFE_INTEGER regression test. The same fix shape applies here — because tuple variants cannot accept per-field serde attributes, introduce a Repr adapter with serde(with = json_safe_u64) on the inner u64 payloads (including the BTreeMap keys/values). Bincode Encode/Decode (consensus binary) is untouched and stays correct.

suggestion: New Repr branches lack direct round-trip tests for TokenDistributionInfo::Perpetual and TokenDistributionTypeWithResolvedRecipient

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322)

Re-verified at head 417114c: json_convertible_tests_token_distribution_info (lines 322–389) still only exercises TokenDistributionInfo::PreProgrammed via fixture() (line 337). The two internal-tag Repr adapters introduced earlier in this PR (TokenDistributionInfoRepr at lines 105–120, TokenDistributionTypeWithResolvedRecipient JsonConvertible/ValueConvertible at lines 209/212) define wire shapes that flow across the Rust↔JS FFI boundary, but key shapes remain unpinned:

• TokenDistributionInfo::Perpetual — the wire shape nests RewardDistributionMoment (itself internally tagged) under moment and TokenDistributionResolvedRecipient under recipient; no JSON or platform_value assertion locks this nested shape in. A future tag/key rename or visitor regression would not be caught.
• TokenDistributionTypeWithResolvedRecipient — both PreProgrammed(Identifier) and Perpetual(...) variants round-trip an Identifier through serde's internal-tag Content buffer (the path that historically had typed-bytes pitfalls between Value::Identifier and Value::Bytes32); no direct round-trip test module exists for this type at all.

Adding json_round_trip_perpetual for TokenDistributionInfo and a json_convertible_tests_token_distribution_type_with_resolved_recipient module covering both variants would lock the new wire shapes in and match the per-variant convention established elsewhere in this PR (e.g., the new json_large_max_supply_serializes_as_string_for_js_safety on TokenConfigurationChangeItem).

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3573 417114c --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 PR #3573 at head 40fa0e9. Latest delta (ArrayItemType internal-tag adapter + clippy allow on RewardDistributionMomentRepr) is narrow serde plumbing with no new blocking issues. Both prior 417114c findings are STILL VALID at this head and carried forward: (1) TokenPricingSchedule canonical JSON still emits raw u64 prices, missing the PR's JS-safe u64 convention; (2) TokenDistributionInfo::Perpetual and TokenDistributionTypeWithResolvedRecipient Repr branches still lack direct round-trip tests. One new in-scope test-coverage gap from codex in the latest delta: ArrayItemType Boolean/Date repr branches are untested. No blockers; review action COMMENT.

🟡 3 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 Credits/TokenAmount — JS clients silently round values above 2^53−1

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

TokenPricingSchedule derives plain Serialize, Deserialize (line 28) on tuple variants SinglePrice(Credits) (line 34) and SetPrices(BTreeMap<TokenAmount, Credits>) (line 44); both aliases are u64. The canonical JsonConvertible/ValueConvertible impls (lines 48, 51) are serde-based, and the round-trip tests at lines 105 and 120 confirm the JSON form emits bare numbers ({"SinglePrice": 1234}, {"SetPrices": {"5": 50, "10": 80}}). This conflicts with the PR's stated #[json_safe_fields] convention for JS-safe large-integer protection at every canonical JSON site — same risk class as the already-protected TimestampMillis on TokenDistributionInfoRepr (token_distribution_key.rs:109-113).

Concrete cross-language path: wasm-dpp2 exposes this canonical JSON via impl_wasm_conversions_inner at packages/wasm-dpp2/src/state_transitions/batch/token_pricing_schedule.rs and round-trips it through json_to_js_value (packages/wasm-dpp2/src/serialization/conversions.rs), which emits plain JS numbers, not BigInt. The same type's dedicated JS getters return BigInt, so the toJSON/fromJSON path silently diverges from the typed path. Token credit prices above ~9.007e15 (Number.MAX_SAFE_INTEGER) round to the nearest double on the JS side; the on-chain bincode/PlatformSerialize path is unaffected (no consensus risk). Recommend applying the project's json_safe_u64 adapter to the Credits/TokenAmount payloads through a Repr (matching the pattern already used elsewhere in this PR), with paired wire-shape tests.

suggestion: No direct round-trip tests for TokenDistributionInfo::Perpetual or TokenDistributionTypeWithResolvedRecipient Repr branches

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322-389)

json_convertible_tests_token_distribution_info (lines 328-389) has a single fixture() (lines 336-338) that builds TokenDistributionInfo::PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32])) and exercises only that variant on both JSON and Value paths. The PR-added Repr adapters at lines 46-51 (TokenDistributionTypeWithResolvedRecipientRepr) and lines 105-120 (TokenDistributionInfoRepr) also define Perpetual branches embedding RewardDistributionMoment and TokenDistributionResolvedRecipient, and TokenDistributionTypeWithResolvedRecipient has its own JsonConvertible/ValueConvertible impls (lines 209, 212) with no test module at all (only json_convertible_tests_token_distribution_type_and_key at line 238 and json_convertible_tests_token_distribution_info at line 328 exist).

A silent variant flip, inner-zero, or future serde-shape regression on the Perpetual adapter or on the resolved-recipient enum would not be caught at this layer. The PR establishes per-variant wire-shape literal assertions as the convention for the rest of the unification pass; close this gap by adding json!{} / platform_value!{} round-trip tests for the Perpetual branch of TokenDistributionInfo and for both variants of TokenDistributionTypeWithResolvedRecipient.

suggestion: ArrayItemType Boolean and Date variants lack canonical JSON/Value round-trip tests

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 42-44)

The latest-delta ArrayItemTypeRepr adds Boolean (line 43) and Date (line 44) as new internally-tagged unit variants, with From/Into arms at lines 58-59 and 77-78. The json_convertible_tests module (lines 712-842) covers Integer, Number, String, ByteArray, and Identifier but never asserts the canonical wire shape for Boolean or Date. Per the PR's convention of pinning every conversion branch with a json!{} / platform_value!{} literal, the two unit variants need direct round-trip assertions ({"type":"boolean"} and {"type":"date"}) so their discriminators are locked in and a future rename or tag-strategy regression fails a focused test.

🤖 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-45: TokenPricingSchedule canonical JSON emits raw u64 Credits/TokenAmount — JS clients silently round values above 2^53−1
  `TokenPricingSchedule` derives plain `Serialize, Deserialize` (line 28) on tuple variants `SinglePrice(Credits)` (line 34) and `SetPrices(BTreeMap<TokenAmount, Credits>)` (line 44); both aliases are `u64`. The canonical `JsonConvertible`/`ValueConvertible` impls (lines 48, 51) are serde-based, and the round-trip tests at lines 105 and 120 confirm the JSON form emits bare numbers (`{"SinglePrice": 1234}`, `{"SetPrices": {"5": 50, "10": 80}}`). This conflicts with the PR's stated `#[json_safe_fields]` convention for JS-safe large-integer protection at every canonical JSON site — same risk class as the already-protected `TimestampMillis` on `TokenDistributionInfoRepr` (token_distribution_key.rs:109-113).

Concrete cross-language path: wasm-dpp2 exposes this canonical JSON via `impl_wasm_conversions_inner` at packages/wasm-dpp2/src/state_transitions/batch/token_pricing_schedule.rs and round-trips it through `json_to_js_value` (packages/wasm-dpp2/src/serialization/conversions.rs), which emits plain JS numbers, not BigInt. The same type's dedicated JS getters return BigInt, so the toJSON/fromJSON path silently diverges from the typed path. Token credit prices above ~9.007e15 (Number.MAX_SAFE_INTEGER) round to the nearest double on the JS side; the on-chain bincode/PlatformSerialize path is unaffected (no consensus risk). Recommend applying the project's `json_safe_u64` adapter to the `Credits`/`TokenAmount` payloads through a Repr (matching the pattern already used elsewhere in this PR), with paired wire-shape tests.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs`:322-389: No direct round-trip tests for TokenDistributionInfo::Perpetual or TokenDistributionTypeWithResolvedRecipient Repr branches
  `json_convertible_tests_token_distribution_info` (lines 328-389) has a single `fixture()` (lines 336-338) that builds `TokenDistributionInfo::PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32]))` and exercises only that variant on both JSON and Value paths. The PR-added Repr adapters at lines 46-51 (`TokenDistributionTypeWithResolvedRecipientRepr`) and lines 105-120 (`TokenDistributionInfoRepr`) also define `Perpetual` branches embedding `RewardDistributionMoment` and `TokenDistributionResolvedRecipient`, and `TokenDistributionTypeWithResolvedRecipient` has its own `JsonConvertible`/`ValueConvertible` impls (lines 209, 212) with no test module at all (only `json_convertible_tests_token_distribution_type_and_key` at line 238 and `json_convertible_tests_token_distribution_info` at line 328 exist).

A silent variant flip, inner-zero, or future serde-shape regression on the `Perpetual` adapter or on the resolved-recipient enum would not be caught at this layer. The PR establishes per-variant wire-shape literal assertions as the convention for the rest of the unification pass; close this gap by adding `json!{}` / `platform_value!{}` round-trip tests for the `Perpetual` branch of `TokenDistributionInfo` and for both variants of `TokenDistributionTypeWithResolvedRecipient`.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/document_type/property/array.rs`:42-44: ArrayItemType Boolean and Date variants lack canonical JSON/Value round-trip tests
  The latest-delta `ArrayItemTypeRepr` adds `Boolean` (line 43) and `Date` (line 44) as new internally-tagged unit variants, with `From`/`Into` arms at lines 58-59 and 77-78. The `json_convertible_tests` module (lines 712-842) covers `Integer`, `Number`, `String`, `ByteArray`, and `Identifier` but never asserts the canonical wire shape for `Boolean` or `Date`. Per the PR's convention of pinning every conversion branch with a `json!{}` / `platform_value!{}` literal, the two unit variants need direct round-trip assertions (`{"type":"boolean"}` and `{"type":"date"}`) so their discriminators are locked in and a future rename or tag-strategy regression fails a focused test.

[thepastaclaw]

Code Review

Incremental review against HEAD 319619c (delta from 40fa0e9: BLS pubkey serde wrapper switched to deserialize_any with hex/bytes/seq visitors, ValidatorSet value round-trip test un-ignored, SPEC doc added). All three coordinator-supplied prior findings re-validated as STILL VALID at HEAD and carried forward. One new convergent suggestion on the BLS visit_seq path. No blocking issues; this is a quality-pass review.

Reviewed commit: 319619c

🟡 3 suggestion(s) | 💬 1 nitpick(s)

Verified findings

suggestion: TokenPricingSchedule canonical JSON emits raw u64 Credits/TokenAmount — JS clients silently round above 2^53−1

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

STILL VALID at 319619c. TokenPricingSchedule derives plain Serialize, Deserialize (line 28) over SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) (both u64), and its canonical JsonConvertible/ValueConvertible impls (lines 48, 51) flow through serde. The round-trip tests (lines 105, 120) explicitly pin the JSON form as bare numbers ({"SinglePrice": 1234}, {"SetPrices": {"5": 50, "10": 80}}).

Cross-language path: wasm-dpp2 exposes this same canonical JSON via impl_wasm_conversions_inner (packages/wasm-dpp2/src/state_transitions/batch/token_pricing_schedule.rs:115-119) and round-trips through json_to_js_value, which emits plain JS Numbers — while the typed wasm getters on the same object return BigInt. Token prices/amounts above Number.MAX_SAFE_INTEGER (≈9.007e15) silently round on the JS toJSON/fromJSON path while the typed accessor stays exact.

This conflicts with the PR's #[json_safe_fields] convention for JS-safe large integers — the same risk class already mitigated for TimestampMillis on TokenDistributionInfoRepr (token_distribution_key.rs:109-113). No on-chain consensus impact (bincode/PlatformSerialize unaffected).

Apply the project's json_safe_u64 adapter via a TokenPricingScheduleRepr (matching the Repr pattern already used in this PR) and add wire-shape tests that pin the safe-string form for values > 2^53.

suggestion: No round-trip tests for TokenDistributionInfo::Perpetual or TokenDistributionTypeWithResolvedRecipient Repr branches

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322)

STILL VALID at 319619c. json_convertible_tests_token_distribution_info (lines 328-389) has a single fixture() that builds TokenDistributionInfo::PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32])) and exercises only that variant. The PR-added Repr adapters define Perpetual branches embedding RewardDistributionMoment and TokenDistributionResolvedRecipient, and TokenDistributionTypeWithResolvedRecipient has its own JsonConvertible/ValueConvertible impls with no test module at all.

Cross-language consequence: TokenDistributionTypeWithResolvedRecipient flows out to JS via TokenEventWasm.toJSON/fromJSON (packages/wasm-dpp2/src/group/token_event.rs:113-119). A silent variant flip, inner-zero, or future serde tag/payload regression on the Perpetual adapter or the resolved-recipient enum would not fail any focused test.

Close the gap with literal json!{} / platform_value!{} round-trip assertions for the Perpetual branch of TokenDistributionInfo and for both variants of TokenDistributionTypeWithResolvedRecipient, matching the per-variant wire-shape convention this PR establishes elsewhere.

suggestion: ArrayItemType Boolean and Date variants lack canonical JSON/Value round-trip tests

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 42)

STILL VALID at 319619c. ArrayItemTypeRepr (lines 42-44) includes Boolean and Date as internally-tagged unit variants, with From/Into arms wired at lines 58-59 and 77-78. The json_convertible_tests module covers Integer, Number, String, ByteArray, and Identifier but never asserts the canonical wire shape for Boolean or Date (verified: the only boolean/Boolean test hits in this file are sanitize_value_mut encode tests at lines 399-498, not Repr round-trips).

Per the PR's own convention of pinning every conversion branch with a json!{} / platform_value!{} literal, the two unit variants need direct round-trip assertions ({"type":"boolean"} and {"type":"date"}) so their discriminators are locked in and a future rename or tag-strategy regression fails a focused test rather than silently changing the document-schema wire shape consumed by JS/Swift clients.

nitpick: visit_seq buffers entire sequence before enforcing 48-byte length

packages/rs-dpp/src/core_types/bls_pubkey_serde.rs (line 151)

The new visit_seq impl (lines 151-157) drains every element from SeqAccess into a Vec<u8> and only afterward defers length validation to from_compressed_g1_bytes (which checks COMPRESSED_G1_LEN == 48). The Vec::with_capacity(48) is just a hint — push grows unboundedly.

Threat model is narrow: bincode never reaches this visitor (per the doc-comment at lines 87-96, it goes through the derived Decode), and the in-scope self-describing callers (serde_json, platform_value) bound input at the parsed-document level. So this is defense-in-depth, not an exploit primitive — but a maliciously large public_key / threshold_public_key array in a ValidatorSet JSON/Value payload would currently force O(N) allocation and copy work before the 48-byte check rejects it.

The fixed 48-byte invariant lets the visitor fail fast: read exactly 48 elements then ensure the 49th is absent, mirroring what visit_str already does up-front for the hex form. This keeps memory bounded regardless of caller-side input limits.

    fn visit_seq<A: serde::de::SeqAccess<'de>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
        let mut bytes = [0u8; COMPRESSED_G1_LEN];
        for (index, slot) in bytes.iter_mut().enumerate() {
            *slot = seq.next_element::<u8>()?.ok_or_else(|| {
                <A::Error as serde::de::Error>::custom(format!(
                    "expected {COMPRESSED_G1_LEN} compressed-G1 bytes for public key, got {index}"
                ))
            })?;
        }
        if seq.next_element::<u8>()?.is_some() {
            return Err(<A::Error as serde::de::Error>::custom(format!(
                "expected {COMPRESSED_G1_LEN} compressed-G1 bytes for public key, got more"
            )));
        }
        from_compressed_g1_bytes(&bytes)
    }

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3573 319619c --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 5892c55. New blocking finding: docs/v12-upgrade-boundary-risks.md commits a 'Runbook (local only — do not commit)' section that publishes a named live HPMN validator IP, a second mainnet node IP, the ssh user, and the ssh key path. All four prior 319619c findings remain STILL VALID against the current worktree (no code touched in their files in the latest delta): TokenPricingSchedule u64 JSON precision, TokenDistributionInfo Repr-branch test coverage, ArrayItemType Boolean/Date round-trip coverage, and bls_pubkey_serde visit_seq unbounded buffering.

🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(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

blocking: Runbook section commits live-validator IP, ssh user, and ssh key path despite its own 'do not commit' warning

docs/v12-upgrade-boundary-risks.md (line 119-135)

The 'Runbook' section is self-labeled # Runbook (local only — contains infra details, do not commit) but is being committed anyway. It publishes:

• A named live HPMN validator host/IP — hp-masternode-8 = 68.67.122.8 (lines 121–123), tagged 'Live HPMN validator — read-only dump, do NOT stop the drive container.'
• A second host IP (54.185.219.212, line 124) confirmed to carry a synced mainnet drive volume.
• The ssh user (ubuntu) and the exact private-key path (~/.ssh/evo-app-deploy.rsa) on lines 126 and 132.
• A complete tar-and-scp recipe to extract drive state from a running validator.

In a public repository this is operational targeting information: it confirms which IP is a HPMN, narrows credential-attack/SSH-brute-force scope, and tells an attacker exactly where to look on disk to exfiltrate drive state. Once merged, the IP/user pairing is permanent in git history — deleting the file in a follow-up PR will not undo it. The doc itself acknowledges this section should not ship.

Drop the entire 'Runbook' section (or replace concrete identifiers with placeholders like <hpmn-host>, <ssh-user>, <ssh-key>). Also scrub the testnet hostname hp-masternode-8 from the Results preamble (line 95). The rest of the doc (Risk #1 / Risk #2 analysis) is fine to keep.

suggestion: TokenPricingSchedule canonical JSON emits raw u64 Credits/TokenAmount — JS clients silently round above 2^53−1

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

Carried forward from 319619c, re-verified at current head: this file is unchanged in the latest delta and the issue still applies.

TokenPricingSchedule derives plain Serialize, Deserialize (line 28) over SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) (lines 34, 44). Both Credits and TokenAmount are u64 aliases. The canonical JsonConvertible/ValueConvertible impls (lines 47–51) delegate straight through serde, emitting u64s as raw JSON numbers. Wire-shape tests confirm raw-number emission with no JS-safe wrapping.

When this JSON crosses the WASM bindgen boundary into JavaScript, JSON.parse / serde-wasm-bindgen materializes the numbers as IEEE-754 doubles, silently rounding any value above Number.MAX_SAFE_INTEGER (≈9.0×10^15). Since 1 Dash = 1e11 credits, prices above ~90k Dash worth of credits already exceed JS-safe range. This is exactly the hazard the PR's #[json_safe_fields] convention was introduced to prevent (see PR description: '25+ V0/V1 struct leaves carry the attribute'), and TokenPricingSchedule is reachable from TokenPerpetualDistribution and other distribution-config types that flow into JS via wasm-dpp2. The hand-written value getter returns BigInt safely, but the canonical toJSON() path still rounds.

Apply #[json_safe_fields(...)] (or the equivalent safe-integer/safe-integer-map adapter used elsewhere) to the Credits/TokenAmount payloads, or explicitly document that the JSON form is internal-only and JS clients must consume the platform_value form.

suggestion: TokenDistributionInfo round-trip tests only cover PreProgrammed; Perpetual and TokenDistributionTypeWithResolvedRecipient Repr branches untested

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322-389)

Carried forward from 319619c, re-verified at current head — file unchanged in the latest delta.

json_convertible_tests_token_distribution_info defines a single fixture() at lines 336–338 that builds TokenDistributionInfo::PreProgrammed(...), and the two tests (json_round_trip_with_full_wire_shape and value_round_trip_with_full_wire_shape) only exercise that fixture. The PR-added Repr adapters cover additional variants — notably TokenDistributionInfo::Perpetual and both TokenDistributionTypeWithResolvedRecipient::{PreProgrammed, Perpetual} branches — but those have no direct JSON/Value wire-shape pin tests.

Given the PR's stated 'wire-shape per type' convention (~85 tests on this convention per the PR description), the Perpetual variant and the resolved-recipient branches should get the same json!{} / hand-built Value::Map(...) assertion-style pins so that a future change to the internal tag shape, field renaming, or #[serde(rename_all)] configuration is caught locally. These conversion paths sit on contract/token-distribution JSON and Value boundaries; if the untested branches drift, JS-facing consumers can misinterpret reward recipients or distribution moments at runtime.

suggestion: ArrayItemType Boolean and Date variants lack canonical JSON/Value round-trip tests

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 706-842)

Carried forward from 319619c, re-verified at current head — file unchanged in the latest delta.

ArrayItemTypeRepr defines all seven variants including Boolean and Date (lines 42–44) as internally-tagged unit variants, and the From/Into adapters wire them through. The json_convertible_tests module (lines 712–842) covers Integer, Number, String, ByteArray, and Identifier for both JSON and Value paths, but has no Boolean or Date round-trip test. Both are unit variants and the camelCase rename matters ({"type":"boolean"} / {"type":"date"}).

Array item type metadata is part of document schema interpretation at untrusted contract/document boundaries; if these branches regress, clients and validators can disagree about boolean/date array field shapes during JSON/Value conversion. Adding two JSON and two Value tests for the remaining variants completes the convention used elsewhere in the file and matches the wire-shape pin pattern the PR establishes.

nitpick: BLS pubkey visit_seq buffers entire sequence into a Vec before enforcing 48-byte length

packages/rs-dpp/src/core_types/bls_pubkey_serde.rs (line 151-157)

Carried forward from 319619c, re-verified at current head — file unchanged in the latest delta.

BlsPublicKeyVisitor::visit_seq (lines 151–157) drains every u8 element from SeqAccess into a Vec<u8> (while let Some(b) = seq.next_element::<u8>()? { bytes.push(b); }) and only afterward defers the length check to from_compressed_g1_bytes, which enforces COMPRESSED_G1_LEN == 48. Vec::with_capacity(48) is only a sizing hint; push reallocates without bound.

Threat model: any code path that deserializes a BlsPublicKey<Bls12381G2Impl> from an attacker-influenced serde source emitting a u8 sequence (e.g., platform_value::Value::Array of Value::U8s for ValidatorSet::threshold_public_key or Validator::public_key, or values routed through wasm-dpp2 fromJSON/fromObject) can force allocation proportional to the attacker's input length before the 48-byte check rejects it. In practice the reachable sources here (serde_json, platform_value) are memory-bounded upstream, so this is not a standalone amplification vector — but it removes a cheap, type-local invariant ('this visitor never allocates more than 48 bytes') that future callers might rely on. Kept at nitpick: defense in depth, not exploitable in isolation.

Hardening: fill a fixed [u8; COMPRESSED_G1_LEN], fail fast on the 49th element, and fail fast if any of the 48 slots is missing.

    fn visit_seq<A: serde::de::SeqAccess<'de>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
        let mut bytes = [0u8; COMPRESSED_G1_LEN];
        for (i, slot) in bytes.iter_mut().enumerate() {
            *slot = seq
                .next_element::<u8>()?
                .ok_or_else(|| serde::de::Error::invalid_length(i, &self))?;
        }
        if seq.next_element::<u8>()?.is_some() {
            return Err(serde::de::Error::invalid_length(
                COMPRESSED_G1_LEN + 1,
                &self,
            ));
        }
        from_compressed_g1_bytes(&bytes)
    }

🤖 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 `docs/v12-upgrade-boundary-risks.md`:119-135: Runbook section commits live-validator IP, ssh user, and ssh key path despite its own 'do not commit' warning
  The 'Runbook' section is self-labeled `# Runbook (local only — contains infra details, do not commit)` but is being committed anyway. It publishes:

- A named **live HPMN validator** host/IP — `hp-masternode-8 = 68.67.122.8` (lines 121–123), tagged 'Live HPMN validator — read-only dump, do NOT stop the drive container.'
- A second host IP (`54.185.219.212`, line 124) confirmed to carry a synced mainnet drive volume.
- The ssh user (`ubuntu`) and the exact private-key path (`~/.ssh/evo-app-deploy.rsa`) on lines 126 and 132.
- A complete tar-and-scp recipe to extract drive state from a running validator.

In a public repository this is operational targeting information: it confirms which IP is a HPMN, narrows credential-attack/SSH-brute-force scope, and tells an attacker exactly where to look on disk to exfiltrate drive state. Once merged, the IP/user pairing is permanent in git history — deleting the file in a follow-up PR will not undo it. The doc itself acknowledges this section should not ship.

Drop the entire 'Runbook' section (or replace concrete identifiers with placeholders like `<hpmn-host>`, `<ssh-user>`, `<ssh-key>`). Also scrub the testnet hostname `hp-masternode-8` from the Results preamble (line 95). The rest of the doc (Risk #1 / Risk #2 analysis) is fine to keep.
- [SUGGESTION] In `packages/rs-dpp/src/tokens/token_pricing_schedule.rs`:28-51: TokenPricingSchedule canonical JSON emits raw u64 Credits/TokenAmount — JS clients silently round above 2^53−1
  Carried forward from 319619c4, re-verified at current head: this file is unchanged in the latest delta and the issue still applies.

`TokenPricingSchedule` derives plain `Serialize, Deserialize` (line 28) over `SinglePrice(Credits)` and `SetPrices(BTreeMap<TokenAmount, Credits>)` (lines 34, 44). Both `Credits` and `TokenAmount` are `u64` aliases. The canonical `JsonConvertible`/`ValueConvertible` impls (lines 47–51) delegate straight through serde, emitting `u64`s as raw JSON numbers. Wire-shape tests confirm raw-number emission with no JS-safe wrapping.

When this JSON crosses the WASM bindgen boundary into JavaScript, `JSON.parse` / `serde-wasm-bindgen` materializes the numbers as IEEE-754 doubles, silently rounding any value above `Number.MAX_SAFE_INTEGER` (≈9.0×10^15). Since 1 Dash = 1e11 credits, prices above ~90k Dash worth of credits already exceed JS-safe range. This is exactly the hazard the PR's `#[json_safe_fields]` convention was introduced to prevent (see PR description: '25+ V0/V1 struct leaves carry the attribute'), and `TokenPricingSchedule` is reachable from `TokenPerpetualDistribution` and other distribution-config types that flow into JS via wasm-dpp2. The hand-written `value` getter returns BigInt safely, but the canonical `toJSON()` path still rounds.

Apply `#[json_safe_fields(...)]` (or the equivalent safe-integer/safe-integer-map adapter used elsewhere) to the `Credits`/`TokenAmount` payloads, or explicitly document that the JSON form is internal-only and JS clients must consume the platform_value form.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs`:322-389: TokenDistributionInfo round-trip tests only cover PreProgrammed; Perpetual and TokenDistributionTypeWithResolvedRecipient Repr branches untested
  Carried forward from 319619c4, re-verified at current head — file unchanged in the latest delta.

`json_convertible_tests_token_distribution_info` defines a single `fixture()` at lines 336–338 that builds `TokenDistributionInfo::PreProgrammed(...)`, and the two tests (`json_round_trip_with_full_wire_shape` and `value_round_trip_with_full_wire_shape`) only exercise that fixture. The PR-added Repr adapters cover additional variants — notably `TokenDistributionInfo::Perpetual` and both `TokenDistributionTypeWithResolvedRecipient::{PreProgrammed, Perpetual}` branches — but those have no direct JSON/Value wire-shape pin tests.

Given the PR's stated 'wire-shape per type' convention (~85 tests on this convention per the PR description), the Perpetual variant and the resolved-recipient branches should get the same `json!{}` / hand-built `Value::Map(...)` assertion-style pins so that a future change to the internal tag shape, field renaming, or `#[serde(rename_all)]` configuration is caught locally. These conversion paths sit on contract/token-distribution JSON and Value boundaries; if the untested branches drift, JS-facing consumers can misinterpret reward recipients or distribution moments at runtime.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/document_type/property/array.rs`:706-842: ArrayItemType Boolean and Date variants lack canonical JSON/Value round-trip tests
  Carried forward from 319619c4, re-verified at current head — file unchanged in the latest delta.

`ArrayItemTypeRepr` defines all seven variants including `Boolean` and `Date` (lines 42–44) as internally-tagged unit variants, and the `From`/`Into` adapters wire them through. The `json_convertible_tests` module (lines 712–842) covers `Integer`, `Number`, `String`, `ByteArray`, and `Identifier` for both JSON and Value paths, but has no `Boolean` or `Date` round-trip test. Both are unit variants and the camelCase rename matters (`{"type":"boolean"}` / `{"type":"date"}`).

Array item type metadata is part of document schema interpretation at untrusted contract/document boundaries; if these branches regress, clients and validators can disagree about boolean/date array field shapes during JSON/Value conversion. Adding two JSON and two Value tests for the remaining variants completes the convention used elsewhere in the file and matches the wire-shape pin pattern the PR establishes.
- [NITPICK] In `packages/rs-dpp/src/core_types/bls_pubkey_serde.rs`:151-157: BLS pubkey visit_seq buffers entire sequence into a Vec before enforcing 48-byte length
  Carried forward from 319619c4, re-verified at current head — file unchanged in the latest delta.

`BlsPublicKeyVisitor::visit_seq` (lines 151–157) drains every `u8` element from `SeqAccess` into a `Vec<u8>` (`while let Some(b) = seq.next_element::<u8>()? { bytes.push(b); }`) and only afterward defers the length check to `from_compressed_g1_bytes`, which enforces `COMPRESSED_G1_LEN == 48`. `Vec::with_capacity(48)` is only a sizing hint; `push` reallocates without bound.

Threat model: any code path that deserializes a `BlsPublicKey<Bls12381G2Impl>` from an attacker-influenced serde source emitting a `u8` sequence (e.g., `platform_value::Value::Array` of `Value::U8`s for `ValidatorSet::threshold_public_key` or `Validator::public_key`, or values routed through wasm-dpp2 `fromJSON`/`fromObject`) can force allocation proportional to the attacker's input length before the 48-byte check rejects it. In practice the reachable sources here (`serde_json`, `platform_value`) are memory-bounded upstream, so this is not a standalone amplification vector — but it removes a cheap, type-local invariant ('this visitor never allocates more than 48 bytes') that future callers might rely on. Kept at nitpick: defense in depth, not exploitable in isolation.

Hardening: fill a fixed `[u8; COMPRESSED_G1_LEN]`, fail fast on the 49th element, and fail fast if any of the 48 slots is missing.

[thepastaclaw]

Code Review

Incremental review at ee80f99. The latest delta is a pure relocation of the BLS pubkey serde wrapper from core_types/ to serialization/dashcore/ with import-path updates and four new round-trip tests; no new defects were introduced by the move. All five prior 5892c55 findings are STILL VALID against the current head: F1 (committed live HPMN runbook), F2 (TokenPricingSchedule raw u64 JSON), F3/F4 (variant test-coverage gaps), and F5 (BLS visit_seq unbounded buffering). One additional defense-in-depth observation: the generic fixed-byte serde helper at serialization/serde_bytes.rs:79-87 exhibits the same unbounded-Vec pattern as F5 and is reused across canonical JSON/Value boundaries in this PR.

🔴 1 blocking | 🟡 3 suggestion(s) | 💬 2 nitpick(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

blocking: Runbook self-labeled "do not commit" still publishes live validator host, SSH user, and key path

docs/v12-upgrade-boundary-risks.md (line 119-135)

This section's own heading is ## Runbook (local only — contains infra details, do not commit), yet it was committed in 5892c55 and still ships at ee80f99. The committed body names a live HPMN validator host/IP (hp-masternode-8 = 68.67.122.8), a second node IP (54.185.219.212), the ansible/ssh user (ubuntu), the ssh private-key filename (~/.ssh/evo-app-deploy.rsa), and step-by-step operator instructions for extracting the drive docker volume. Even though the key file itself is not in the repo, publishing the (host, user, key-path) tuple plus a working extraction procedure on a public repo is exactly the footprinting detail the section says should not be committed, and it's unrelated to the PR's stated JSON/Value unification goal. Redact the section (or move it to private operational storage) before this doc lands.

suggestion: TokenPricingSchedule canonical JSON emits raw u64 Credits/TokenAmount — JS clients silently round above 2^53−1

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

TokenPricingSchedule derives plain Serialize/Deserialize and the canonical impls are bare (impl JsonConvertible for TokenPricingSchedule {}, impl ValueConvertible for TokenPricingSchedule {}). Both SinglePrice(Credits) and the keys/values of SetPrices(BTreeMap<TokenAmount, Credits>) are u64, so a price ≥ 2^53 round-trips through any JSON-based JS consumer (JSON.parse, fetch, wasm-dpp2 bridges via serde_json::Value) as an IEEE-754 double, silently losing precision. The PR's json_safe_u64 wrapper (applied to e.g. TokenDistributionInfoRepr::timestamp) is the convention being rolled out for exactly this class of bug but has not been applied here. The round-trip tests at lines 100-158 pin only small values (1234, 50, 80) so the precision loss never surfaces. Apply the json_safe_u64 wrapper to a serde-driving Repr (and the map equivalent for SetPrices), or document explicitly why credit prices are exempt from the JS-safe-integer convention applied to the rest of the cluster.

suggestion: TokenDistributionInfo round-trip tests still only cover PreProgrammed; Perpetual + TokenDistributionTypeWithResolvedRecipient have no canonical tests

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 328-389)

mod json_convertible_tests_token_distribution_info defines a single fixture() returning TokenDistributionInfo::PreProgrammed(...); both json_round_trip_with_full_wire_shape and value_round_trip_with_full_wire_shape reuse only that fixture. The Perpetual(RewardDistributionMoment, TokenDistributionResolvedRecipient) variant — which routes through a different Repr shape and the nested internally-tagged RewardDistributionMoment — has no direct round-trip test, and the sibling TokenDistributionTypeWithResolvedRecipient enum has no json_convertible_tests/value_convertible_tests module at all. Given this PR's stated pattern of pinning the full wire shape per variant, the Perpetual branches are a coverage gap worth closing — a silent variant-name flip in the Repr would be invisible today.

suggestion: ArrayItemType Boolean and Date variants still lack canonical JSON/Value round-trip tests

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 706-842)

mod json_convertible_tests covers Integer, Number, String, ByteArray, and Identifier for both JSON and Value paths, but the enum also has Boolean and Date internally-tagged unit variants wired through ArrayItemTypeRepr. These two leaves are exactly what the internally-tagged-Repr sweep this PR rolls out is supposed to lock in — a silent rename in the Repr would be invisible without a test. Add a {"type":"boolean"} and {"type":"date"} round-trip pair (JSON + Value) to bring this module in line with the surrounding convention.

nitpick: BLS pubkey visit_seq buffers the entire input sequence into a Vec before length-checking

packages/rs-dpp/src/serialization/dashcore/bls_pubkey.rs (line 151-157)

After the rename from core_types/bls_pubkey_serde.rs to serialization/dashcore/bls_pubkey.rs, the visitor body is byte-identical: let mut bytes = Vec::with_capacity(COMPRESSED_G1_LEN); while let Some(b) = seq.next_element::<u8>()? { bytes.push(b); } from_compressed_g1_bytes(&bytes). with_capacity(48) is only a hint; an oversized array from a self-describing deserializer (serde_json, platform_value::from_value, or any serde Content-buffered path) will be drained and reallocated well past 48 bytes before length validation in from_compressed_g1_bytes kicks in. The wrapper is now exposed on every wire form through deserialize_any and used by Validator::public_key / ValidatorSetV0::threshold_public_key. The risk is muted today because populators are trusted Tenderdash/gRPC server responses, but the boundary should bound memory at the source. Fill a fixed [u8; COMPRESSED_G1_LEN], reject the 49th element, and only then call from_compressed_g1_bytes.

    fn visit_seq<A: serde::de::SeqAccess<'de>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
        let mut bytes = [0u8; COMPRESSED_G1_LEN];
        for (i, slot) in bytes.iter_mut().enumerate() {
            *slot = seq.next_element::<u8>()?.ok_or_else(|| {
                <A::Error as serde::de::Error>::custom(format!(
                    "expected {COMPRESSED_G1_LEN} compressed-G1 bytes for public key, got {i}"
                ))
            })?;
        }
        if seq.next_element::<serde::de::IgnoredAny>()?.is_some() {
            return Err(<A::Error as serde::de::Error>::custom(format!(
                "expected exactly {COMPRESSED_G1_LEN} compressed-G1 bytes for public key, got more"
            )));
        }
        from_compressed_g1_bytes(&bytes)
    }

nitpick: Fixed-size byte helper drains oversized sequences into a Vec before rejecting them

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

Same defense-in-depth issue as the BLS visit_seq, in the generic helper used by json_safe_fields for fixed-size byte fields across the canonical JSON/Value conversion this PR rolls out. The sequence path is let mut buf = Vec::with_capacity(N); while let Some(b) = seq.next_element::<u8>()? { buf.push(b); } buf.try_into() — a JSON/Value array of arbitrary length is fully buffered before the length is checked via try_into. Bound the read so a > N-element sequence is rejected without buffering. Severity nitpick because both serde_json and platform_value enforce outer document size limits upstream, but this helper is the most-reused fixed-byte boundary in the PR, so fixing it once removes the pattern everywhere.

        fn visit_seq<A: SeqAccess<'de>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
            let mut buf = [0u8; N];
            for (i, slot) in buf.iter_mut().enumerate() {
                *slot = seq
                    .next_element::<u8>()?
                    .ok_or_else(|| de::Error::custom(format!("expected {} bytes, got {}", N, i)))?;
            }
            if seq.next_element::<de::IgnoredAny>()?.is_some() {
                return Err(de::Error::custom(format!("expected {} bytes, got more", N)));
            }
            Ok(buf)
        }

🤖 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 `docs/v12-upgrade-boundary-risks.md`:119-135: Runbook self-labeled "do not commit" still publishes live validator host, SSH user, and key path
  This section's own heading is `## Runbook (local only — contains infra details, do not commit)`, yet it was committed in 5892c55c and still ships at ee80f994. The committed body names a live HPMN validator host/IP (`hp-masternode-8 = 68.67.122.8`), a second node IP (`54.185.219.212`), the ansible/ssh user (`ubuntu`), the ssh private-key filename (`~/.ssh/evo-app-deploy.rsa`), and step-by-step operator instructions for extracting the drive docker volume. Even though the key file itself is not in the repo, publishing the (host, user, key-path) tuple plus a working extraction procedure on a public repo is exactly the footprinting detail the section says should not be committed, and it's unrelated to the PR's stated JSON/Value unification goal. Redact the section (or move it to private operational storage) before this doc lands.
- [SUGGESTION] In `packages/rs-dpp/src/tokens/token_pricing_schedule.rs`:28-51: TokenPricingSchedule canonical JSON emits raw u64 Credits/TokenAmount — JS clients silently round above 2^53−1
  `TokenPricingSchedule` derives plain `Serialize`/`Deserialize` and the canonical impls are bare (`impl JsonConvertible for TokenPricingSchedule {}`, `impl ValueConvertible for TokenPricingSchedule {}`). Both `SinglePrice(Credits)` and the keys/values of `SetPrices(BTreeMap<TokenAmount, Credits>)` are `u64`, so a price ≥ 2^53 round-trips through any JSON-based JS consumer (JSON.parse, fetch, wasm-dpp2 bridges via serde_json::Value) as an IEEE-754 double, silently losing precision. The PR's `json_safe_u64` wrapper (applied to e.g. `TokenDistributionInfoRepr::timestamp`) is the convention being rolled out for exactly this class of bug but has not been applied here. The round-trip tests at lines 100-158 pin only small values (1234, 50, 80) so the precision loss never surfaces. Apply the `json_safe_u64` wrapper to a serde-driving Repr (and the map equivalent for `SetPrices`), or document explicitly why credit prices are exempt from the JS-safe-integer convention applied to the rest of the cluster.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs`:328-389: TokenDistributionInfo round-trip tests still only cover PreProgrammed; Perpetual + TokenDistributionTypeWithResolvedRecipient have no canonical tests
  `mod json_convertible_tests_token_distribution_info` defines a single `fixture()` returning `TokenDistributionInfo::PreProgrammed(...)`; both `json_round_trip_with_full_wire_shape` and `value_round_trip_with_full_wire_shape` reuse only that fixture. The `Perpetual(RewardDistributionMoment, TokenDistributionResolvedRecipient)` variant — which routes through a different Repr shape and the nested internally-tagged `RewardDistributionMoment` — has no direct round-trip test, and the sibling `TokenDistributionTypeWithResolvedRecipient` enum has no `json_convertible_tests`/`value_convertible_tests` module at all. Given this PR's stated pattern of pinning the full wire shape per variant, the Perpetual branches are a coverage gap worth closing — a silent variant-name flip in the Repr would be invisible today.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/document_type/property/array.rs`:706-842: ArrayItemType Boolean and Date variants still lack canonical JSON/Value round-trip tests
  `mod json_convertible_tests` covers Integer, Number, String, ByteArray, and Identifier for both JSON and Value paths, but the enum also has `Boolean` and `Date` internally-tagged unit variants wired through `ArrayItemTypeRepr`. These two leaves are exactly what the internally-tagged-Repr sweep this PR rolls out is supposed to lock in — a silent rename in the Repr would be invisible without a test. Add a `{"type":"boolean"}` and `{"type":"date"}` round-trip pair (JSON + Value) to bring this module in line with the surrounding convention.
- [NITPICK] In `packages/rs-dpp/src/serialization/dashcore/bls_pubkey.rs`:151-157: BLS pubkey visit_seq buffers the entire input sequence into a Vec before length-checking
  After the rename from `core_types/bls_pubkey_serde.rs` to `serialization/dashcore/bls_pubkey.rs`, the visitor body is byte-identical: `let mut bytes = Vec::with_capacity(COMPRESSED_G1_LEN); while let Some(b) = seq.next_element::<u8>()? { bytes.push(b); } from_compressed_g1_bytes(&bytes)`. `with_capacity(48)` is only a hint; an oversized array from a self-describing deserializer (serde_json, platform_value::from_value, or any serde Content-buffered path) will be drained and reallocated well past 48 bytes before length validation in `from_compressed_g1_bytes` kicks in. The wrapper is now exposed on every wire form through `deserialize_any` and used by `Validator::public_key` / `ValidatorSetV0::threshold_public_key`. The risk is muted today because populators are trusted Tenderdash/gRPC server responses, but the boundary should bound memory at the source. Fill a fixed `[u8; COMPRESSED_G1_LEN]`, reject the 49th element, and only then call `from_compressed_g1_bytes`.
- [NITPICK] In `packages/rs-dpp/src/serialization/serde_bytes.rs`:79-87: Fixed-size byte helper drains oversized sequences into a Vec before rejecting them
  Same defense-in-depth issue as the BLS `visit_seq`, in the generic helper used by `json_safe_fields` for fixed-size byte fields across the canonical JSON/Value conversion this PR rolls out. The sequence path is `let mut buf = Vec::with_capacity(N); while let Some(b) = seq.next_element::<u8>()? { buf.push(b); } buf.try_into()` — a JSON/Value array of arbitrary length is fully buffered before the length is checked via `try_into`. Bound the read so a > N-element sequence is rejected without buffering. Severity nitpick because both serde_json and platform_value enforce outer document size limits upstream, but this helper is the most-reused fixed-byte boundary in the PR, so fixing it once removes the pattern everywhere.

[thepastaclaw]

Code Review

Verified all six prior ee80f99 findings against the current head: all STILL VALID. The latest commit (refactor(dpp)!: use $type for ALL internally-tagged enum discriminators) renamed serde tag attributes across rs-dpp but did not sync the corresponding typescript_custom_section declarations in wasm-dpp2 — six TS types now misdescribe the runtime JS shape, breaking the FFI contract for AssetLockProof, AddressWitness, FeeStrategyStep, ResourceVoteChoice, ContestedDocumentVotePollWinnerInfo, and VotePoll. Combined with the carried-forward blocking infra disclosure in docs/v12-upgrade-boundary-risks.md, this run yields two blocking findings. Nitpicks (visit_seq buffering) dropped due to comment budget after retaining higher-value suggestions.

🔴 2 blocking | 🟡 3 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

blocking: Runbook self-labeled 'do not commit' still publishes live HPMN validator IP, SSH user, and key path

docs/v12-upgrade-boundary-risks.md (line 119-135)

Carried forward from ee80f99 — STILL VALID at current head. The section heading on line 119 literally reads ## Runbook (local only — contains infra details, do not commit), yet the file still ships:

• live HPMN testnet validator: hp-masternode-8 = 68.67.122.8 (ansible_user ubuntu), marked 'Live HPMN validator — read-only dump'
• mainnet node IP 54.185.219.212
• SSH user ubuntu and key path ~/.ssh/evo-app-deploy.rsa

This is on a public PR in a public repo. The IP + SSH-user + key-filename triad narrows reconnaissance scope for an attacker (validator host, account name to spray, signal of where the operator's private key lives). The section's own heading marks the content as never-meant-to-commit. Either remove the runbook section entirely or replace the live identifiers with placeholders (<HPMN_HOST>, <SSH_USER>, <SSH_KEY_PATH>) before merging.

blocking: $type discriminator rename was not synced to wasm-dpp2 TypeScript declarations (6 affected types)

packages/wasm-dpp2/src/asset_lock_proof/proof.rs (line 18-38)

The latest commit 7f63b891 (refactor(dpp)!: use $type for ALL internally-tagged enum discriminators) renamed the serde discriminator across rs-dpp from type to $type. The corresponding typescript_custom_section declarations in wasm-dpp2 were not updated, so the runtime wire shape (emitted via toObject()/toJSON() through JsonConvertible/ValueConvertible) no longer matches the published TypeScript. JS consumers narrowing on obj.type === "..." will dead-end on undefined, and any JS-constructed object following the documented TS shape will fail from_object/from_json ingest because the new visitors only accept $type.

Affected files (all need type: → $type: in both interface members and surrounding docstrings):

1. packages/wasm-dpp2/src/asset_lock_proof/proof.rs — AssetLockProofObject / AssetLockProofJSON (lines 18–38). rs-dpp source: packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38 now serde(tag = "$type").
2. packages/wasm-dpp2/src/shielded/address_witness.rs — AddressWitnessP2pkhObject / AddressWitnessP2shObject / AddressWitnessP2pkhJSON / AddressWitnessP2shJSON (lines 12–56). rs-dpp source: packages/rs-dpp/src/address_funds/witness.rs:32.
3. packages/wasm-dpp2/src/platform_address/fee_strategy.rs — FeeStrategyStepObject / FeeStrategyStepJSON (lines 8–40), plus stale docstring on line 13 and stale comment on line 39. rs-dpp source: packages/rs-dpp/src/address_funds/fee_strategy/mod.rs:42,46 (manual Serialize/Deserialize now read/write $type).
4. packages/wasm-dpp2/src/voting/resource_vote_choice.rs — ResourceVoteChoiceObject / ResourceVoteChoiceJSON (lines 9–30). rs-dpp source: packages/rs-dpp/src/voting/vote_choices/resource_vote_choice/mod.rs:47,53,58 (manual Serialize emits $type).
5. packages/wasm-dpp2/src/voting/winner_info.rs — ContestedDocumentVotePollWinnerInfoObject / …JSON (lines 8–29). rs-dpp source: packages/rs-dpp/src/voting/vote_info_storage/contested_document_vote_poll_winner_info/mod.rs:31,36,42.
6. packages/wasm-dpp2/src/voting/vote_poll.rs — VotePollObject / VotePollJSON (lines 31–55), plus stale comment on line 34 (Internally tagged with "type"). rs-dpp source: packages/rs-dpp/src/voting/vote_polls/mod.rs:29 now serde(tag = "$type", rename_all = "camelCase").

All six wasm-dpp2 wrappers route toObject/toJSON through the rs-dpp serde via impl_wasm_conversions_inner! or #[serde(transparent)], so the runtime mismatch is real and round-trips will break. Existing wasm-dpp2 JS test fixtures (e.g. packages/wasm-dpp2/tests/unit/FeeStrategyStep.spec.ts:64-89) and wasm-sdk fixtures (e.g. packages/wasm-sdk/tests/unit/fixtures/data-contract-v1-with-docs-tokens-groups.ts:118-163 for token rule discriminators) also still use the legacy type shape and will need to be updated alongside the TS declarations.

suggestion: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round above 2^53−1

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

Carried forward from ee80f99 — STILL VALID. TokenPricingSchedule derives plain Serialize/Deserialize and the canonical conversion impls are bare (impl JsonConvertible/ValueConvertible with no wrapper). Both SinglePrice(Credits) and the keys/values of SetPrices(BTreeMap<TokenAmount, Credits>) are u64, and the wire-shape test on line 120 pins {"SetPrices": {"5": 50, "10": 80}} with bare numeric values.

This PR's stated convention is json_safe_fields on 25+ V0/V1 leaves to protect JS-safe large-integer shapes; TokenPricingSchedule is a domain leaf carrying token economic data, and both Credits and TokenAmount can plausibly exceed 2^53−1. Through TokenPricingScheduleWasm the canonical JSON path reaches JS via json_to_js_value, where values above 2^53−1 silently round, causing UIs over token prices to misdisplay or miscalculate. Wrap the u64 payloads in JS-safe encoding (string in JSON, U64 in non-HR), consistent with the rest of the json_safe_fields rollout.

suggestion: TokenDistributionInfo round-trip tests cover only PreProgrammed; Perpetual + TokenDistributionTypeWithResolvedRecipient unpinned

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322-389)

Carried forward from ee80f99 — STILL VALID. mod json_convertible_tests_token_distribution_info defines a single fixture() returning TokenDistributionInfo::PreProgrammed(...) (line 336–338), and both json_round_trip_with_full_wire_shape and value_round_trip_with_full_wire_shape reuse only that fixture. The Perpetual(RewardDistributionType::...) branch and the related TokenDistributionTypeWithResolvedRecipient branches have no direct canonical round-trip test.

This is especially relevant because the latest $type rename touched these enums' discriminator key, and the entire Perpetual family was reshaped by the $action → $type unification in this commit. Without direct wire-shape pins for those branches, a future rename or repr change for Perpetual / resolved-recipient shapes would not be caught by the convention this PR is establishing. Add round-trip tests building TokenDistributionInfo::Perpetual(...) and at least one TokenDistributionTypeWithResolvedRecipient variant.

suggestion: ArrayItemType Boolean and Date variants lack canonical JSON/Value round-trip tests

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 706-842)

Carried forward from ee80f99 — STILL VALID. mod json_convertible_tests (lines 712–842) exercises round-trip wire shape for Integer, Number, String, ByteArray, and Identifier on both JSON and Value paths. The unit variants ArrayItemType::Boolean and ArrayItemType::Date, both reachable through ArrayItemTypeRepr, have no direct canonical round-trip test.

The latest $type rename touched the camelCase tags on this exact enum (per the array.rs diff in commit 7f63b89), making coverage of the remaining two unit variants cheap and worth adding so the wire shape is locked for every branch.

🤖 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 `docs/v12-upgrade-boundary-risks.md`:119-135: Runbook self-labeled 'do not commit' still publishes live HPMN validator IP, SSH user, and key path
  Carried forward from ee80f994 — STILL VALID at current head. The section heading on line 119 literally reads `## Runbook (local only — contains infra details, do not commit)`, yet the file still ships:
- live HPMN testnet validator: `hp-masternode-8 = 68.67.122.8` (ansible_user `ubuntu`), marked 'Live HPMN validator — read-only dump'
- mainnet node IP `54.185.219.212`
- SSH user `ubuntu` and key path `~/.ssh/evo-app-deploy.rsa`

This is on a public PR in a public repo. The IP + SSH-user + key-filename triad narrows reconnaissance scope for an attacker (validator host, account name to spray, signal of where the operator's private key lives). The section's own heading marks the content as never-meant-to-commit. Either remove the runbook section entirely or replace the live identifiers with placeholders (`<HPMN_HOST>`, `<SSH_USER>`, `<SSH_KEY_PATH>`) before merging.
- [BLOCKING] In `packages/wasm-dpp2/src/asset_lock_proof/proof.rs`:18-38: $type discriminator rename was not synced to wasm-dpp2 TypeScript declarations (6 affected types)
  The latest commit `7f63b891` (`refactor(dpp)!: use $type for ALL internally-tagged enum discriminators`) renamed the serde discriminator across rs-dpp from `type` to `$type`. The corresponding `typescript_custom_section` declarations in wasm-dpp2 were not updated, so the runtime wire shape (emitted via `toObject()`/`toJSON()` through `JsonConvertible`/`ValueConvertible`) no longer matches the published TypeScript. JS consumers narrowing on `obj.type === "..."` will dead-end on `undefined`, and any JS-constructed object following the documented TS shape will fail `from_object`/`from_json` ingest because the new visitors only accept `$type`.

Affected files (all need `type:` → `$type:` in both interface members and surrounding docstrings):
1. `packages/wasm-dpp2/src/asset_lock_proof/proof.rs` — `AssetLockProofObject` / `AssetLockProofJSON` (lines 18–38). rs-dpp source: `packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38` now `serde(tag = "$type")`.
2. `packages/wasm-dpp2/src/shielded/address_witness.rs` — `AddressWitnessP2pkhObject` / `AddressWitnessP2shObject` / `AddressWitnessP2pkhJSON` / `AddressWitnessP2shJSON` (lines 12–56). rs-dpp source: `packages/rs-dpp/src/address_funds/witness.rs:32`.
3. `packages/wasm-dpp2/src/platform_address/fee_strategy.rs` — `FeeStrategyStepObject` / `FeeStrategyStepJSON` (lines 8–40), plus stale docstring on line 13 and stale comment on line 39. rs-dpp source: `packages/rs-dpp/src/address_funds/fee_strategy/mod.rs:42,46` (manual Serialize/Deserialize now read/write `$type`).
4. `packages/wasm-dpp2/src/voting/resource_vote_choice.rs` — `ResourceVoteChoiceObject` / `ResourceVoteChoiceJSON` (lines 9–30). rs-dpp source: `packages/rs-dpp/src/voting/vote_choices/resource_vote_choice/mod.rs:47,53,58` (manual Serialize emits `$type`).
5. `packages/wasm-dpp2/src/voting/winner_info.rs` — `ContestedDocumentVotePollWinnerInfoObject` / `…JSON` (lines 8–29). rs-dpp source: `packages/rs-dpp/src/voting/vote_info_storage/contested_document_vote_poll_winner_info/mod.rs:31,36,42`.
6. `packages/wasm-dpp2/src/voting/vote_poll.rs` — `VotePollObject` / `VotePollJSON` (lines 31–55), plus stale comment on line 34 (`Internally tagged with "type"`). rs-dpp source: `packages/rs-dpp/src/voting/vote_polls/mod.rs:29` now `serde(tag = "$type", rename_all = "camelCase")`.

All six wasm-dpp2 wrappers route `toObject`/`toJSON` through the rs-dpp serde via `impl_wasm_conversions_inner!` or `#[serde(transparent)]`, so the runtime mismatch is real and round-trips will break. Existing wasm-dpp2 JS test fixtures (e.g. `packages/wasm-dpp2/tests/unit/FeeStrategyStep.spec.ts:64-89`) and wasm-sdk fixtures (e.g. `packages/wasm-sdk/tests/unit/fixtures/data-contract-v1-with-docs-tokens-groups.ts:118-163` for token rule discriminators) also still use the legacy `type` shape and will need to be updated alongside the TS declarations.
- [SUGGESTION] In `packages/rs-dpp/src/tokens/token_pricing_schedule.rs`:28-51: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round above 2^53−1
  Carried forward from ee80f994 — STILL VALID. `TokenPricingSchedule` derives plain `Serialize`/`Deserialize` and the canonical conversion impls are bare (`impl JsonConvertible`/`ValueConvertible` with no wrapper). Both `SinglePrice(Credits)` and the keys/values of `SetPrices(BTreeMap<TokenAmount, Credits>)` are `u64`, and the wire-shape test on line 120 pins `{"SetPrices": {"5": 50, "10": 80}}` with bare numeric values.

This PR's stated convention is `json_safe_fields` on 25+ V0/V1 leaves to protect JS-safe large-integer shapes; TokenPricingSchedule is a domain leaf carrying token economic data, and both `Credits` and `TokenAmount` can plausibly exceed 2^53−1. Through `TokenPricingScheduleWasm` the canonical JSON path reaches JS via `json_to_js_value`, where values above 2^53−1 silently round, causing UIs over token prices to misdisplay or miscalculate. Wrap the u64 payloads in JS-safe encoding (string in JSON, U64 in non-HR), consistent with the rest of the `json_safe_fields` rollout.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs`:322-389: TokenDistributionInfo round-trip tests cover only PreProgrammed; Perpetual + TokenDistributionTypeWithResolvedRecipient unpinned
  Carried forward from ee80f994 — STILL VALID. `mod json_convertible_tests_token_distribution_info` defines a single `fixture()` returning `TokenDistributionInfo::PreProgrammed(...)` (line 336–338), and both `json_round_trip_with_full_wire_shape` and `value_round_trip_with_full_wire_shape` reuse only that fixture. The `Perpetual(RewardDistributionType::...)` branch and the related `TokenDistributionTypeWithResolvedRecipient` branches have no direct canonical round-trip test.

This is especially relevant because the latest `$type` rename touched these enums' discriminator key, and the entire Perpetual family was reshaped by the `$action → $type` unification in this commit. Without direct wire-shape pins for those branches, a future rename or repr change for `Perpetual` / resolved-recipient shapes would not be caught by the convention this PR is establishing. Add round-trip tests building `TokenDistributionInfo::Perpetual(...)` and at least one `TokenDistributionTypeWithResolvedRecipient` variant.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/document_type/property/array.rs`:706-842: ArrayItemType Boolean and Date variants lack canonical JSON/Value round-trip tests
  Carried forward from ee80f994 — STILL VALID. `mod json_convertible_tests` (lines 712–842) exercises round-trip wire shape for `Integer`, `Number`, `String`, `ByteArray`, and `Identifier` on both JSON and Value paths. The unit variants `ArrayItemType::Boolean` and `ArrayItemType::Date`, both reachable through `ArrayItemTypeRepr`, have no direct canonical round-trip test.

The latest `$type` rename touched the camelCase tags on this exact enum (per the array.rs diff in commit 7f63b891), making coverage of the remaining two unit variants cheap and worth adding so the wire shape is locked for every branch.

[thepastaclaw]

Correction to the automated review at 7f63b891

The review I just posted correctly verified all six prior ee80f994 findings as still valid, but the body incorrectly demoted two still-valid carried-forward nitpicks due to a comment-budget note. That was too aggressive for this incremental/cumulative review: prior still-valid findings must stay carried forward.

Adding the two omitted active findings here, with current-head evidence:

nitpick: BLS pubkey visit_seq buffers oversized sequences before enforcing the 48-byte length

packages/rs-dpp/src/serialization/dashcore/bls_pubkey.rs (lines 151-157)

[Carried forward — STILL VALID] Verified at current head: visit_seq still allocates Vec::with_capacity(COMPRESSED_G1_LEN), drains every u8 from SeqAccess with while let Some(b) = seq.next_element::<u8>()? { bytes.push(b); }, and only then calls from_compressed_g1_bytes(&bytes) for length validation. with_capacity(48) is only a hint, so an oversized JSON/Value array is fully buffered/reallocated before the fixed-size check rejects it. Fill a fixed [u8; COMPRESSED_G1_LEN], reject early EOF while reading, and reject the 49th element immediately before calling from_compressed_g1_bytes.

nitpick: Fixed-size byte helper drains oversized sequences into a Vec before rejecting them

packages/rs-dpp/src/serialization/serde_bytes.rs (lines 79-87)

[Carried forward — STILL VALID] Verified at current head: the generic fixed-size byte helper still uses let mut buf = Vec::with_capacity(N); while let Some(b) = seq.next_element::<u8>()? { buf.push(b); } and only checks the final length via buf.try_into(). This helper is reused by the canonical JSON/Value fixed-byte conversion path, so an oversized array is fully buffered before rejection. Fill a fixed [u8; N], error on missing elements, then check for and reject any extra (N+1)th element without buffering the rest.

Net active findings for 7f63b891: 7 total (2 blocking, 3 suggestions, 2 nitpicks). The original review action remains REQUEST_CHANGES.

[thepastaclaw]

Code Review

Verified at head 4d75999. Latest delta from 7f63b89 is a comment-only sweep touching six rs-dpp files (no executable changes). All five prior findings re-validated as STILL VALID and are carried forward; codex-ffi-engineer surfaced one additional TS-declaration drift (TokenEvent) that I confirmed and folded into the existing TS/serde discriminator finding. Two blocking issues remain (committed do-not-commit runbook leaking live HPMN infra; wasm-dpp2 TypeScript declarations advertise type while runtime emits $type across 7 user-facing types).

Reviewed commit: 4d75999

🔴 2 blocking | 🟡 3 suggestion(s)

Verified findings

blocking: Runbook self-labeled 'do not commit' still ships live HPMN validator IP, SSH user, and key path

docs/v12-upgrade-boundary-risks.md (line 119)

Verified at 4d75999 — the section heading literally reads ## Runbook (local only — contains infra details, do not commit), yet the body publishes operator-sensitive metadata: a live HPMN testnet validator (hp-masternode-8 = 68.67.122.8, ansible_user ubuntu), a mainnet test host (54.185.219.212), and the SSH key path ~/.ssh/evo-app-deploy.rsa. Pinning a named SSH key + user + role to a live consensus participant produces ready-made reconnaissance and spear-phishing material that the author's own heading flags as out-of-bounds for commit. Scrub the section before merge, rotate the named SSH key, and audit git log --all -S 68.67.122.8 and -S evo-app-deploy.rsa to confirm no prior commit also leaks these details.

blocking: wasm-dpp2 TypeScript declarations still advertise `type` while runtime uses `$type` (7 affected types)

packages/wasm-dpp2/src/asset_lock_proof/proof.rs (line 18)

Verified at 4d75999. Commit 7f63b89 renamed every internally-tagged serde discriminator in rs-dpp from type to $type (confirmed: packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49 uses #[serde(tag = "$type")]; packages/rs-dpp/src/tokens/token_event.rs:194-271,329,352 writes/reads $type). The wasm-dpp2 wrappers route toObject()/toJSON()/fromObject()/fromJSON() through these rs-dpp conversions via impl_wasm_conversions_inner!, so runtime payloads now carry $type. However, seven typescript_custom_section declarations still advertise the old type field to TS consumers and the surrounding JSDoc still describes the rs-dpp shape as #[serde(tag = "type")]:

• packages/wasm-dpp2/src/asset_lock_proof/proof.rs:23-37 (AssetLockProofObject/JSON)
• packages/wasm-dpp2/src/shielded/address_witness.rs:17-47 (AddressWitness Object/JSON)
• packages/wasm-dpp2/src/platform_address/fee_strategy.rs:18-30 (FeeStrategyStep Object/JSON)
• packages/wasm-dpp2/src/voting/resource_vote_choice.rs:19-29 (ResourceVoteChoice)
• packages/wasm-dpp2/src/voting/winner_info.rs:18-28 (ContestedDocumentVotePollWinnerInfo)
• packages/wasm-dpp2/src/voting/vote_poll.rs:39,50 (VotePoll types)
• packages/wasm-dpp2/src/group/token_event.rs:33-45 (TokenEventObject/JSON — newly identified, comments at lines 25-27 also still document { type, ... })
  (packages/wasm-dpp2/src/voting/vote.rs:22,32 was migrated correctly, confirming $type is the intended new shape.) Consequence: TS consumers that destructure obj.type get undefined; objects constructed per the published .d.ts will fail deserialization at the wasm boundary because rs-dpp's MapAccess only honours $type. This is a hard contract break for vote routing, asset-lock proof variant selection, fee-strategy resolution, and token-event dispatch. packages/wasm-dpp2/CONVENTIONS.md:90-102 also still documents the old type shape. Sync all seven TS sections (and CONVENTIONS.md) to $type in lockstep with the rs-dpp rename.

#[wasm_bindgen(typescript_custom_section)]
const TS_TYPES: &str = r#"
/**
 * AssetLockProof serialized as a plain object.
 *
 * Internally-tagged discriminated union — `$type` discriminates the variant and
 * the variant's fields sit alongside it. Mirrors the rs-dpp serde shape (which
 * uses `#[serde(tag = "$type")]` on the enum) and the convention used by
 * `AddressWitness` / `AddressFundsFeeStrategyStep`.
 */
export type AssetLockProofObject =
    | ({ $type: "instant" } & InstantAssetLockProofObject)
    | ({ $type: "chain" } & ChainAssetLockProofObject);

/**
 * AssetLockProof serialized as JSON.
 */
export type AssetLockProofJSON =
    | ({ $type: "instant" } & InstantAssetLockProofJSON)
    | ({ $type: "chain" } & ChainAssetLockProofJSON);
"#;

suggestion: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round above 2^53−1

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

Verified at 4d75999 — file unchanged. TokenPricingSchedule derives plain Serialize/Deserialize (line 28) and the canonical conversion impls are bare wrappers (impl JsonConvertible/impl ValueConvertible, lines 48 & 51) with no Repr struct or json_safe_u64 annotation. Both SinglePrice(Credits) and the keys/values of SetPrices(BTreeMap<TokenAmount, Credits>) are u64 aliases, so canonical JSON emits raw numeric literals. The PR explicitly establishes a json_safe_u64 / #[json_safe_fields] convention to avoid silent JS rounding above Number.MAX_SAFE_INTEGER (2^53−1) and applies it at ~25 V0/V1 leaves. Pricing values denominated in credits (1 DASH = 1e11 credits) cross 2^53 around 90,000 DASH, well within plausible enterprise/treasury-tier pricing. A JS wallet or dApp reading the canonical JSON above this threshold authorises or submits a different price than the consensus value. Apply the same Repr-with-json_safe_u64 pattern used in token_distribution_key.rs's TokenDistributionInfoRepr.

suggestion: TokenDistributionInfo round-trip tests cover only PreProgrammed; Perpetual + TokenDistributionTypeWithResolvedRecipient unpinned

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322)

Verified at 4d75999 (latest delta only touched a comment in this file). mod json_convertible_tests_token_distribution_info defines a single PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32])) fixture and both JSON and Value round-trip tests exercise only that branch (lines 336-388). The Perpetual(RewardDistributionMoment, TokenDistributionResolvedRecipient) variant — which composes another internally-tagged enum plus a recipient enum, exactly the nesting most likely to break the $type sweep — has no wire-shape assertion. TokenDistributionTypeWithResolvedRecipient (declared at line 47) also has no dedicated round-trip module. Given the PR's wire-shape-literal regression convention, add a Perpetual fixture and a per-variant fixture for TokenDistributionTypeWithResolvedRecipient with json!{} / platform_value!{} literal assertions matching the patterns already established.

suggestion: ArrayItemType Boolean and Date variants lack canonical JSON/Value round-trip tests

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 706)

Verified at 4d75999 (delta only retouched a comment). ArrayItemType declares seven variants but json_convertible_tests pins JSON and Value wire shapes for only Integer, Number, String, ByteArray, and Identifier (lines 715-841). Boolean and Date (both unit variants) have neither JSON nor Value assertions on either path. Given the PR's stated wire-shape-literal coverage goal, a future serde rename or variant-name drift on these two would not be caught by CI. Add parallel json_round_trip_boolean_variant / value_round_trip_boolean_variant / json_round_trip_date_variant / value_round_trip_date_variant asserting {"$type":"boolean"} and {"$type":"date"} respectively.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3573 4d75999 --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

Reviewed PR #3573 at fd85cd3. Latest delta (4d75999→fd85cd320f) is a test-fixture refresh swapping type/kind → $type/$kind in 19 wasm-dpp2 spec files. No new defects from the delta itself, but the test changes lock in a wire shape that the public typescript_custom_section declarations have not been updated to match — making prior finding #2 strictly more conspicuous. All five prior findings from 4d75999 remain STILL VALID against the current head: #1 (runbook infra leak), #2 (stale TS discriminator declarations across 9 wasm-dpp2 files), #3 (TokenPricingSchedule raw u64 JSON), #4 (TokenDistributionInfo Perpetual variant unpinned), and #5 (ArrayItemType Boolean/Date unpinned). None are fixed, outdated, or intentionally deferred.

🔴 2 blocking | 🟡 3 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

blocking: 'Do not commit' runbook ships live HPMN validator IP, SSH user, and key path

docs/v12-upgrade-boundary-risks.md (line 119-135)

STILL VALID at fd85cd3. The section heading at line 119 literally reads ## Runbook (local only — contains infra details, do not commit), but the body (lines 121–135) publishes operator-sensitive metadata that the author self-classified as private:

• Live HPMN testnet validator: hp-masternode-8 = 68.67.122.8, ansible_user ubuntu, explicitly labeled Live HPMN validator — read-only dump, do NOT stop the drive container.
• Mainnet-synced test host: 54.185.219.212.
• SSH key path: ~/.ssh/evo-app-deploy.rsa — the filename names a deploy key tied to evo-app and is repeated twice (lines 126, 132).

The file was introduced in this PR's commit 5892c55 and the runbook section remains unchanged through fd85cd3. Publishing it via this PR contradicts the author's own warning header and narrows reconnaissance against a live HPMN (operator-side SSH username + key filename + node→IP mapping).

Resolution: delete the ## Runbook section, or replace concrete hostnames/IPs/usernames/key paths with placeholders (e.g. <testnet-hpmn>, <mainnet-test-node>, <ssh-key>). The Risk #1 / #2 analysis above stands on its own without these operator details. Given the key filename has been pushed to a branch, the referenced SSH key should also be rotated.

blocking: wasm-dpp2 TypeScript declarations still advertise legacy `type`/`kind`/old-shape discriminators; runtime requires `$type`/`$kind`

packages/wasm-dpp2/src/asset_lock_proof/proof.rs (line 18-38)

STILL VALID at fd85cd3, and reinforced by the latest delta which updated every paired .spec.ts to assert $type / $kind against the runtime.

Commit 7f63b89 migrated every internally-tagged serde enum in rs-dpp to #[serde(tag = "$type")] (and tag = "$kind" for GroupActionEvent). Verified at current head, e.g.:

• packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49 — #[serde(tag = "$type")].
• packages/rs-dpp/src/group/action_event.rs:25 — serde(tag = "$kind").

The wasm-bindgen(typescript_custom_section) blocks that publish the TS contract to SDK consumers were NOT updated in lock-step. The mismatch covers nine files:

• packages/wasm-dpp2/src/asset_lock_proof/proof.rs:28-37 — AssetLockProofObject/JSON key on type: "instant"|"chain"; doc at lines 23-26 still cites #[serde(tag = "type")].
• packages/wasm-dpp2/src/voting/vote_poll.rs:38-55 — VotePollObject/JSON declare type: "contestedDocumentResourceVotePoll"; doc still says Internally tagged with "type".
• packages/wasm-dpp2/src/voting/resource_vote_choice.rs:18-29 — ResourceVoteChoiceObject/JSON declare type: "towardsIdentity"|"abstain"|"lock".
• packages/wasm-dpp2/src/voting/winner_info.rs:17-28 — ContestedDocumentVotePollWinnerInfoObject/JSON declare type: "noWinner"|"locked"|"wonByIdentity".
• packages/wasm-dpp2/src/platform_address/fee_strategy.rs:17-30 — FeeStrategyStepObject/JSON declare type: "deductFromInput"|"reduceOutput".
• packages/wasm-dpp2/src/shielded/address_witness.rs:16-55 — AddressWitnessP2pkh{Object,JSON} and AddressWitnessP2sh{Object,JSON} declare type: "p2pkh"|"p2sh".
• packages/wasm-dpp2/src/group/token_event.rs:11-44 — open shape { type: string; [field: string]: unknown } keyed on type:; the per-variant doc examples (lines 16-28) all use type:.
• packages/wasm-dpp2/src/group/action_event.rs:7-22 — GroupActionEventObject/JSON declare { kind: "tokenEvent" } & TokenEventObject (runtime is $kind).
• packages/wasm-dpp2/src/data_contract/contract_bounds.rs:11-29 — declares the old { identifier, documentTypeName?, contractBoundsType } shape; the canonical Rust shape is { "$type": "singleContract"|"documentType", id, documentTypeName? }.

Impact at the TS↔WASM boundary:

1. Consumers building objects per the published TS types pass type: (or kind:/contractBoundsType:) which the runtime serde deserializer does not recognize as the discriminator — fromObject/fromJSON will reject the payload or fall through to a default variant.
2. Consumers reading toObject()/toJSON() results will type-check accesses against result.type/result.kind, while the runtime actually emits result.$type/result.$kind — every such access is undefined with no TS error.

This is a public-API discriminator mismatch on the SDK trust boundary in a PR whose stated purpose is to unify and pin JSON/Value conversion contracts. The PR's own JS tests now assert the new shape; the TS declarations consumed by SDK clients must be updated to match.

Fix: sweep every affected typescript_custom_section block — rename type: → $type:, kind: → $kind:, update doc comments that still reference tag = "type", and rewrite the ContractBounds TS types to match the actual { "$type", id, documentTypeName? } runtime shape.

suggestion: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round above 2^53−1

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

STILL VALID at fd85cd3. TokenPricingSchedule (lines 29-45) still derives plain Serialize/Deserialize, and the canonical impls at lines 48 / 51 are bare blanket impls with no Repr struct, no #[json_safe_fields], and no json_safe_u64 annotation.

SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) both serialize their u64 values as raw JSON numbers. JavaScript's Number is IEEE-754 double, so any credit value above 2^53−1 (≈9.007e15) is silently rounded by JSON.parse. With 1 Dash = 100 billion credits, 2^53−1 credits ≈ 90,071 Dash — well within plausible token-pricing ranges, and the u64 native max allows values ~10× the entire Dash supply.

Consumer impact at the wasm boundary: a wasm-sdk / js-evo-sdk consumer reading a TokenPricingSchedule via toJSON() for UI display or pre-validation can show a value that differs from the on-chain (bincode-derived) value by more than a credit. Signing happens against bytes, so consensus is unaffected — but the user-facing number can diverge silently, and a malicious token issuer could craft a value that rounds to a smaller number in a JS dashboard to inflate apparent affordability.

This PR explicitly established the json_safe_u64 / #[json_safe_fields] convention (per the PR description, '25+ V0/V1 struct leaves carry the attribute. JS-safe large-integer protection at every serialization site.') and TokenPricingSchedule was missed. Apply the same Repr/json_safe_u64 treatment used elsewhere for Credits fields.

suggestion: TokenDistributionInfo round-trip tests cover only PreProgrammed; Perpetual and resolved-recipient variants unpinned

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322-389)

STILL VALID at fd85cd3. mod json_convertible_tests_token_distribution_info (lines 328-389) defines a single fixture TokenDistributionInfo::PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32])) and asserts both JSON and Value wire shapes for that one variant.

TokenDistributionInfo::Perpetual { moment, recipient } has no direct canonical wire-shape pin. TokenDistributionTypeWithResolvedRecipient has canonical trait impls (touched by this PR) but no direct branch-shape tests in either direction.

The PR's stated convention is literal json!{} / platform_value!{} per-variant assertions (~85 sites elsewhere). Perpetual and the resolved-recipient type should be pinned with the same rigor: a silent variant flip, tag-key drift, or inner-shape change would not be caught.

suggestion: ArrayItemType canonical round-trip tests miss Boolean and Date variants

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 706-842)

STILL VALID at fd85cd3. ArrayItemType declares seven variants, but mod json_convertible_tests (lines 712-842) pins JSON + Value wire shapes for only Integer (716-726, 782-790), Number (729-737, 793-801), String (740-754, 804-816), ByteArray (757-768, 819-830), and Identifier (771-779, 833-841). The Boolean and Date unit variants have no canonical wire-shape pin in either direction.

Given the PR adopted literal-shape assertions as a deliberate architectural convention, a silent discriminator drift or visitor-fallthrough for Boolean / Date would not be caught by the current suite. Two short tests mirroring json_round_trip_integer_variant and value_round_trip_integer_variant close the gap.

🤖 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 `docs/v12-upgrade-boundary-risks.md`:119-135: 'Do not commit' runbook ships live HPMN validator IP, SSH user, and key path
  STILL VALID at fd85cd320f. The section heading at line 119 literally reads `## Runbook (local only — contains infra details, do not commit)`, but the body (lines 121–135) publishes operator-sensitive metadata that the author self-classified as private:
- Live HPMN testnet validator: `hp-masternode-8` = `68.67.122.8`, ansible_user `ubuntu`, explicitly labeled `Live HPMN validator — read-only dump, do NOT stop the drive container.`
- Mainnet-synced test host: `54.185.219.212`.
- SSH key path: `~/.ssh/evo-app-deploy.rsa` — the filename names a deploy key tied to evo-app and is repeated twice (lines 126, 132).

The file was introduced in this PR's commit 5892c55c26 and the runbook section remains unchanged through fd85cd320f. Publishing it via this PR contradicts the author's own warning header and narrows reconnaissance against a live HPMN (operator-side SSH username + key filename + node→IP mapping).

Resolution: delete the `## Runbook` section, or replace concrete hostnames/IPs/usernames/key paths with placeholders (e.g. `<testnet-hpmn>`, `<mainnet-test-node>`, `<ssh-key>`). The Risk #1 / #2 analysis above stands on its own without these operator details. Given the key filename has been pushed to a branch, the referenced SSH key should also be rotated.
- [BLOCKING] In `packages/wasm-dpp2/src/asset_lock_proof/proof.rs`:18-38: wasm-dpp2 TypeScript declarations still advertise legacy `type`/`kind`/old-shape discriminators; runtime requires `$type`/`$kind`
  STILL VALID at fd85cd320f, and reinforced by the latest delta which updated every paired `.spec.ts` to assert `$type` / `$kind` against the runtime.

Commit 7f63b891 migrated every internally-tagged serde enum in rs-dpp to `#[serde(tag = "$type")]` (and `tag = "$kind"` for `GroupActionEvent`). Verified at current head, e.g.:
- `packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49` — `#[serde(tag = "$type")]`.
- `packages/rs-dpp/src/group/action_event.rs:25` — `serde(tag = "$kind")`.

The `wasm-bindgen(typescript_custom_section)` blocks that publish the TS contract to SDK consumers were NOT updated in lock-step. The mismatch covers nine files:
- `packages/wasm-dpp2/src/asset_lock_proof/proof.rs:28-37` — `AssetLockProofObject`/`JSON` key on `type: "instant"|"chain"`; doc at lines 23-26 still cites `#[serde(tag = "type")]`.
- `packages/wasm-dpp2/src/voting/vote_poll.rs:38-55` — `VotePollObject`/`JSON` declare `type: "contestedDocumentResourceVotePoll"`; doc still says `Internally tagged with "type"`.
- `packages/wasm-dpp2/src/voting/resource_vote_choice.rs:18-29` — `ResourceVoteChoiceObject`/`JSON` declare `type: "towardsIdentity"|"abstain"|"lock"`.
- `packages/wasm-dpp2/src/voting/winner_info.rs:17-28` — `ContestedDocumentVotePollWinnerInfoObject`/`JSON` declare `type: "noWinner"|"locked"|"wonByIdentity"`.
- `packages/wasm-dpp2/src/platform_address/fee_strategy.rs:17-30` — `FeeStrategyStepObject`/`JSON` declare `type: "deductFromInput"|"reduceOutput"`.
- `packages/wasm-dpp2/src/shielded/address_witness.rs:16-55` — `AddressWitnessP2pkh{Object,JSON}` and `AddressWitnessP2sh{Object,JSON}` declare `type: "p2pkh"|"p2sh"`.
- `packages/wasm-dpp2/src/group/token_event.rs:11-44` — open shape `{ type: string; [field: string]: unknown }` keyed on `type:`; the per-variant doc examples (lines 16-28) all use `type:`.
- `packages/wasm-dpp2/src/group/action_event.rs:7-22` — `GroupActionEventObject`/`JSON` declare `{ kind: "tokenEvent" } & TokenEventObject` (runtime is `$kind`).
- `packages/wasm-dpp2/src/data_contract/contract_bounds.rs:11-29` — declares the old `{ identifier, documentTypeName?, contractBoundsType }` shape; the canonical Rust shape is `{ "$type": "singleContract"|"documentType", id, documentTypeName? }`.

Impact at the TS↔WASM boundary:
1. Consumers building objects per the published TS types pass `type:` (or `kind:`/`contractBoundsType:`) which the runtime serde deserializer does not recognize as the discriminator — `fromObject`/`fromJSON` will reject the payload or fall through to a default variant.
2. Consumers reading `toObject()`/`toJSON()` results will type-check accesses against `result.type`/`result.kind`, while the runtime actually emits `result.$type`/`result.$kind` — every such access is `undefined` with no TS error.

This is a public-API discriminator mismatch on the SDK trust boundary in a PR whose stated purpose is to unify and pin JSON/Value conversion contracts. The PR's own JS tests now assert the new shape; the TS declarations consumed by SDK clients must be updated to match.

Fix: sweep every affected `typescript_custom_section` block — rename `type:` → `$type:`, `kind:` → `$kind:`, update doc comments that still reference `tag = "type"`, and rewrite the `ContractBounds` TS types to match the actual `{ "$type", id, documentTypeName? }` runtime shape.
- [SUGGESTION] In `packages/rs-dpp/src/tokens/token_pricing_schedule.rs`:28-51: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round above 2^53−1
  STILL VALID at fd85cd320f. `TokenPricingSchedule` (lines 29-45) still derives plain `Serialize`/`Deserialize`, and the canonical impls at lines 48 / 51 are bare blanket impls with no Repr struct, no `#[json_safe_fields]`, and no `json_safe_u64` annotation.

`SinglePrice(Credits)` and `SetPrices(BTreeMap<TokenAmount, Credits>)` both serialize their `u64` values as raw JSON numbers. JavaScript's `Number` is IEEE-754 double, so any credit value above 2^53−1 (≈9.007e15) is silently rounded by `JSON.parse`. With 1 Dash = 100 billion credits, 2^53−1 credits ≈ 90,071 Dash — well within plausible token-pricing ranges, and the u64 native max allows values ~10× the entire Dash supply.

Consumer impact at the wasm boundary: a wasm-sdk / js-evo-sdk consumer reading a `TokenPricingSchedule` via `toJSON()` for UI display or pre-validation can show a value that differs from the on-chain (bincode-derived) value by more than a credit. Signing happens against bytes, so consensus is unaffected — but the user-facing number can diverge silently, and a malicious token issuer could craft a value that rounds to a smaller number in a JS dashboard to inflate apparent affordability.

This PR explicitly established the `json_safe_u64` / `#[json_safe_fields]` convention (per the PR description, '25+ V0/V1 struct leaves carry the attribute. JS-safe large-integer protection at every serialization site.') and `TokenPricingSchedule` was missed. Apply the same `Repr`/`json_safe_u64` treatment used elsewhere for `Credits` fields.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs`:322-389: TokenDistributionInfo round-trip tests cover only PreProgrammed; Perpetual and resolved-recipient variants unpinned
  STILL VALID at fd85cd320f. `mod json_convertible_tests_token_distribution_info` (lines 328-389) defines a single fixture `TokenDistributionInfo::PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32]))` and asserts both JSON and Value wire shapes for that one variant.

`TokenDistributionInfo::Perpetual { moment, recipient }` has no direct canonical wire-shape pin. `TokenDistributionTypeWithResolvedRecipient` has canonical trait impls (touched by this PR) but no direct branch-shape tests in either direction.

The PR's stated convention is literal `json!{}` / `platform_value!{}` per-variant assertions (~85 sites elsewhere). Perpetual and the resolved-recipient type should be pinned with the same rigor: a silent variant flip, tag-key drift, or inner-shape change would not be caught.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/document_type/property/array.rs`:706-842: ArrayItemType canonical round-trip tests miss Boolean and Date variants
  STILL VALID at fd85cd320f. `ArrayItemType` declares seven variants, but `mod json_convertible_tests` (lines 712-842) pins JSON + Value wire shapes for only Integer (716-726, 782-790), Number (729-737, 793-801), String (740-754, 804-816), ByteArray (757-768, 819-830), and Identifier (771-779, 833-841). The `Boolean` and `Date` unit variants have no canonical wire-shape pin in either direction.

Given the PR adopted literal-shape assertions as a deliberate architectural convention, a silent discriminator drift or visitor-fallthrough for Boolean / Date would not be caught by the current suite. Two short tests mirroring `json_round_trip_integer_variant` and `value_round_trip_integer_variant` close the gap.

[thepastaclaw]

Code Review

Cumulative verification at 2686052: latest delta only updates two wasm-sdk test files for the $type/$kind discriminator and introduces no new defects. All five prior fd85cd3 findings are STILL VALID. Two blocking issues persist: (1) the committed runbook in docs/v12-upgrade-boundary-risks.md self-labelled 'do not commit' still publishes a live HPMN testnet validator IP, SSH user, and operator key path; (2) wasm-dpp2 TypeScript declarations across seven files still advertise legacy type/kind discriminators while the rs-dpp runtime (changed in this PR) emits $type/$kind. Three suggestion-grade carry-forwards remain (TokenPricingSchedule u64 JS-safety gap, TokenDistributionInfo wire-pin coverage gap, ArrayItemType wire-pin coverage gap).

🔴 2 blocking | 🟡 3 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

blocking: Committed runbook self-labelled 'do not commit' leaks live HPMN validator IP, SSH user, and operator key path

docs/v12-upgrade-boundary-risks.md (line 119-135)

STILL VALID at 2686052. The section heading at line 119 reads ## Runbook (local only — contains infra details, do not commit), yet lines 122–132 publish exactly that:

• Live HPMN testnet validator host: hp-masternode-8 = 68.67.122.8, ansible_user ubuntu, explicitly annotated as a 'Live HPMN validator'.
• A second probe host: 54.185.219.212.
• The deploy key path used to reach both: ~/.ssh/evo-app-deploy.rsa, plus the matching ssh -i ... ubuntu@<host> invocation.
• Operator-internal procedure (docker volume, tar layout, scp target) confirming a single shared deploy key spans both the live validator and the disposable mainnet probe node.

Why this is blocking:

1. The author's own do not commit label is the only control here, and it failed — this is a process gap and a disclosure event, not a documentation stylistic issue.
2. Once pushed to a public Git host, the data is effectively permanent (mirrors, clones, GitHub search/UI cache); reverting later does not erase it. Mitigation requires history rewrite before merge plus rotation/audit of the named key.
3. Testnet HPMN validators participate in Tenderdash consensus on testnet, so naming a specific live validator hands an attacker a confirmed target for SSH brute-force, key-stuffing, scanning, or resource-exhaustion. The shared deploy-key path expands blast radius if that key is ever leaked from any operator workstation.

Fix before merge: remove lines 119–135 from the file in this PR, force-rewrite the branch so 2686052 is not the merged SHA, rotate evo-app-deploy.rsa, and audit auth.log on both hosts for the window the commit was reachable. If a runbook for this work is needed, keep it in a private operator note and reference it from the PR by name only.

blocking: wasm-dpp2 TypeScript declarations still advertise legacy `type`/`kind` while rs-dpp runtime (changed in this PR) emits `$type`/`$kind`

packages/wasm-dpp2/src/asset_lock_proof/proof.rs (line 18-38)

STILL VALID at 2686052. The Phase-D refactor in commit 7f63b89 flipped every internally-tagged rs-dpp enum to #[serde(tag = "$type")] / "$kind", but the matching typescript_custom_section blocks in wasm-dpp2 were not migrated in lockstep. Verified divergences at current head:

• packages/wasm-dpp2/src/asset_lock_proof/proof.rs:23-37 — declares { type: "instant"|"chain" }; rs-dpp AssetLockProof is #[serde(tag = "$type")] at packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49. The doc comment in the TS block still references tag = "type", which is also stale.
• packages/wasm-dpp2/src/voting/resource_vote_choice.rs:18-29 — declares { type: "towardsIdentity"|"abstain"|"lock" }; the wasm-sdk tests updated in this very delta (packages/wasm-sdk/tests/unit/conversion-vote.spec.ts) now assert $type on the same shape.
• packages/wasm-dpp2/src/voting/vote_poll.rs:38-55 — declares type: "contestedDocumentResourceVotePoll"; rs-dpp VotePoll is #[serde(tag = "$type")] at packages/rs-dpp/src/voting/vote_polls/mod.rs:29.
• packages/wasm-dpp2/src/voting/winner_info.rs:17-28 — declares { type: "noWinner"|"locked"|"wonByIdentity" }; runtime is the rs-dpp Vote/winner-info $type path.
• packages/wasm-dpp2/src/shielded/address_witness.rs:16-50 — declares type: "p2pkh"|"p2sh"; rs-dpp AddressWitness is #[serde(tag = "$type")] at packages/rs-dpp/src/address_funds/witness.rs:32.
• packages/wasm-dpp2/src/platform_address/fee_strategy.rs:17-30 — declares type: "deductFromInput"|"reduceOutput".
• packages/wasm-dpp2/src/group/token_event.rs:33-45 — declares type: string and the inline variant doc lists type: keys; rs-dpp TokenEvent custom Serialize emits "$type".
• packages/wasm-dpp2/src/group/action_event.rs:7-22 — declares { kind: "tokenEvent" }; rs-dpp GroupActionEvent is #[serde(tag = "$kind")] at packages/rs-dpp/src/group/action_event.rs:25. The doc comment in this block still rationalises kind as the chosen key, which is stale.

Consequences at the Rust↔JS boundary:

1. TS consumers writing object literals to satisfy these declarations will type-check { type: "..." } but AssetLockProof.fromObject(...) (and the other impl_wasm_conversions_inner! paths) delegates straight to rs-dpp's serde, which expects $type/$kind — deserialization will fail.
2. .toJSON() / .toObject() return objects with $type/$kind, so TypeScript discriminated-union narrowing on the declared type/kind field is dead — if (obj.type === "instant") compiles, runs, and never matches.
3. The Rust round-trip tests pin $type and pass, so this divergence is invisible to cargo test — it only surfaces at the JS boundary, which is exactly what this PR's unification was meant to lock down.

Fix in lockstep with the runtime + the new test fixtures: rename the discriminator key to $type (or $kind for GroupActionEvent) in every TS section, refresh the surrounding doc comments that still describe the legacy type/kind tag, and add at least one JS spec per type asserting the runtime shape so this regression class is caught next time.

suggestion: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round above 2^53-1

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

STILL VALID at 2686052. TokenPricingSchedule still relies on plain #[cfg_attr(feature = "serde-conversion", derive(Serialize, Deserialize))] (line 28) and the canonical JsonConvertible / ValueConvertible impls (lines 47-51) are bare blanket impls with no Repr struct, no #[json_safe_fields], and no json_safe_u64 annotation. The SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) variants therefore emit raw u64 JSON values (BTreeMap keys serialize as decimal strings); the round-trip tests at lines 100-123 explicitly pin { "SinglePrice": 1234 } and { "SetPrices": { "5": 50 } }, confirming the wire shape.

Credits tracks Dash credits at 1e11 per Dash, so realistic values can exceed JS Number.MAX_SAFE_INTEGER (2^53-1) — any JS consumer that runs the canonical-JSON path through JSON.parse silently rounds. This PR's stated convention applies the JS-safe-u64 protection to ~25 other V0/V1 leaves; TokenPricingSchedule was missed. Add #[json_safe_fields] (or route the canonical path through a repr that uses json_safe_u64) and pin the wire shape with a fixture above 2^53.

suggestion: TokenDistributionInfo round-trip tests only pin PreProgrammed; Perpetual and TokenDistributionTypeWithResolvedRecipient unpinned

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322-389)

STILL VALID at 2686052. mod json_convertible_tests_token_distribution_info (lines 328-389) defines a single fixture TokenDistributionInfo::PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32])) (line 337) and pins JSON + Value wire shapes only for that variant. TokenDistributionInfo::Perpetual { moment, recipient } and the sibling TokenDistributionTypeWithResolvedRecipient (which exposes its own canonical JsonConvertible / ValueConvertible impls in this file) have no canonical wire-shape pin. Given this PR's stated goal is comprehensive round-trip pinning of every $type-tagged enum, leaving these arms unpinned weakens the safety net — a future contributor could rename the field, change camelCase mapping, or flip a default and tests would still pass. Add value_round_trip_perpetual, json_round_trip_perpetual, and TokenDistributionTypeWithResolvedRecipient cases pinning both wire shapes.

suggestion: ArrayItemType canonical round-trip tests miss Boolean and Date variants

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 706-842)

STILL VALID at 2686052. ArrayItemType declares seven variants (Integer, Number, String, ByteArray, Identifier, Boolean, Date), but mod json_convertible_tests (lines 712-842) only pins JSON + Value wire shapes for five of them. The Boolean and Date unit variants — both of which travel through the $type tag — have no canonical wire-shape pin in either direction. This leaves a hole in the per-variant pin sweep that the rest of the PR is consistent about. Add json_round_trip_boolean_variant, json_round_trip_date_variant, and the symmetric value_round_trip_* cases asserting {"$type": "boolean"} / {"$type": "date"}.

🤖 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 `docs/v12-upgrade-boundary-risks.md`:119-135: Committed runbook self-labelled 'do not commit' leaks live HPMN validator IP, SSH user, and operator key path
  STILL VALID at 26860523f9. The section heading at line 119 reads `## Runbook (local only — contains infra details, do not commit)`, yet lines 122–132 publish exactly that:
- Live HPMN testnet validator host: `hp-masternode-8` = `68.67.122.8`, ansible_user `ubuntu`, explicitly annotated as a 'Live HPMN validator'.
- A second probe host: `54.185.219.212`.
- The deploy key path used to reach both: `~/.ssh/evo-app-deploy.rsa`, plus the matching `ssh -i ... ubuntu@<host>` invocation.
- Operator-internal procedure (docker volume, tar layout, scp target) confirming a single shared deploy key spans both the live validator and the disposable mainnet probe node.

Why this is blocking:
1. The author's own `do not commit` label is the only control here, and it failed — this is a process gap and a disclosure event, not a documentation stylistic issue.
2. Once pushed to a public Git host, the data is effectively permanent (mirrors, clones, GitHub search/UI cache); reverting later does not erase it. Mitigation requires history rewrite before merge plus rotation/audit of the named key.
3. Testnet HPMN validators participate in Tenderdash consensus on testnet, so naming a specific live validator hands an attacker a confirmed target for SSH brute-force, key-stuffing, scanning, or resource-exhaustion. The shared deploy-key path expands blast radius if that key is ever leaked from any operator workstation.

Fix before merge: remove lines 119–135 from the file in this PR, force-rewrite the branch so 26860523f9 is not the merged SHA, rotate `evo-app-deploy.rsa`, and audit auth.log on both hosts for the window the commit was reachable. If a runbook for this work is needed, keep it in a private operator note and reference it from the PR by name only.
- [BLOCKING] In `packages/wasm-dpp2/src/asset_lock_proof/proof.rs`:18-38: wasm-dpp2 TypeScript declarations still advertise legacy `type`/`kind` while rs-dpp runtime (changed in this PR) emits `$type`/`$kind`
  STILL VALID at 26860523f9. The Phase-D refactor in commit 7f63b89198 flipped every internally-tagged rs-dpp enum to `#[serde(tag = "$type")]` / `"$kind"`, but the matching `typescript_custom_section` blocks in wasm-dpp2 were not migrated in lockstep. Verified divergences at current head:

- packages/wasm-dpp2/src/asset_lock_proof/proof.rs:23-37 — declares `{ type: "instant"|"chain" }`; rs-dpp `AssetLockProof` is `#[serde(tag = "$type")]` at packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49. The doc comment in the TS block still references `tag = "type"`, which is also stale.
- packages/wasm-dpp2/src/voting/resource_vote_choice.rs:18-29 — declares `{ type: "towardsIdentity"|"abstain"|"lock" }`; the wasm-sdk tests updated in this very delta (packages/wasm-sdk/tests/unit/conversion-vote.spec.ts) now assert `$type` on the same shape.
- packages/wasm-dpp2/src/voting/vote_poll.rs:38-55 — declares `type: "contestedDocumentResourceVotePoll"`; rs-dpp `VotePoll` is `#[serde(tag = "$type")]` at packages/rs-dpp/src/voting/vote_polls/mod.rs:29.
- packages/wasm-dpp2/src/voting/winner_info.rs:17-28 — declares `{ type: "noWinner"|"locked"|"wonByIdentity" }`; runtime is the rs-dpp Vote/winner-info `$type` path.
- packages/wasm-dpp2/src/shielded/address_witness.rs:16-50 — declares `type: "p2pkh"|"p2sh"`; rs-dpp `AddressWitness` is `#[serde(tag = "$type")]` at packages/rs-dpp/src/address_funds/witness.rs:32.
- packages/wasm-dpp2/src/platform_address/fee_strategy.rs:17-30 — declares `type: "deductFromInput"|"reduceOutput"`.
- packages/wasm-dpp2/src/group/token_event.rs:33-45 — declares `type: string` and the inline variant doc lists `type:` keys; rs-dpp `TokenEvent` custom Serialize emits `"$type"`.
- packages/wasm-dpp2/src/group/action_event.rs:7-22 — declares `{ kind: "tokenEvent" }`; rs-dpp `GroupActionEvent` is `#[serde(tag = "$kind")]` at packages/rs-dpp/src/group/action_event.rs:25. The doc comment in this block still rationalises `kind` as the chosen key, which is stale.

Consequences at the Rust↔JS boundary:
1. TS consumers writing object literals to satisfy these declarations will type-check `{ type: "..." }` but `AssetLockProof.fromObject(...)` (and the other `impl_wasm_conversions_inner!` paths) delegates straight to rs-dpp's serde, which expects `$type`/`$kind` — deserialization will fail.
2. `.toJSON()` / `.toObject()` return objects with `$type`/`$kind`, so TypeScript discriminated-union narrowing on the declared `type`/`kind` field is dead — `if (obj.type === "instant")` compiles, runs, and never matches.
3. The Rust round-trip tests pin `$type` and pass, so this divergence is invisible to `cargo test` — it only surfaces at the JS boundary, which is exactly what this PR's unification was meant to lock down.

Fix in lockstep with the runtime + the new test fixtures: rename the discriminator key to `$type` (or `$kind` for GroupActionEvent) in every TS section, refresh the surrounding doc comments that still describe the legacy `type`/`kind` tag, and add at least one JS spec per type asserting the runtime shape so this regression class is caught next time.
- [SUGGESTION] In `packages/rs-dpp/src/tokens/token_pricing_schedule.rs`:28-51: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round above 2^53-1
  STILL VALID at 26860523f9. `TokenPricingSchedule` still relies on plain `#[cfg_attr(feature = "serde-conversion", derive(Serialize, Deserialize))]` (line 28) and the canonical `JsonConvertible` / `ValueConvertible` impls (lines 47-51) are bare blanket impls with no Repr struct, no `#[json_safe_fields]`, and no `json_safe_u64` annotation. The `SinglePrice(Credits)` and `SetPrices(BTreeMap<TokenAmount, Credits>)` variants therefore emit raw `u64` JSON values (BTreeMap keys serialize as decimal strings); the round-trip tests at lines 100-123 explicitly pin `{ "SinglePrice": 1234 }` and `{ "SetPrices": { "5": 50 } }`, confirming the wire shape.

`Credits` tracks Dash credits at 1e11 per Dash, so realistic values can exceed JS `Number.MAX_SAFE_INTEGER` (2^53-1) — any JS consumer that runs the canonical-JSON path through `JSON.parse` silently rounds. This PR's stated convention applies the JS-safe-u64 protection to ~25 other V0/V1 leaves; `TokenPricingSchedule` was missed. Add `#[json_safe_fields]` (or route the canonical path through a repr that uses `json_safe_u64`) and pin the wire shape with a fixture above 2^53.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs`:322-389: TokenDistributionInfo round-trip tests only pin PreProgrammed; Perpetual and TokenDistributionTypeWithResolvedRecipient unpinned
  STILL VALID at 26860523f9. `mod json_convertible_tests_token_distribution_info` (lines 328-389) defines a single fixture `TokenDistributionInfo::PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32]))` (line 337) and pins JSON + Value wire shapes only for that variant. `TokenDistributionInfo::Perpetual { moment, recipient }` and the sibling `TokenDistributionTypeWithResolvedRecipient` (which exposes its own canonical `JsonConvertible` / `ValueConvertible` impls in this file) have no canonical wire-shape pin. Given this PR's stated goal is comprehensive round-trip pinning of every `$type`-tagged enum, leaving these arms unpinned weakens the safety net — a future contributor could rename the field, change camelCase mapping, or flip a default and tests would still pass. Add `value_round_trip_perpetual`, `json_round_trip_perpetual`, and `TokenDistributionTypeWithResolvedRecipient` cases pinning both wire shapes.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/document_type/property/array.rs`:706-842: ArrayItemType canonical round-trip tests miss Boolean and Date variants
  STILL VALID at 26860523f9. `ArrayItemType` declares seven variants (Integer, Number, String, ByteArray, Identifier, Boolean, Date), but `mod json_convertible_tests` (lines 712-842) only pins JSON + Value wire shapes for five of them. The Boolean and Date unit variants — both of which travel through the `$type` tag — have no canonical wire-shape pin in either direction. This leaves a hole in the per-variant pin sweep that the rest of the PR is consistent about. Add `json_round_trip_boolean_variant`, `json_round_trip_date_variant`, and the symmetric `value_round_trip_*` cases asserting `{"$type": "boolean"}` / `{"$type": "date"}`.

[thepastaclaw]

Code Review

Verified at head a592d79. Prior finding reconciliation: #1 (runbook leak in docs/v12-upgrade-boundary-risks.md) STILL VALID — file at lines 119-135 still contains the self-labelled 'do not commit' section with live HPMN validator IP, ansible user, and operator SSH key path; BLOCKING. #2 (wasm-dpp2 TS type vs runtime $type/$kind) STILL VALID — proof.rs:18-38 still declares type discriminator and the doc-comment still says #[serde(tag = "type")], while rs-dpp asset_lock_proof/mod.rs:38,49 uses $type; BLOCKING. #3 (TokenPricingSchedule raw u64) STILL VALID — no json_safe_fields, tests pin raw numeric shape; SUGGESTION. #4 (TokenDistributionInfo unpinned variants) STILL VALID — only PreProgrammed pinned; SUGGESTION. #5 (ArrayItemType Boolean/Date unpinned) STILL VALID — only 5 of 7 variants pinned; SUGGESTION. New delta finding: the rewritten lenient constructor at wasm-dpp/.../data_contract_update_transition/mod.rs:52-79 defaults $formatVersion/userFeeIncrease/signaturePublicKeyId/signature but does not default or alias $identity-contract-nonce, which is a required serde field; two still-checked-in JS specs construct the transition without it.

🔴 2 blocking | 🟡 4 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

blocking: Committed runbook self-labelled 'do not commit' discloses live HPMN validator IP, SSH user, and operator SSH key path

docs/v12-upgrade-boundary-risks.md (line 119-135)

STILL VALID at head a592d79. The section heading at line 119 reads ## Runbook (local only — contains infra details, do not commit), but the body publishes operator-sensitive infrastructure metadata the author themselves marked as private:

• hp-masternode-8 = 68.67.122.8 is identified as a live HPMN testnet validator with ansible_user ubuntu and do NOT stop the drive container annotation.
• 54.185.219.212 is identified as a disposable mainnet-synced node.
• SSH access template is published verbatim: ssh -i ~/.ssh/evo-app-deploy.rsa ubuntu@<host> — exposing both the SSH username and the operator's private-key filename (evo-app-deploy.rsa).
• Volume layout (docker volume ls | grep -i drive, tar strategy) gives an attacker a reliable map of the live drive container.

This content is entirely unrelated to the JSON/Value unification work this PR is about and the author's own heading explicitly classifies it as not-for-commit. Pairing operator workflow + key-file paths with named live infra is materially more useful for credential-phishing or social-engineering than each fact in isolation. Remove the runbook section before merge; keep operator runbooks in a private store. Consider rotating evo-app-deploy.rsa out of caution.

blocking: wasm-dpp2 TypeScript declarations advertise `type`/`kind` while rs-dpp runtime emits `$type`/`$kind`

packages/wasm-dpp2/src/asset_lock_proof/proof.rs (line 18-38)

STILL VALID at head a592d79. The #[wasm_bindgen(typescript_custom_section)] at proof.rs:23-37 declares AssetLockProofObject/AssetLockProofJSON as { type: "instant" | "chain" } & ... and the doc-comment at line 25 still says #[serde(tag = "type")]. The actual rs-dpp enum at packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49 uses #[serde(tag = "$type", rename_all = "camelCase")], and the wasm-dpp2 JS unit specs updated earlier in this PR assert $type on the runtime objects.

The same mismatch persists across this PR's TS sections: packages/wasm-dpp2/src/voting/winner_info.rs:18-28, packages/wasm-dpp2/src/voting/vote_poll.rs:39,50, packages/wasm-dpp2/src/voting/resource_vote_choice.rs:19-29, packages/wasm-dpp2/src/shielded/address_witness.rs:17,25,39,47, packages/wasm-dpp2/src/platform_address/fee_strategy.rs:18-30, and packages/wasm-dpp2/src/group/token_event.rs — all corresponding to rs-dpp enums migrated to serde(tag = "$type") / serde(tag = "$kind") by 7f63b89.

Net effect for wasm-dpp2 consumers compiling against the published .d.ts:

1. JSON.parse(json).type is undefined; the discriminator is at $type.
2. Every TS discriminated-union narrowing case "instant": resolves to never.
3. JS code constructing { type: "instant", ... } per the TS declaration is rejected by from_object/fromJSON strict deserialization.

Sync each typescript_custom_section literal and the Mirrors the rs-dpp serde shape doc-comment to the runtime tag ($type, and $kind for GroupAction). Also update CONVENTIONS.md / any Mirrors the rs-dpp serde shape (... "type" ...) text in matching JS doc-comments.

#[wasm_bindgen(typescript_custom_section)]
const TS_TYPES: &str = r#"
/**
 * AssetLockProof serialized as a plain object.
 *
 * Internally-tagged discriminated union — `$type` discriminates the variant and
 * the variant's fields sit alongside it. Mirrors the rs-dpp serde shape (which
 * uses `#[serde(tag = "$type")]` on the enum) and the convention used by
 * `AddressWitness` / `AddressFundsFeeStrategyStep`.
 */
export type AssetLockProofObject =
    | ({ $type: "instant" } & InstantAssetLockProofObject)
    | ({ $type: "chain" } & ChainAssetLockProofObject);

/**
 * AssetLockProof serialized as JSON.
 */
export type AssetLockProofJSON =
    | ({ $type: "instant" } & InstantAssetLockProofJSON)
    | ({ $type: "chain" } & ChainAssetLockProofJSON);
"#;

suggestion: Lenient constructor does not default or alias `$identity-contract-nonce`, breaking existing JS callers

packages/wasm-dpp/src/data_contract/state_transition/data_contract_update_transition/mod.rs (line 52-79)

Introduced by the latest delta. The rewritten constructor calls strict DataContractUpdateTransition::from_object(raw) after defaulting $formatVersion, userFeeIncrease, signaturePublicKeyId, and signature for legacy callers, but does not handle the identity_contract_nonce field — required at packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v0/mod.rs:33-38 with the serde rename "$identity-contract-nonce".

Two existing JS specs construct new DataContractUpdateTransition({...}) without that field at packages/wasm-dpp/test/integration/dataContract/stateTransition/DataContractUpdateTransition/validation/basic/validateDataContractUpdateTransitionBasicFactory.spec.js:47-52 and packages/wasm-dpp/test/unit/dataContract/stateTransition/DataContractUpdateTransition/validation/state/validateDataContractUpdateTransitionStateFactory.spec.js:40-43. Strict from_object will reject these with a missing-field error. The one spec updated in this delta (DataContractUpdateTransition.spec.js:24) had to add the canonical key explicitly, confirming the breakage.

Either default the nonce to 0 alongside the other ensure_* calls if the legacy constructor was previously permissive about its absence, or update/skip the two remaining specs to supply the canonical key. If a legacy camelCase alias (identityContractNonce) was ever supported by the pre-canonical lenient path, translate it to the canonical name as part of the ensure_* block.

suggestion: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round Credits above 2^53−1

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

STILL VALID at head a592d79. TokenPricingSchedule (lines 28-45) derives plain Serialize/Deserialize with no #[json_safe_fields], and the canonical impls (lines 48, 51) are blanket impls with no Repr wrapper. The variants SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) both expose raw u64 on the JSON wire surface; the tests at lines 100-123 pin the raw numeric shape {"SinglePrice": 1234} / {"SetPrices": {"5": 50, "10": 80}}.

This PR establishes a stated #[json_safe_fields] convention applied to 25+ V0/V1 struct leaves to protect JS clients from silently rounding u64 values above 2^53−1. Token prices use the 100-billion-per-Dash credit scale (per CLAUDE.md), so a tier price above ~92,233 DASH (~9.2 × 10^15 duffs) drops precision when round-tripped through JsonConvertible::to_json and consumed by JSON.parse on the JS side. Because TokenPricingSchedule is referenced from TokenConfiguration distribution rules, this can manifest as off-by-rounding fees with no JS-side error.

Apply #[json_safe_fields] (or a Repr wrapper with json_safe_u64 on the inner Credits/TokenAmount fields) so JS receives the decimal-string form, matching the convention applied throughout this PR.

suggestion: TokenDistributionInfo round-trip tests only pin PreProgrammed; Perpetual and TokenDistributionTypeWithResolvedRecipient unpinned

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322-389)

STILL VALID at head a592d79. mod json_convertible_tests_token_distribution_info (lines 322-389) defines a single fixture TokenDistributionInfo::PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32])) and pins both JSON and Value wire shapes only for that variant. The Perpetual { moment, recipient } variant — where moment is RewardDistributionMoment (an internally-tagged enum migrated to $type in 51d3193) and recipient is TokenDistributionRecipient — is not pinned, and TokenDistributionTypeWithResolvedRecipient (the resolved-recipient wrapper with its own $type-tagged repr) is also missing canonical wire-shape pins.

The broader internally-tag everything with $type sweep is the consensus-critical change being landed here; the unpinned Perpetual variant is exactly where a future regression (missing tag = "$type" or a silent variant-flip on a nested enum) would land without a fixture there to catch it. Mirror the PreProgrammed test for at least one Perpetual case (e.g. BlockBasedTimeInterval) and one TokenDistributionTypeWithResolvedRecipient case.

suggestion: ArrayItemType canonical round-trip tests miss Boolean and Date variants

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 706-842)

STILL VALID at head a592d79. mod json_convertible_tests (lines 712-842) pins JSON + Value wire shapes for only five of ArrayItemType's seven variants: Integer (716, 782), Number (729, 793), String (740, 804), ByteArray (757, 819), and Identifier (771, 833). The two unit variants Boolean and Date have no canonical wire-shape pin in either path.

This was the last enum landed in the internally tag everything with $type sweep (commit c341be6) and is the consensus-critical schema-definition surface for document contracts; a future serde-rename slip on Boolean ({"$type":"boolean"} vs {"$type":"bool"}) would silently change contract document schemas with no test failure.

Add two more tests symmetric to the existing pattern, one each for ArrayItemType::Boolean and ArrayItemType::Date, asserting the JSON and Value layers.

🤖 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 `docs/v12-upgrade-boundary-risks.md`:119-135: Committed runbook self-labelled 'do not commit' discloses live HPMN validator IP, SSH user, and operator SSH key path
  STILL VALID at head a592d79bec. The section heading at line 119 reads `## Runbook (local only — contains infra details, do not commit)`, but the body publishes operator-sensitive infrastructure metadata the author themselves marked as private:

- `hp-masternode-8` = `68.67.122.8` is identified as a live HPMN testnet validator with ansible_user `ubuntu` and `do NOT stop the drive container` annotation.
- `54.185.219.212` is identified as a disposable mainnet-synced node.
- SSH access template is published verbatim: `ssh -i ~/.ssh/evo-app-deploy.rsa ubuntu@<host>` — exposing both the SSH username and the operator's private-key filename (`evo-app-deploy.rsa`).
- Volume layout (`docker volume ls | grep -i drive`, tar strategy) gives an attacker a reliable map of the live drive container.

This content is entirely unrelated to the JSON/Value unification work this PR is about and the author's own heading explicitly classifies it as not-for-commit. Pairing operator workflow + key-file paths with named live infra is materially more useful for credential-phishing or social-engineering than each fact in isolation. Remove the runbook section before merge; keep operator runbooks in a private store. Consider rotating `evo-app-deploy.rsa` out of caution.
- [BLOCKING] In `packages/wasm-dpp2/src/asset_lock_proof/proof.rs`:18-38: wasm-dpp2 TypeScript declarations advertise `type`/`kind` while rs-dpp runtime emits `$type`/`$kind`
  STILL VALID at head a592d79bec. The `#[wasm_bindgen(typescript_custom_section)]` at proof.rs:23-37 declares `AssetLockProofObject`/`AssetLockProofJSON` as `{ type: "instant" | "chain" } & ...` and the doc-comment at line 25 still says `#[serde(tag = "type")]`. The actual rs-dpp enum at packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49 uses `#[serde(tag = "$type", rename_all = "camelCase")]`, and the wasm-dpp2 JS unit specs updated earlier in this PR assert `$type` on the runtime objects.

The same mismatch persists across this PR's TS sections: `packages/wasm-dpp2/src/voting/winner_info.rs:18-28`, `packages/wasm-dpp2/src/voting/vote_poll.rs:39,50`, `packages/wasm-dpp2/src/voting/resource_vote_choice.rs:19-29`, `packages/wasm-dpp2/src/shielded/address_witness.rs:17,25,39,47`, `packages/wasm-dpp2/src/platform_address/fee_strategy.rs:18-30`, and `packages/wasm-dpp2/src/group/token_event.rs` — all corresponding to rs-dpp enums migrated to `serde(tag = "$type")` / `serde(tag = "$kind")` by 7f63b89.

Net effect for `wasm-dpp2` consumers compiling against the published `.d.ts`:
1. `JSON.parse(json).type` is `undefined`; the discriminator is at `$type`.
2. Every TS discriminated-union narrowing `case "instant":` resolves to `never`.
3. JS code constructing `{ type: "instant", ... }` per the TS declaration is rejected by `from_object`/`fromJSON` strict deserialization.

Sync each `typescript_custom_section` literal and the `Mirrors the rs-dpp serde shape` doc-comment to the runtime tag (`$type`, and `$kind` for GroupAction). Also update CONVENTIONS.md / any `Mirrors the rs-dpp serde shape (... "type" ...)` text in matching JS doc-comments.
- [SUGGESTION] In `packages/wasm-dpp/src/data_contract/state_transition/data_contract_update_transition/mod.rs`:52-79: Lenient constructor does not default or alias `$identity-contract-nonce`, breaking existing JS callers
  Introduced by the latest delta. The rewritten constructor calls strict `DataContractUpdateTransition::from_object(raw)` after defaulting `$formatVersion`, `userFeeIncrease`, `signaturePublicKeyId`, and `signature` for legacy callers, but does not handle the `identity_contract_nonce` field — required at `packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v0/mod.rs:33-38` with the serde rename `"$identity-contract-nonce"`.

Two existing JS specs construct `new DataContractUpdateTransition({...})` without that field at `packages/wasm-dpp/test/integration/dataContract/stateTransition/DataContractUpdateTransition/validation/basic/validateDataContractUpdateTransitionBasicFactory.spec.js:47-52` and `packages/wasm-dpp/test/unit/dataContract/stateTransition/DataContractUpdateTransition/validation/state/validateDataContractUpdateTransitionStateFactory.spec.js:40-43`. Strict `from_object` will reject these with a missing-field error. The one spec updated in this delta (`DataContractUpdateTransition.spec.js:24`) had to add the canonical key explicitly, confirming the breakage.

Either default the nonce to `0` alongside the other ensure_* calls if the legacy constructor was previously permissive about its absence, or update/skip the two remaining specs to supply the canonical key. If a legacy camelCase alias (`identityContractNonce`) was ever supported by the pre-canonical lenient path, translate it to the canonical name as part of the ensure_* block.
- [SUGGESTION] In `packages/rs-dpp/src/tokens/token_pricing_schedule.rs`:28-51: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round Credits above 2^53−1
  STILL VALID at head a592d79bec. `TokenPricingSchedule` (lines 28-45) derives plain `Serialize`/`Deserialize` with no `#[json_safe_fields]`, and the canonical impls (lines 48, 51) are blanket impls with no Repr wrapper. The variants `SinglePrice(Credits)` and `SetPrices(BTreeMap<TokenAmount, Credits>)` both expose raw `u64` on the JSON wire surface; the tests at lines 100-123 pin the raw numeric shape `{"SinglePrice": 1234}` / `{"SetPrices": {"5": 50, "10": 80}}`.

This PR establishes a stated `#[json_safe_fields]` convention applied to 25+ V0/V1 struct leaves to protect JS clients from silently rounding u64 values above 2^53−1. Token prices use the 100-billion-per-Dash credit scale (per CLAUDE.md), so a tier price above ~92,233 DASH (~9.2 × 10^15 duffs) drops precision when round-tripped through `JsonConvertible::to_json` and consumed by `JSON.parse` on the JS side. Because `TokenPricingSchedule` is referenced from `TokenConfiguration` distribution rules, this can manifest as off-by-rounding fees with no JS-side error.

Apply `#[json_safe_fields]` (or a Repr wrapper with `json_safe_u64` on the inner `Credits`/`TokenAmount` fields) so JS receives the decimal-string form, matching the convention applied throughout this PR.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs`:322-389: TokenDistributionInfo round-trip tests only pin PreProgrammed; Perpetual and TokenDistributionTypeWithResolvedRecipient unpinned
  STILL VALID at head a592d79bec. `mod json_convertible_tests_token_distribution_info` (lines 322-389) defines a single fixture `TokenDistributionInfo::PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32]))` and pins both JSON and Value wire shapes only for that variant. The `Perpetual { moment, recipient }` variant — where `moment` is `RewardDistributionMoment` (an internally-tagged enum migrated to `$type` in 51d31930) and `recipient` is `TokenDistributionRecipient` — is not pinned, and `TokenDistributionTypeWithResolvedRecipient` (the resolved-recipient wrapper with its own `$type`-tagged repr) is also missing canonical wire-shape pins.

The broader `internally-tag everything with $type` sweep is the consensus-critical change being landed here; the unpinned `Perpetual` variant is exactly where a future regression (missing `tag = "$type"` or a silent variant-flip on a nested enum) would land without a fixture there to catch it. Mirror the `PreProgrammed` test for at least one Perpetual case (e.g. `BlockBasedTimeInterval`) and one `TokenDistributionTypeWithResolvedRecipient` case.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/document_type/property/array.rs`:706-842: ArrayItemType canonical round-trip tests miss Boolean and Date variants
  STILL VALID at head a592d79bec. `mod json_convertible_tests` (lines 712-842) pins JSON + Value wire shapes for only five of `ArrayItemType`'s seven variants: Integer (716, 782), Number (729, 793), String (740, 804), ByteArray (757, 819), and Identifier (771, 833). The two unit variants `Boolean` and `Date` have no canonical wire-shape pin in either path.

This was the last enum landed in the `internally tag everything with $type` sweep (commit c341be6cb7) and is the consensus-critical schema-definition surface for document contracts; a future serde-rename slip on Boolean (`{"$type":"boolean"}` vs `{"$type":"bool"}`) would silently change contract document schemas with no test failure.

Add two more tests symmetric to the existing pattern, one each for `ArrayItemType::Boolean` and `ArrayItemType::Date`, asserting the JSON and Value layers.

[thepastaclaw]

Code Review

Verified at head ed30168. Latest delta is a small feature-gating repair: ungates crate::serialization::serde_bytes so it compiles in every feature configuration (now needed by always-serde ExtendedBlockInfoV0.signature: [u8; 96]) and removes erroneous #[cfg(feature = "json-conversion"|"value-conversion")] gates from state-transition submodules so rs-dapi (state-transitions-only) compiles. No new defects introduced. All six coordinator-prior-findings re-verified against current head: prior-1, prior-2, prior-3, prior-4, prior-5, prior-6 are STILL VALID and carried forward unchanged. The codex-security latest-delta finding on bounded byte deserialization is dropped as speculative pre-existing hardening unrelated to this PR's scope.

Reviewed commit: ed30168

🔴 2 blocking | 🟡 4 suggestion(s)

Verified findings

blocking: Committed runbook self-labelled 'do not commit' discloses live HPMN validator IP, SSH user, and operator key path

docs/v12-upgrade-boundary-risks.md (line 119)

Re-verified at head ed30168 — file unchanged by the latest delta. The section heading itself reads ## Runbook (local only — contains infra details, do not commit), but the body then publishes operator-sensitive metadata: live testnet HPMN validator hp-masternode-8 = 68.67.122.8 (explicitly identified as a producing validator: "do NOT stop the drive container"), a second host IP 54.185.219.212, SSH user ubuntu, the operator's SSH key path ~/.ssh/evo-app-deploy.rsa, and step-by-step ssh/scp extraction commands. dashpay/platform is a public repository — anyone reading this PR (or the git history once it lands) gets a confirmed live validator IP, the SSH account name, the exact key filename, and confirmation of the validator's Docker layout. This is meaningful reconnaissance against the operator and contradicts the author's own privacy marker. The runbook is also out of scope for the JSON/Value unification work. Remove (or move to local-only storage) before merge; consider history rewrite + key rotation if this commit was already pushed to a public ref.

blocking: wasm-dpp2 TypeScript declarations advertise `type` while rs-dpp runtime emits `$type`

packages/wasm-dpp2/src/asset_lock_proof/proof.rs (line 18)

Re-verified at head ed30168 — file unchanged by the latest delta. The typescript_custom_section declares AssetLockProofObject/AssetLockProofJSON as ({ type: "instant" | "chain" } & ...) and the doc-comment at line 25 still claims the rs-dpp enum uses #[serde(tag = "type")]. The canonical enum at packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49 actually uses #[serde(tag = "$type", rename_all = "camelCase")] (this PR's deliberate Critical-2 fix). TS consumers constructing { type: "instant", … } will fail rs-dpp deserialization at the wasm boundary; consumers branching on obj.type will never match the runtime shape (which carries $type). Worse, the TS type permits objects with both type and $type present, allowing a UI/TS layer to think it's signing one variant while the runtime dispatches on $type and produces the other — exactly the cross-variant confusion the $type rename was meant to prevent. Same divergence pattern likely exists across other wasm-dpp2 typescript_custom_section blocks (winner_info, resource_vote_choice, vote_poll, fee_strategy, address_witness, token_event) — sweep with grep before merge.

#[wasm_bindgen(typescript_custom_section)]
const TS_TYPES: &str = r#"
/**
 * AssetLockProof serialized as a plain object.
 *
 * Internally-tagged discriminated union — `$type` discriminates the variant and
 * the variant's fields sit alongside it. Mirrors the rs-dpp serde shape (which
 * uses `#[serde(tag = "$type")]` on the enum) and the convention used by
 * `AddressWitness` / `AddressFundsFeeStrategyStep`.
 */
export type AssetLockProofObject =
    | ({ $type: "instant" } & InstantAssetLockProofObject)
    | ({ $type: "chain" } & ChainAssetLockProofObject);

/**
 * AssetLockProof serialized as JSON.
 */
export type AssetLockProofJSON =
    | ({ $type: "instant" } & InstantAssetLockProofJSON)
    | ({ $type: "chain" } & ChainAssetLockProofJSON);
"#;

suggestion: Lenient constructor does not default or alias `identityContractNonce`, breaking legacy JS callers

packages/wasm-dpp/src/data_contract/state_transition/data_contract_update_transition/mod.rs (line 52)

Re-verified at head ed30168 — file unchanged by the latest delta. The rewritten constructor defaults $formatVersion, userFeeIncrease, signaturePublicKeyId, and signature to keep legacy partial JS input passing the strict from_object(raw) canonical deserializer, but does NOT default or alias the canonical required field identityContractNonce (renamed from the legacy hyphenated $identity-contract-nonce form at packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v0/mod.rs:31-36). External JS consumers that historically passed identityContractNonce / identity_contract_nonce / $identity-contract-nonce, or that omitted the field entirely (intending to set it later via a setter), will now fail at construction with an opaque missing-field error. Because this is replay-protection state, do NOT silently default it to zero — instead, accept the legacy aliases and rename them to the canonical key before calling from_object, so legacy callers continue to work without weakening the nonce contract.

suggestion: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round Credits above 2^53−1

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

Re-verified at head ed30168 — file unchanged by the latest delta. TokenPricingSchedule derives plain Serialize/Deserialize (no #[json_safe_fields] attribute) and the canonical impls at lines 48 and 51 are blanket impls with no SafeU64/Repr wrapper. The variants SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) carry u64 aliases that round-trip through serde_json::Value::Number(u64), which JavaScript silently rounds above Number.MAX_SAFE_INTEGER (2^53 − 1 ≈ 9.0e15). Token pricing carries Credits (1 DASH = 10^11 credits, so ~90 000 DASH exceeds the safe range) — this is exactly the JS-large-integer hazard the rest of this PR's #[json_safe_fields] rollout (25+ V0/V1 leaves) was designed to prevent. A wallet UI fetching a pricing schedule via the JS SDK can silently round the displayed price, leading to a user authorizing one price while the underlying transition charges another. Apply #[json_safe_fields] (or equivalent serde(with = "...") SafeU64 wrappers) consistent with the rest of the codebase.

suggestion: TokenDistributionInfo round-trip tests only pin PreProgrammed; Perpetual + TokenDistributionTypeWithResolvedRecipient unpinned

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322)

Re-verified at head ed30168 — test module unchanged by the latest delta. mod json_convertible_tests_token_distribution_info defines a single fixture TokenDistributionInfo::PreProgrammed(1_700_000_000_000, Identifier::new([0x42; 32])) and pins both JSON and Value wire shapes only for that variant. The Perpetual variant of TokenDistributionInfo and every variant of the sibling enum TokenDistributionTypeWithResolvedRecipient are unpinned. TokenDistributionInfo controls who receives newly distributed tokens and when, so a future serde/tag-shape regression on these variants is a token-routing risk that the current test matrix would not catch. Add literal json!{} / platform_value!{} assertions per variant following the established pattern at lines 340-388.

suggestion: ArrayItemType canonical round-trip tests miss Boolean and Date variants

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 706)

Re-verified at head ed30168 — test module unchanged by the latest delta. mod json_convertible_tests (lines 712-842) pins JSON + Value wire shapes for only five of ArrayItemType's seven variants: Integer, Number, String, ByteArray, Identifier. The unit variants Boolean and Date remain unpinned. These shape a document property's array-element type and feed the schema validator; a silent wire-shape regression (e.g. boolean → bool, date → dateTime) would not be caught. Add four short tests mirroring the existing json_round_trip_integer_variant / value_round_trip_integer_variant patterns so the full 7-variant enum is locked in.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3573 ed30168 --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

Re-verified all six coordinator-supplied prior findings against head a496b0b — every one is STILL VALID and carried forward. The latest delta (ed30168..a496b0b) is dominated by a v3.1-dev merge of ~400 unrelated files; it did not touch any of the prior-flagged lines. Beyond carry-forwards, this verification also confirms a systematic type vs $type discriminator mismatch across multiple wasm-dpp2 TypeScript custom sections (the same root cause as prior-2 in proof.rs, now confirmed in fee_strategy.rs, vote_poll.rs, resource_vote_choice.rs, winner_info.rs, address_witness.rs, and the TokenEvent doc-comment) — rs-dpp custom Serialize impls emit $type everywhere but the public TS surface advertises type. Consolidated into one additional blocking finding to stay within the 10-finding budget. The Swift identityToHandle dangling-pointer issue raised by codex-ffi-engineer is real but lives in a pre-existing file untouched by this PR's own commits (introduced by upstream PRs pulled in via the v3.1-dev merge) and is out of scope for this PR's stated JSON/Value conversion goal.

Reviewed commit: a496b0b

🔴 3 blocking | 🟡 4 suggestion(s)

Verified findings

blocking: Committed runbook self-labelled 'do not commit' discloses live HPMN validator IP, SSH user, and operator key path

docs/v12-upgrade-boundary-risks.md (line 119)

Re-verified at HEAD a496b0b — file unchanged by the v3.1-dev merge delta. The section heading at L119 itself reads ## Runbook (local only — contains infra details, do not commit), yet the body (L121–135) publishes operator-sensitive metadata into a public-repo branch staged for merge: live testnet HPMN validator hp-masternode-8 = 68.67.122.8, secondary host 54.185.219.212, SSH user ubuntu, operator key path ~/.ssh/evo-app-deploy.rsa, and step-by-step ssh/scp extraction commands targeting the drive volume. The 'do not commit' caveat in the heading does not undo the disclosure once committed.

Remove this section from the PR (move to private/local-only storage). Because the branch was pushed to GitHub, content may already be indexed; consider history rewrite and rotation of the named SSH key, and review whether the named validator host needs hardening.

blocking: TypeScript declarations for AssetLockProof advertise `type` while rs-dpp runtime emits `$type`

packages/wasm-dpp2/src/asset_lock_proof/proof.rs (line 18)

Re-verified at HEAD a496b0b — file unchanged by the latest delta. The typescript_custom_section (L19–38) declares AssetLockProofObject / AssetLockProofJSON as unions discriminated by type, and the in-file doc-comment claims rs-dpp uses #[serde(tag = "type")]. The actual rs-dpp source at packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49 is #[serde(tag = "$type", rename_all = "camelCase")] for both AssetLockProof Serialize and RawAssetLockProof Deserialize.

Consequences across the wasm boundary:

• TS callers constructing { type: "instant", ... } produce objects the rs-dpp Deserialize rejects (or treats the type field as a non-tag map entry).
• TS callers branching on proof.type never see the actual $type discriminator and silently take the wrong branch — chain-locked proofs could be treated as instant-locked.

Update both TS aliases and the doc-comment to use $type so the public TS surface matches the wire shape (consistent with AddressWitness / AddressFundsFeeStrategyStep conventions).

#[wasm_bindgen(typescript_custom_section)]
const TS_TYPES: &str = r#"
/**
 * AssetLockProof serialized as a plain object.
 *
 * Internally-tagged discriminated union — `$type` discriminates the variant and
 * the variant's fields sit alongside it. Mirrors the rs-dpp serde shape (which
 * uses `#[serde(tag = "$type")]` on the enum) and the convention used by
 * `AddressWitness` / `AddressFundsFeeStrategyStep`.
 */
export type AssetLockProofObject =
    | ({ $type: "instant" } & InstantAssetLockProofObject)
    | ({ $type: "chain" } & ChainAssetLockProofObject);

/**
 * AssetLockProof serialized as JSON.
 */
export type AssetLockProofJSON =
    | ({ $type: "instant" } & InstantAssetLockProofJSON)
    | ({ $type: "chain" } & ChainAssetLockProofJSON);
"#;

blocking: Systematic `type` vs `$type` discriminator mismatch across wasm-dpp2 TypeScript custom sections

packages/wasm-dpp2/src/platform_address/fee_strategy.rs (line 8)

The same root-cause defect that affects AssetLockProof exists in at least five additional wasm-dpp2 TypeScript surfaces. In every case the rs-dpp custom Serialize impl emits $type, but the exported TS declaration declares type:

• packages/wasm-dpp2/src/platform_address/fee_strategy.rs:17-19,28-30 — TS declares { type: "deductFromInput"|"reduceOutput"; index: number }; rs-dpp at packages/rs-dpp/src/address_funds/fee_strategy/mod.rs:42,46,99 serializes/deserializes $type via a custom Visitor that errors with missing_field("$type").
• packages/wasm-dpp2/src/voting/vote_poll.rs:38-44,49-55 — TS declares type: "contestedDocumentResourceVotePoll"; rs-dpp at packages/rs-dpp/src/voting/vote_polls/mod.rs:29 uses serde(tag = "$type", rename_all = "camelCase"). The neighboring doc-comment even contradicts the code by claiming the tag is plain type.
• packages/wasm-dpp2/src/voting/resource_vote_choice.rs:18-21,26-29 — TS declares { type: "towardsIdentity"|"abstain"|"lock"; ... }; rs-dpp's custom Serialize at packages/rs-dpp/src/voting/vote_choices/resource_vote_choice/mod.rs:47,53,58,85,103 keys on $type.
• packages/wasm-dpp2/src/voting/winner_info.rs:17-20,25-28 — TS declares { type: "noWinner"|"locked"|"wonByIdentity"; ... }; rs-dpp at packages/rs-dpp/src/voting/vote_info_storage/contested_document_vote_poll_winner_info/mod.rs:31,36,42,70,88 keys on $type (and its own internal tests assert "$type": "wonByIdentity").
• packages/wasm-dpp2/src/shielded/address_witness.rs:16-49 — TS declares type: "p2pkh"|"p2sh"; rs-dpp at packages/rs-dpp/src/address_funds/witness.rs:32 is serde(tag = "$type").
• packages/wasm-dpp2/src/group/token_event.rs:7-46 — TS interface is permissive ({ type: string; [field: string]: unknown }), but every example in the doc-comment uses { type: ... } while the rs-dpp custom Serialize at packages/rs-dpp/src/tokens/token_event.rs:194,202,210,217,... emits "$type". Typed callers following the doc-comment will build undeserializable events.

The failure mode is identical across all of these: JS callers construct or branch on obj.type while the runtime object only has $type. Either the wire shape is wrong and rs-dpp should emit type (consistent with the in-tree vote_poll doc-comment), or the TS declarations are wrong and should advertise $type (consistent with the AddressWitness/AddressFundsFeeStrategyStep convention the rest of this PR cites). Pick one and align all sites — leaving them divergent guarantees silent client/server disagreement across the wasm boundary on every one of these enums.

suggestion: Lenient constructor does not default or alias `identityContractNonce`, breaking legacy JS callers

packages/wasm-dpp/src/data_contract/state_transition/data_contract_update_transition/mod.rs (line 52)

Re-verified at HEAD a496b0b — file unchanged by the latest delta. The rewritten constructor (L62–75) builds a lenient pre-pass that ensures $formatVersion, userFeeIncrease, signaturePublicKeyId, and signature are present before delegating to strict DataContractUpdateTransition::from_object(raw). But identityContractNonce is canonical-required and is neither defaulted nor aliased from legacy spellings (e.g. $identity-contract-nonce / identity_contract_nonce). Existing JS callers that previously constructed a transition from minimal inputs and signed it afterwards will now hit from_object failure on construction.

Do NOT default to zero — zero is a valid nonce, and silently substituting it would let unsigned/wrong-nonce transitions through with replay/ordering risk. Either accept the legacy alias by remapping it to identityContractNonce before from_object, or document the migration explicitly in the PR's migration notes so consumers know to rename.

suggestion: TokenPricingSchedule canonical JSON emits raw u64 — JS clients silently round Credits above 2^53−1

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

Re-verified at HEAD a496b0b — file unchanged by the latest delta. TokenPricingSchedule derives plain Serialize/Deserialize (L28) with no #[json_safe_fields] attribute, and the JsonConvertible/ValueConvertible impls (L48, L51) are blanket impls with no SafeU64/Repr wrapper.

SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) both carry u64 aliases. When the canonical JSON is consumed by JS at the wasm boundary, any value above Number.MAX_SAFE_INTEGER (2^53 − 1 ≈ 9.007e15) will silently lose precision. Credit-denominated values for tokens can easily exceed that bound (1 Dash = 1e11 credits, so 100k Dash worth = 1e16 credits, past the safe range). A wallet quoting a price or signing a direct-purchase transition could end up agreeing to different economic terms than the contract author signed.

The PR established #[json_safe_fields] as the canonical convention for large-integer protection. This type should adopt it to be consistent with the 25+ V0/V1 leaves the PR description claims now carry it.

suggestion: TokenDistributionInfo round-trip tests only pin PreProgrammed; Perpetual + TokenDistributionTypeWithResolvedRecipient unpinned

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (line 322)

Re-verified at HEAD a496b0b — test module unchanged. mod json_convertible_tests_token_distribution_info (L322–389) defines a single fixture: TokenDistributionInfo::PreProgrammed(...). Only that variant has JSON and Value wire-shape pinning. The Perpetual variant of TokenDistributionInfo and the related TokenDistributionTypeWithResolvedRecipient enum (also in this file) are unpinned.

This is the same wire-shape regression risk the PR sets out to prevent for all canonical types — a future refactor of either unpinned variant (rename, tag change, field reorder) can silently break the wire shape without any test failing. Add two more fixture-and-assert tests (one for Perpetual, one for the resolved-recipient enum) to match the convention established by ~85 tests elsewhere in the PR.

suggestion: ArrayItemType canonical round-trip tests miss Boolean and Date variants

packages/rs-dpp/src/data_contract/document_type/property/array.rs (line 706)

Re-verified at HEAD a496b0b — test module unchanged. The ArrayItemType enum (L9–17) has seven variants: Integer, Number, String, ByteArray, Identifier, Boolean, Date. The json_convertible_tests module (L706–842) pins JSON + Value wire shapes for only the first five — Boolean and Date are uncovered.

Both missing variants have production callers that branch on type (Date even has special I64→U64 coercion at L168 in this same file), so any silent rename or tag-shape change is a real wire-shape regression risk. Add the four missing tests (json + value × Boolean + Date) to close the symmetry the rest of the module establishes.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3573 a496b0b --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]

Automated review for 955e5eac.

Body-only fallback: GitHub refused PR diff retrieval for this PR (>300 files), so inline mapping was impossible; posting the same verifier-approved findings as an exact-SHA body-only review.

Summary

Incremental delta a496b0b..955e5ea is comment-only and fixes none of the prior review findings. All seven prior findings are STILL VALID at current HEAD; two additional in-scope suggestions were verified in the cumulative PR surface.

Verified findings

1. blocking: Committed local runbook exposes live validator infrastructure details

docs/v12-upgrade-boundary-risks.md:119-135

Prior status: STILL VALID. This section is explicitly labeled local-only and 'do not commit', but it is checked into the PR and publishes live HPMN/testnet host information, a second host IP, SSH username, operator key path, and concrete ssh/scp dump commands. That is operator-sensitive reconnaissance material in a public repository branch and should not ship with the JSON/Value conversion work. Remove the runbook from the PR and treat the already-pushed host/key details as exposed.

2. blocking: AssetLockProof TypeScript declarations advertise `type` while rs-dpp uses `$type`

packages/wasm-dpp2/src/asset_lock_proof/proof.rs:18-38

Prior status: STILL VALID. The exported TypeScript custom section tells JS callers to construct and inspect AssetLockProof objects with a plain type discriminator, and the comment claims rs-dpp uses #[serde(tag = "type")]. The actual rs-dpp enum and raw deserializer use #[serde(tag = "$type", rename_all = "camelCase")], so callers following these typings will send objects the Rust side rejects and will branch on a field that runtime output does not contain. Align the TS declarations and comments with the executable $type wire shape.

#[wasm_bindgen(typescript_custom_section)]
const TS_TYPES: &str = r#"
/**
 * AssetLockProof serialized as a plain object.
 *
 * Internally-tagged discriminated union: `$type` discriminates the variant and
 * the variant's fields sit alongside it. Mirrors the rs-dpp serde shape (which
 * uses `#[serde(tag = "$type")]` on the enum) and the convention used by
 * `AddressWitness` / `AddressFundsFeeStrategyStep`.
 */
export type AssetLockProofObject =
    | ({ $type: "instant" } & InstantAssetLockProofObject)
    | ({ $type: "chain" } & ChainAssetLockProofObject);

/**
 * AssetLockProof serialized as JSON.
 */
export type AssetLockProofJSON =
    | ({ $type: "instant" } & InstantAssetLockProofJSON)
    | ({ $type: "chain" } & ChainAssetLockProofJSON);
"#;

3. blocking: Additional wasm-dpp2 TypeScript surfaces still use `type` instead of `$type`

packages/wasm-dpp2/src/platform_address/fee_strategy.rs:8-31

Prior status: STILL VALID. This same discriminator mismatch appears beyond AssetLockProof. FeeStrategyStepObject and FeeStrategyStepJSON declare { type: ... }, while AddressFundsFeeStrategyStep serializes $type and its deserializer errors on missing $type; the same stale plain-type pattern remains in VotePoll, ResourceVoteChoice, ContestedDocumentVotePollWinnerInfo, AddressWitness, and TokenEvent TypeScript custom sections or examples. These are public JS contracts for the new canonical conversion surface, so they must match the runtime objects emitted and accepted by rs-dpp.

4. suggestion: Lenient update-transition constructor no longer accepts legacy nonce field

packages/wasm-dpp/src/data_contract/state_transition/data_contract_update_transition/mod.rs:52-79

Prior status: STILL VALID. The constructor now pre-fills $formatVersion, userFeeIncrease, signaturePublicKeyId, and signature before strict from_object, but it does not map the previous JS constructor field identityContractNonce to the canonical serde field $identity-contract-nonce. The PR even changes the existing JS test fixture from identityContractNonce to $identity-contract-nonce, which means existing callers following the old public constructor shape will fail construction. Keep requiring a real nonce, but alias the legacy field or document the migration explicitly.

5. suggestion: TokenPricingSchedule JSON conversion emits u64 prices as unsafe JS numbers

packages/rs-dpp/src/tokens/token_pricing_schedule.rs:28-51

Prior status: STILL VALID. TokenPricingSchedule derives plain serde and its JsonConvertible/ValueConvertible impls do not use json_safe_fields or an equivalent wrapper, so SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) serialize credit and token amounts as raw JSON numbers. Those u64 values can exceed JavaScript's safe integer range, causing wasm/JS callers to round token prices or thresholds silently. Apply the same large-integer-safe JSON convention used elsewhere in this PR's conversion sweep.

6. suggestion: TokenDistributionInfo tests pin only the PreProgrammed wire shape

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs:322-389

Prior status: STILL VALID. The canonical conversion tests for TokenDistributionInfo use only a PreProgrammed fixture, leaving the Perpetual variant unpinned. The related TokenDistributionTypeWithResolvedRecipient enum also has JsonConvertible/ValueConvertible impls but no literal wire-shape tests. Add fixture assertions for the missing variants so future tag, field, or payload-shape changes fail in the test suite.

7. suggestion: ArrayItemType conversion tests miss Boolean and Date variants

packages/rs-dpp/src/data_contract/document_type/property/array.rs:706-842

Prior status: STILL VALID. ArrayItemType has seven variants, but the canonical JSON/Value tests only pin Integer, Number, String, ByteArray, and Identifier. Boolean and Date are still uncovered, even though Date has dedicated conversion behavior in this file. Add JSON and Value wire-shape assertions for both variants to complete the variant matrix introduced by this PR.

8. suggestion: C/Swift data-contract JSON no longer uses the SDK protocol version

packages/rs-sdk-ffi/src/data_contract/queries/fetch_with_serialization.rs:110-112

New finding. This PR changed the FFI JSON path from contract.to_json(wrapper.sdk.version()) to serde_json::to_value(&contract). DataContract's serde impl explicitly calls PlatformVersion::get_version_or_current_or_latest(None), so the JSON returned to C/Swift can now be serialized with process current/latest platform version instead of the version pinned on this SDK handle. The same regression is present in fetch_json.rs; build DataContractInSerializationFormat with wrapper.sdk.version() before serializing JSON across the FFI boundary.

9. suggestion: BLS public-key sequence deserialization allocates unbounded input before rejecting it

packages/rs-dpp/src/serialization/dashcore/bls_pubkey.rs:151-157

New finding. The new BLS public-key serde wrapper accepts byte sequences for JSON/Value round-trips, but visit_seq pushes every element into a Vec and only checks that the length is exactly 48 after the sequence ends. A malformed payload with a very large array in a public-key slot forces unnecessary allocation and parsing before the deserializer can reject it. Stop as soon as more than 48 bytes are seen.

    fn visit_seq<A: serde::de::SeqAccess<'de>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
        let mut bytes = Vec::with_capacity(COMPRESSED_G1_LEN);
        while let Some(b) = seq.next_element::<u8>()? {
            if bytes.len() >= COMPRESSED_G1_LEN {
                return Err(serde::de::Error::custom(format!(
                    "expected {COMPRESSED_G1_LEN} compressed-G1 bytes for public key, got more"
                )));
            }
            bytes.push(b);
        }
        from_compressed_g1_bytes(&bytes)
    }

Dropped / out-of-scope observations

• JS→Rust boundary: 32-byte Uint8Array silently re-typed as Value::Identifier — Out of scope for this PR. The heuristic is pre-existing and was not introduced or newly relied on by the JSON/Value conversion changes under review.
• JS→Rust boundary: f64 as u64 cast loses precision silently above 2^53 — Out of scope for this PR. The conversion behavior predates the reviewed changes and should be handled as a separate wasm input-boundary cleanup.
• Pre-existing AssetLockProof variant getters can panic from JS — Out of scope for this PR. The panicking getters predate the conversion-surface changes and are not part of the current delta.
• C-FFI string return-value ownership documentation should name the free routine — Dropped as routine adjacent documentation cleanup. It is not required for the JSON/Value conversion surface to work and does not affect this review action.

🤖 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 `docs/v12-upgrade-boundary-risks.md`:119-135: Committed local runbook exposes live validator infrastructure details
  Prior status: STILL VALID. This section is explicitly labeled local-only and 'do not commit', but it is checked into the PR and publishes live HPMN/testnet host information, a second host IP, SSH username, operator key path, and concrete ssh/scp dump commands. That is operator-sensitive reconnaissance material in a public repository branch and should not ship with the JSON/Value conversion work. Remove the runbook from the PR and treat the already-pushed host/key details as exposed.
- [BLOCKING] In `packages/wasm-dpp2/src/asset_lock_proof/proof.rs`:18-38: AssetLockProof TypeScript declarations advertise `type` while rs-dpp uses `$type`
  Prior status: STILL VALID. The exported TypeScript custom section tells JS callers to construct and inspect AssetLockProof objects with a plain `type` discriminator, and the comment claims rs-dpp uses `#[serde(tag = "type")]`. The actual rs-dpp enum and raw deserializer use `#[serde(tag = "$type", rename_all = "camelCase")]`, so callers following these typings will send objects the Rust side rejects and will branch on a field that runtime output does not contain. Align the TS declarations and comments with the executable `$type` wire shape.
- [BLOCKING] In `packages/wasm-dpp2/src/platform_address/fee_strategy.rs`:8-31: Additional wasm-dpp2 TypeScript surfaces still use `type` instead of `$type`
  Prior status: STILL VALID. This same discriminator mismatch appears beyond AssetLockProof. `FeeStrategyStepObject` and `FeeStrategyStepJSON` declare `{ type: ... }`, while `AddressFundsFeeStrategyStep` serializes `$type` and its deserializer errors on missing `$type`; the same stale plain-`type` pattern remains in VotePoll, ResourceVoteChoice, ContestedDocumentVotePollWinnerInfo, AddressWitness, and TokenEvent TypeScript custom sections or examples. These are public JS contracts for the new canonical conversion surface, so they must match the runtime objects emitted and accepted by rs-dpp.
- [SUGGESTION] In `packages/wasm-dpp/src/data_contract/state_transition/data_contract_update_transition/mod.rs`:52-79: Lenient update-transition constructor no longer accepts legacy nonce field
  Prior status: STILL VALID. The constructor now pre-fills `$formatVersion`, `userFeeIncrease`, `signaturePublicKeyId`, and `signature` before strict `from_object`, but it does not map the previous JS constructor field `identityContractNonce` to the canonical serde field `$identity-contract-nonce`. The PR even changes the existing JS test fixture from `identityContractNonce` to `$identity-contract-nonce`, which means existing callers following the old public constructor shape will fail construction. Keep requiring a real nonce, but alias the legacy field or document the migration explicitly.
- [SUGGESTION] In `packages/rs-dpp/src/tokens/token_pricing_schedule.rs`:28-51: TokenPricingSchedule JSON conversion emits u64 prices as unsafe JS numbers
  Prior status: STILL VALID. `TokenPricingSchedule` derives plain serde and its JsonConvertible/ValueConvertible impls do not use `json_safe_fields` or an equivalent wrapper, so `SinglePrice(Credits)` and `SetPrices(BTreeMap<TokenAmount, Credits>)` serialize credit and token amounts as raw JSON numbers. Those u64 values can exceed JavaScript's safe integer range, causing wasm/JS callers to round token prices or thresholds silently. Apply the same large-integer-safe JSON convention used elsewhere in this PR's conversion sweep.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs`:322-389: TokenDistributionInfo tests pin only the PreProgrammed wire shape
  Prior status: STILL VALID. The canonical conversion tests for TokenDistributionInfo use only a `PreProgrammed` fixture, leaving the `Perpetual` variant unpinned. The related `TokenDistributionTypeWithResolvedRecipient` enum also has JsonConvertible/ValueConvertible impls but no literal wire-shape tests. Add fixture assertions for the missing variants so future tag, field, or payload-shape changes fail in the test suite.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/document_type/property/array.rs`:706-842: ArrayItemType conversion tests miss Boolean and Date variants
  Prior status: STILL VALID. `ArrayItemType` has seven variants, but the canonical JSON/Value tests only pin Integer, Number, String, ByteArray, and Identifier. Boolean and Date are still uncovered, even though Date has dedicated conversion behavior in this file. Add JSON and Value wire-shape assertions for both variants to complete the variant matrix introduced by this PR.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/data_contract/queries/fetch_with_serialization.rs`:110-112: C/Swift data-contract JSON no longer uses the SDK protocol version
  New finding. This PR changed the FFI JSON path from `contract.to_json(wrapper.sdk.version())` to `serde_json::to_value(&contract)`. `DataContract`'s serde impl explicitly calls `PlatformVersion::get_version_or_current_or_latest(None)`, so the JSON returned to C/Swift can now be serialized with process current/latest platform version instead of the version pinned on this SDK handle. The same regression is present in `fetch_json.rs`; build `DataContractInSerializationFormat` with `wrapper.sdk.version()` before serializing JSON across the FFI boundary.
- [SUGGESTION] In `packages/rs-dpp/src/serialization/dashcore/bls_pubkey.rs`:151-157: BLS public-key sequence deserialization allocates unbounded input before rejecting it
  New finding. The new BLS public-key serde wrapper accepts byte sequences for JSON/Value round-trips, but `visit_seq` pushes every element into a Vec and only checks that the length is exactly 48 after the sequence ends. A malformed payload with a very large array in a public-key slot forces unnecessary allocation and parsing before the deserializer can reject it. Stop as soon as more than 48 bytes are seen.

[thepastaclaw]

Code Review

Automated review for 6e5d5683.

Body-only fallback: GitHub refused PR diff retrieval for this PR (>300 files), so inline mapping was impossible; posting the same verifier-approved findings as an exact-SHA body-only review.

Summary

Incremental review against head 6e5d568. Reconciled all 9 prior findings against current code: all 9 are STILL VALID and verified in place. The latest delta restores DataContract validate-by-default Deserialize (with explicit from_json(_, false, _) opt-out) and a behavior pin — that change is internally consistent and correct, so codex-general's new claim that it is a blocking bug is dropped as a false positive. No new in-scope findings emerged. Two blocking issues (committed live-infra runbook; wasm-dpp2 TS type vs runtime $type) gate this PR.

🔴 3 blocking | 🟡 6 suggestion(s)

Carried-forward prior findings

blocking: Committed local runbook exposes live validator infrastructure details

docs/v12-upgrade-boundary-risks.md:119-135

This runbook section is explicitly labeled local only — contains infra details, do not commit, yet it has been checked into the PR. It publishes a live testnet HPMN validator hostname and IP (hp-masternode-8 = 68.67.122.8), a second mainnet-synced host IP (54.185.219.212), the SSH user (ubuntu), the operator key path (~/.ssh/evo-app-deploy.rsa), and the concrete ssh/scp commands used to reach them. That is operator-sensitive reconnaissance material that should not ship in a public branch, and it is unrelated to the JSON/Value conversion work this PR is about. Remove the entire ## Runbook (local only ...) section before merge and treat the already-pushed host/key details as exposed (rotate the deploy key, audit access on the named hosts).

blocking: AssetLockProof TypeScript declarations advertise `type` while rs-dpp uses `$type`

packages/wasm-dpp2/src/asset_lock_proof/proof.rs:18-38

The exported TypeScript custom section tells JS callers to construct and inspect AssetLockProofObject / AssetLockProofJSON with a plain type discriminator, and the doc comment claims rs-dpp uses #[serde(tag = "type")]. The actual rs-dpp enum at packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49 uses #[serde(tag = "$type", rename_all = "camelCase")]. Callers following these public typings will construct objects the Rust side rejects on from_object / fromJSON and will branch on a runtime field that does not exist. Align the TS declarations and surrounding comments with the executable $type wire shape.

#[wasm_bindgen(typescript_custom_section)]
const TS_TYPES: &str = r#"
/**
 * AssetLockProof serialized as a plain object.
 *
 * Internally-tagged discriminated union: `$type` discriminates the variant and
 * the variant's fields sit alongside it. Mirrors the rs-dpp serde shape (which
 * uses `#[serde(tag = "$type")]` on the enum) and the convention used by
 * `AddressWitness` / `AddressFundsFeeStrategyStep`.
 */
export type AssetLockProofObject =
    | ({ $type: "instant" } & InstantAssetLockProofObject)
    | ({ $type: "chain" } & ChainAssetLockProofObject);

/**
 * AssetLockProof serialized as JSON.
 */
export type AssetLockProofJSON =
    | ({ $type: "instant" } & InstantAssetLockProofJSON)
    | ({ $type: "chain" } & ChainAssetLockProofJSON);
"#;

blocking: Additional wasm-dpp2 TypeScript surfaces still use `type` instead of `$type`

packages/wasm-dpp2/src/platform_address/fee_strategy.rs:8-31

Same discriminator mismatch beyond AssetLockProof. FeeStrategyStepObject and FeeStrategyStepJSON declare { type: "deductFromInput" | "reduceOutput"; index }, but the runtime serde in packages/rs-dpp/src/address_funds/fee_strategy/mod.rs emits and requires $type (verified at lines 42, 46, 99, with round-trip tests at 131, 141, 148, 153, 206, 217, 237, 250). The same stale plain-type pattern remains in the wasm-dpp2 TS custom sections / examples for VotePoll, ResourceVoteChoice, ContestedDocumentVotePollWinnerInfo, AddressWitness, and TokenEvent. These are public JS contracts for the new canonical conversion surface — they must match the runtime objects rs-dpp emits and accepts. Sweep every wasm-dpp2 typescript_custom_section and example for type: "..." and rewrite to $type: "...".

#[wasm_bindgen(typescript_custom_section)]
const FEE_STRATEGY_STEP_TS_TYPES: &str = r#"
/**
 * Fee strategy step in Object form (output of a transition's `toObject()`).
 *
 * Discriminated by `$type`: "deductFromInput" reduces an input's contribution
 * by the fee, "reduceOutput" reduces an output's amount by the fee. The
 * `index` selects the input/output position.
 */
export type FeeStrategyStepObject =
    | { $type: "deductFromInput"; index: number }
    | { $type: "reduceOutput"; index: number };

/**
 * Fee strategy step in JSON form (output of a transition's `toJSON()`).
 */
export type FeeStrategyStepJSON =
    | { $type: "deductFromInput"; index: number }
    | { $type: "reduceOutput"; index: number };
"#;

suggestion: C/Swift data-contract JSON no longer pins the SDK protocol version

packages/rs-sdk-ffi/src/data_contract/queries/fetch_with_serialization.rs:110-131

This PR changed the FFI JSON path from contract.to_json(wrapper.sdk.version()) to serde_json::to_value(&contract) (line 111). DataContract's manual Serialize impl in packages/rs-dpp/src/data_contract/conversion/serde/mod.rs:48-60 resolves the version via PlatformVersion::get_version_or_current_or_latest(None), which reads process-global state and falls back to the latest known platform version. The JSON now crossing the C ABI to Swift is therefore governed by global state instead of the version pinned on the SDK handle the caller passed in (wrapper.sdk). In the same FFI result the bincode serialized_data is still produced through SDK-version-aware paths, so a divergence between json_string and serialized_data for the same contract is reachable. The same regression is in packages/rs-sdk-ffi/src/data_contract/queries/fetch_json.rs:54. Build DataContractInSerializationFormat with wrapper.sdk.version() and serialize that explicitly across the FFI boundary.

suggestion: Lenient update-transition constructor no longer accepts the legacy `identityContractNonce` field

packages/wasm-dpp/src/data_contract/state_transition/data_contract_update_transition/mod.rs:52-79

The constructor now pre-fills $formatVersion, userFeeIncrease, signaturePublicKeyId, and signature before strict DataContractUpdateTransition::from_object(raw) (lines 71-76), but it does not alias the previous JS public field identityContractNonce to the canonical serde field $identity-contract-nonce. This PR also rewrites the existing JS fixture from identityContractNonce to $identity-contract-nonce, so existing callers built against the old public constructor shape will fail with a missing-field error from the strict deserializer. Either alias the legacy key into the canonical key inside this Value::Map normalization step (keep requiring a real nonce — do not default it to zero), or document the field rename as a public migration in the PR.

suggestion: TokenPricingSchedule JSON emits u64 prices as unsafe JS numbers

packages/rs-dpp/src/tokens/token_pricing_schedule.rs:28-51

TokenPricingSchedule derives plain serde and its JsonConvertible / ValueConvertible impls (lines 48, 51) do not use json_safe_fields or an equivalent large-integer wrapper. As a result SinglePrice(Credits) and SetPrices(BTreeMap<TokenAmount, Credits>) serialize u64 credit and token amounts as raw JSON numbers. Values above Number.MAX_SAFE_INTEGER (2^53) will be silently rounded by JavaScript wasm/SDK callers that go through toJSON() — a financial-amount truncation in the same conversion sweep this PR introduces. Apply the safe-integer JSON convention used elsewhere in the PR to both variants.

suggestion: TokenDistributionInfo conversion tests pin only the PreProgrammed variant

packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs:322-389

The canonical conversion tests for TokenDistributionInfo use only a PreProgrammed fixture (lines 336-388), leaving the Perpetual variant unpinned. The related TokenDistributionTypeWithResolvedRecipient enum also has JsonConvertible / ValueConvertible impls but no literal wire-shape tests. Wire-shape pinning is the central guarantee this PR introduces, so the variant matrix should be complete: add JSON and Value fixture assertions for Perpetual and for each variant of TokenDistributionTypeWithResolvedRecipient so a future tag, field, or payload-shape change fails loudly.

suggestion: ArrayItemType conversion tests miss Boolean and Date variants

packages/rs-dpp/src/data_contract/document_type/property/array.rs:706-842

ArrayItemType has seven variants but the new canonical conversion tests only pin Integer, Number, String, ByteArray, and Identifier. Boolean and Date are uncovered, even though Date has dedicated conversion behavior in this file. Add JSON and Value wire-shape assertions for both so the variant matrix introduced by this PR is complete.

suggestion: BLS public-key `visit_seq` collects unbounded input before rejecting it

packages/rs-dpp/src/serialization/dashcore/bls_pubkey.rs:151-157

The new BLS public-key serde wrapper accepts byte sequences for JSON/Value round-trips, but visit_seq pushes every element into a Vec and only checks the exact 48-byte (COMPRESSED_G1_LEN) length inside from_compressed_g1_bytes(&bytes) after the sequence ends. A malformed payload with a very large array in a public-key slot forces unnecessary allocation and parsing before the deserializer rejects it. This is currently reached only from trusted Dash Core validator data, but the with = "..." pattern is designed for reuse and the failure mode amplifies as soon as another type reuses this module on a less trusted ingest path. Cap the loop at COMPRESSED_G1_LEN + 1 and error on overflow so the structural promise ("48-byte key") is enforced inside the visitor.

    fn visit_seq<A: serde::de::SeqAccess<'de>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
        let mut bytes = Vec::with_capacity(COMPRESSED_G1_LEN);
        while let Some(b) = seq.next_element::<u8>()? {
            if bytes.len() >= COMPRESSED_G1_LEN {
                return Err(serde::de::Error::custom(format!(
                    "expected {COMPRESSED_G1_LEN} compressed-G1 bytes for public key, got more"
                )));
            }
            bytes.push(b);
        }
        from_compressed_g1_bytes(&bytes)
    }

Dropped / false-positive observations

• DataContract serde deserialization validates despite the canonical no-validation contract (codex-general) — False positive. The latest commit (6e5d568 'fix(dpp)!: restore validate-by-default DataContract deserialize + from_json(bool)') is an intentional reversal — Deserialize validates by default and the explicit opt-out is from_json(_, false, _) / from_value(_, false, _). A pin test in packages/rs-dpp/src/data_contract/conversion/serde/mod.rs enforces the round-trip behavior. The PR description is stale relative to the shipped semantics, but the code is consistent with itself and with the new opt-out API. The claim that this breaks rs-scripts/src/bin/register_contract.rs is not load-bearing for the PR's stated goal, and the safer default for an SDK trust boundary is to validate. Independent security review (claude-security-auditor) reached the same conclusion.
• wasm-dpp2 fromJSON/fromObject expose full_validation: bool to JS callers with no safer-default helper (claude-security-auditor) — Defense-in-depth ergonomic suggestion only, not a defect. SDK-side validation is not trusted by Drive in any case. Treating it as a hard blocker would require a parallel API redesign on the JS surface that is outside this PR's scope.
• Pre-existing handle leak in dash_sdk_data_contract_fetch_with_serialization JSON error paths (claude-ffi-engineer) — Pre-existing on origin/v3.1-dev and not introduced or worsened by this PR. The PR only changed which JSON-conversion call sits inside the same error-return structure. Belongs in a separate FFI cleanup PR.
• Legacy wasm-dpp toJSON/toObject switched from PlatformVersion::first() to global current (claude-ffi-engineer) — Behavioral change is acknowledged by the PR's scope (canonical serde routes via PlatformVersion::get_version_or_current_or_latest) and the legacy wasm-dpp crate is documented as slated for deletion. Not gating; if there is a concrete JS consumer hitting a V0-vs-V1 shape regression, a focused follow-up makes more sense than backing out the unification here.
• Surplus address proof result omits JS-safe credit serialization in VerifiedAssetLockConsumedWithAddressInfos (codex-security-auditor) — Same root cause as the TokenPricingSchedule JS-safe-integer finding already kept (defense in depth across u64 financial fields). Folding it into one issue under the safe-integer sweep keeps the public review focused; tracking via the kept finding rather than as a separate suggestion.
• Audit remaining serde callsites that previously assumed no-validation Deserialize (claude-security-auditor) — Workspace-wide audit follow-up unrelated to PR-introduced behavior — out of scope for this dispatcher and not concrete enough to track here.

🤖 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 `docs/v12-upgrade-boundary-risks.md:119-135`: Committed local runbook exposes live validator infrastructure details
  This runbook section is explicitly labeled `local only — contains infra details, do not commit`, yet it has been checked into the PR. It publishes a live testnet HPMN validator hostname and IP (`hp-masternode-8` = `68.67.122.8`), a second mainnet-synced host IP (`54.185.219.212`), the SSH user (`ubuntu`), the operator key path (`~/.ssh/evo-app-deploy.rsa`), and the concrete `ssh`/`scp` commands used to reach them. That is operator-sensitive reconnaissance material that should not ship in a public branch, and it is unrelated to the JSON/Value conversion work this PR is about. Remove the entire `## Runbook (local only ...)` section before merge and treat the already-pushed host/key details as exposed (rotate the deploy key, audit access on the named hosts).
- [BLOCKING] In `packages/wasm-dpp2/src/asset_lock_proof/proof.rs:18-38`: AssetLockProof TypeScript declarations advertise `type` while rs-dpp uses `$type`
  The exported TypeScript custom section tells JS callers to construct and inspect `AssetLockProofObject` / `AssetLockProofJSON` with a plain `type` discriminator, and the doc comment claims rs-dpp uses `#[serde(tag = "type")]`. The actual rs-dpp enum at `packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs:38,49` uses `#[serde(tag = "$type", rename_all = "camelCase")]`. Callers following these public typings will construct objects the Rust side rejects on `from_object` / `fromJSON` and will branch on a runtime field that does not exist. Align the TS declarations and surrounding comments with the executable `$type` wire shape.
- [BLOCKING] In `packages/wasm-dpp2/src/platform_address/fee_strategy.rs:8-31`: Additional wasm-dpp2 TypeScript surfaces still use `type` instead of `$type`
  Same discriminator mismatch beyond AssetLockProof. `FeeStrategyStepObject` and `FeeStrategyStepJSON` declare `{ type: "deductFromInput" | "reduceOutput"; index }`, but the runtime serde in `packages/rs-dpp/src/address_funds/fee_strategy/mod.rs` emits and requires `$type` (verified at lines 42, 46, 99, with round-trip tests at 131, 141, 148, 153, 206, 217, 237, 250). The same stale plain-`type` pattern remains in the wasm-dpp2 TS custom sections / examples for `VotePoll`, `ResourceVoteChoice`, `ContestedDocumentVotePollWinnerInfo`, `AddressWitness`, and `TokenEvent`. These are public JS contracts for the new canonical conversion surface — they must match the runtime objects rs-dpp emits and accepts. Sweep every `wasm-dpp2` typescript_custom_section and example for `type: "..."` and rewrite to `$type: "..."`.
- [SUGGESTION] In `packages/rs-sdk-ffi/src/data_contract/queries/fetch_with_serialization.rs:110-131`: C/Swift data-contract JSON no longer pins the SDK protocol version
  This PR changed the FFI JSON path from `contract.to_json(wrapper.sdk.version())` to `serde_json::to_value(&contract)` (line 111). `DataContract`'s manual `Serialize` impl in `packages/rs-dpp/src/data_contract/conversion/serde/mod.rs:48-60` resolves the version via `PlatformVersion::get_version_or_current_or_latest(None)`, which reads process-global state and falls back to the latest known platform version. The JSON now crossing the C ABI to Swift is therefore governed by global state instead of the version pinned on the SDK handle the caller passed in (`wrapper.sdk`). In the same FFI result the bincode `serialized_data` is still produced through SDK-version-aware paths, so a divergence between `json_string` and `serialized_data` for the same contract is reachable. The same regression is in `packages/rs-sdk-ffi/src/data_contract/queries/fetch_json.rs:54`. Build `DataContractInSerializationFormat` with `wrapper.sdk.version()` and serialize that explicitly across the FFI boundary.
- [SUGGESTION] In `packages/wasm-dpp/src/data_contract/state_transition/data_contract_update_transition/mod.rs:52-79`: Lenient update-transition constructor no longer accepts the legacy `identityContractNonce` field
  The constructor now pre-fills `$formatVersion`, `userFeeIncrease`, `signaturePublicKeyId`, and `signature` before strict `DataContractUpdateTransition::from_object(raw)` (lines 71-76), but it does not alias the previous JS public field `identityContractNonce` to the canonical serde field `$identity-contract-nonce`. This PR also rewrites the existing JS fixture from `identityContractNonce` to `$identity-contract-nonce`, so existing callers built against the old public constructor shape will fail with a missing-field error from the strict deserializer. Either alias the legacy key into the canonical key inside this `Value::Map` normalization step (keep requiring a real nonce — do not default it to zero), or document the field rename as a public migration in the PR.
- [SUGGESTION] In `packages/rs-dpp/src/tokens/token_pricing_schedule.rs:28-51`: TokenPricingSchedule JSON emits u64 prices as unsafe JS numbers
  `TokenPricingSchedule` derives plain serde and its `JsonConvertible` / `ValueConvertible` impls (lines 48, 51) do not use `json_safe_fields` or an equivalent large-integer wrapper. As a result `SinglePrice(Credits)` and `SetPrices(BTreeMap<TokenAmount, Credits>)` serialize u64 credit and token amounts as raw JSON numbers. Values above `Number.MAX_SAFE_INTEGER` (2^53) will be silently rounded by JavaScript wasm/SDK callers that go through `toJSON()` — a financial-amount truncation in the same conversion sweep this PR introduces. Apply the safe-integer JSON convention used elsewhere in the PR to both variants.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs:322-389`: TokenDistributionInfo conversion tests pin only the PreProgrammed variant
  The canonical conversion tests for `TokenDistributionInfo` use only a `PreProgrammed` fixture (lines 336-388), leaving the `Perpetual` variant unpinned. The related `TokenDistributionTypeWithResolvedRecipient` enum also has `JsonConvertible` / `ValueConvertible` impls but no literal wire-shape tests. Wire-shape pinning is the central guarantee this PR introduces, so the variant matrix should be complete: add JSON and Value fixture assertions for `Perpetual` and for each variant of `TokenDistributionTypeWithResolvedRecipient` so a future tag, field, or payload-shape change fails loudly.
- [SUGGESTION] In `packages/rs-dpp/src/data_contract/document_type/property/array.rs:706-842`: ArrayItemType conversion tests miss Boolean and Date variants
  `ArrayItemType` has seven variants but the new canonical conversion tests only pin Integer, Number, String, ByteArray, and Identifier. Boolean and Date are uncovered, even though `Date` has dedicated conversion behavior in this file. Add JSON and Value wire-shape assertions for both so the variant matrix introduced by this PR is complete.
- [SUGGESTION] In `packages/rs-dpp/src/serialization/dashcore/bls_pubkey.rs:151-157`: BLS public-key `visit_seq` collects unbounded input before rejecting it
  The new BLS public-key serde wrapper accepts byte sequences for JSON/Value round-trips, but `visit_seq` pushes every element into a `Vec` and only checks the exact 48-byte (`COMPRESSED_G1_LEN`) length inside `from_compressed_g1_bytes(&bytes)` after the sequence ends. A malformed payload with a very large array in a public-key slot forces unnecessary allocation and parsing before the deserializer rejects it. This is currently reached only from trusted Dash Core validator data, but the `with = "..."` pattern is designed for reuse and the failure mode amplifies as soon as another type reuses this module on a less trusted ingest path. Cap the loop at `COMPRESSED_G1_LEN + 1` and error on overflow so the structural promise ("48-byte key") is enforced inside the visitor.