feat(wasm-sdk)!: add shielded pool WASM bindings and query methods

[QuantumExplorer]

Issue being fixed or feature implemented

Adds the JS/WASM layer for shielded-pool functionality across wasm-dpp2, wasm-sdk, and js-evo-sdk.

The PR is scoped to read-only queries + wire-format wrappers. High-level shielded transition methods in wasm-sdk (build → prove → sign → broadcast) are deferred to a follow-up: they require enabling the shielded-client feature, which drags the Halo2 prover (~10MB+) into the bundle and needs a separate browser-vs-server proving design.

What was done?

wasm-dpp2: Shielded state transition wrappers

• ShieldTransitionWasm — shield (deposit) with inputs, actions, amount, proof, binding signature
• ShieldFromAssetLockTransitionWasm — shield from asset lock with proof verification
• ShieldedTransferTransitionWasm — private transfer between shielded addresses
• UnshieldTransitionWasm — withdraw from shielded pool to transparent address
• ShieldedWithdrawalTransitionWasm — withdraw from shielded pool to core chain
• computePlatformSighash utility function exposed to JS
• All wrappers include toBytes/fromBytes/toJSON/toObject/toStateTransition/getModifiedDataIds
• Replaced todo!() panics in StateTransitionWasm for shielded variants with proper handling

wasm-sdk: Shielded pool query methods

• getShieldedPoolState — total shielded pool balance (BigInt)
• getShieldedEncryptedNotes(startIndex, count) — paginated encrypted notes
• getShieldedAnchors — valid anchors for Orchard spend proofs
• getMostRecentShieldedAnchor — latest 32-byte anchor
• getShieldedNullifiers(nullifiers) — check nullifier spent/unspent status
• All 5 queries have WithProofInfo variants returning ProofMetadataResponse<T>
• ShieldedEncryptedNoteWasm and ShieldedNullifierStatusWasm wrapper types with TypeScript interfaces

js-evo-sdk: ShieldedFacade

Read-only access to the shielded pool exposed at sdk.shielded.*, mirroring the existing TokensFacade pattern:

• poolState() / poolStateWithProof()
• encryptedNotes(startIndex, count) / encryptedNotesWithProof(...)
• anchors() / anchorsWithProof()
• mostRecentAnchor() / mostRecentAnchorWithProof()
• nullifiers(nullifiers) / nullifiersWithProof(...)

Building / signing / broadcasting shielded transitions is intentionally out of scope here — that requires the Halo2 prover and is tracked as a follow-up.

Additional fixes during review

• Map-loss toJSON bug in 8 verified-result wrappers (VerifiedShieldedNullifiers*, VerifiedTokenIdentitiesBalances, VerifiedDocuments, VerifiedAddressInfos, VerifiedIdentityFullWithAddressInfos, VerifiedIdentityWithAddressInfos) — js_sys::Map has no enumerable own properties, so JSON.stringify was silently dropping every entry. Now routed through normalize_js_value_for_json.
• pooling_serde HR-aware helper in rs-dpp — emits "never"/"ifAvailable"/"standard" in JSON (was numeric 0/1/2), u8 in non-HR; deserialize accepts both for backwards compat. Applied to all 4 transitions surfacing Pooling.
• address_funds::serde_helpers — field-level helpers reshaping BTreeMap<PlatformAddress, …> and Option<(PlatformAddress, …)> into self-describing {address, nonce?, amount} arrays in JSON. Bincode unaffected, consensus byte-identical.
• AddressFundsFeeStrategyStep — custom Serialize/Deserialize to emit {type, index} shape.
• Symmetric Object/JSON TS interfaces for PlatformAddressInput, PlatformAddressOutput, FeeStrategyStep, AddressWitness.
• Thin-wrapper principle codified in wasm-dpp2/CONVENTIONS.md — wasm-dpp2 doesn't duplicate DPP validation.
• Unified serde_bytes helper (const-generic for fixed-size byte arrays).
• Removed wasm-side size caps that duplicated DPP boundary validation.

How Has This Been Tested?

• cargo check / cargo clippy / cargo fmt — green workspace-wide
• wasm-dpp2 JS suite — 1120 tests passing (+11 new toJSON regression tests)
• wasm-sdk JS unit suite — 394 tests passing (+9 new shielded type tests)
• wasm-sdk JS functional suite — local dashmate node (+12 new shielded query tests)
• js-evo-sdk JS unit suite — 191 tests passing (+12 new ShieldedFacade tests)
• CI green on all checks for the latest push

Breaking Changes

JSON wire shape only — no consensus / binary impact.

AssetLockProof JSON serialisation changed from #[serde(untagged)] to #[serde(tag = "type", rename_all = "camelCase")]. JSON consumers now see a type: "instant" | "chain" discriminator alongside the inner fields. Bincode Encode/Decode derives are independent of serde, so PlatformSerializable / PlatformSignable sighashes and on-wire transitions are byte-identical.

Affected JSON-form consumers: IdentityCreateTransition.assetLockProof, IdentityTopUpTransition.assetLockProof, AddressFundingFromAssetLockTransitionV0.asset_lock_proof, ShieldFromAssetLockTransitionV0.asset_lock_proof. Most JSON parsers ignore the extra type key automatically; only consumers that hand-construct JSON to feed back in need to add it.

Rationale: aligns with the convention used by AddressWitness, AddressFundsFeeStrategyStep, and the new shielded transition enums in this PR; gives JS consumers an unambiguous discriminator instead of relying on inner-field shape inspection.

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional tests
• I have made corresponding changes to the documentation
• I have added the PR to a milestone (if applicable)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Full JS/WASM support for shielded state transitions (shield, unshield, shielded transfer, shielded withdrawal) and new shielded proof-result types.
  • Shielded pool queries in the SDK (pool state, encrypted notes, anchors, nullifier status) including proof-info variants.
  • Improved JSON/byte serialization helpers for address-keyed maps and fixed/variable byte fields.

• Bug Fixes

  • Clearer, non-panicking error handling for operations involving shielded transitions.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a new shielded WASM module and SDK query surface, five shielded state-transition WASM wrappers, shielded proof-result types, multiple serde/json-wire helpers and byte serializers, and applies json-conversion/serde-conversion feature-gated adjustments across shielded and address-funds types.

Changes

Cohort / File(s)	Summary
WASM entry & shielded module packages/wasm-dpp2/src/lib.rs, packages/wasm-dpp2/src/shielded/mod.rs	Added pub mod shielded, re-exported shielded WASM types, and added compute_platform_sighash_wasm with input validation.
Shielded transition WASM wrappers packages/wasm-dpp2/src/shielded/shield_from_asset_lock_transition.rs, .../shield_transition.rs, .../shielded_transfer_transition.rs, .../shielded_withdrawal_transition.rs, .../unshield_transition.rs	Introduced five wasm_bindgen wrappers exposing getters, toBytes/fromBytes/toObject/toJson, conversions to StateTransitionWasm, and wasm type info/From impls.
Proof result: shielded extraction packages/wasm-dpp2/src/state_transitions/proof_result.rs, packages/wasm-dpp2/src/state_transitions/proof_result_shielded.rs	Moved shielded proof-result types into new module, added VerifiedAssetLockConsumed/VerifiedShieldedNullifiers variants, nullifier/doc map builders, and updated convert_proof_result to use new types.
StateTransition base behavior packages/wasm-dpp2/src/state_transitions/base/state_transition.rs	Removed panic/todo branches for shielded transitions: owner/identity getters return None and setters return invalid_argument errors for shielded variants.
WASM SDK queries: shielded packages/wasm-sdk/src/lib.rs, packages/wasm-sdk/src/queries/mod.rs, packages/wasm-sdk/src/queries/shielded.rs	Added queries::shielded module, two shielded query types re-exported, and multiple async WasmSdk methods for shielded pool state, anchors, encrypted notes, nullifier status and proof-info variants.
Serialization: generic byte helpers & json-safe packages/rs-dpp/src/serialization/mod.rs, .../serde_bytes.rs, .../serde_bytes_var.rs, packages/rs-dpp-json-convertible-derive/src/lib.rs, packages/rs-dpp/src/serialization/json/safe_fields.rs	Replaced fixed 64-byte helper with generic [u8; N] and Vec<u8] serde adapters, exposed serde_bytes/serde_bytes_var, and extended json_safe_fields to inject byte helpers and mark additional types as JSON-safe.
Address-funds serde helpers packages/rs-dpp/src/address_funds/serde_helpers/*.rs, packages/rs-dpp/src/address_funds/mod.rs	Added multiple serde with helpers for address-keyed maps and singular outputs (serialize/deserialize helpers) and feature-gated export for serde_helpers.
Apply json helpers to transitions packages/rs-dpp/src/state_transition/.../v0/*.rs (address_funding, address_funds_transfer, address_credit_withdrawal, identity topup/credit transfer, multiple shielded v0 files)	Applied cfg_attr(feature="json-conversion", ...) serde adapters and json_safe_fields to various transition structs/fields to change JSON wire shapes when enabled; removed per-field serde_bytes_64 in favor of generic helpers.
Shielded type tagging & derives packages/rs-dpp/src/state_transition/state_transitions/shielded/.../mod.rs and .../v0/mod.rs (shield_from_asset_lock, shield, shielded_transfer, shielded_withdrawal, unshield)	Changed serde tag key from "$version" → "$formatVersion" under serde-conversion, and added conditional JsonConvertible/ValueConvertible derives and imports.
AddressFunds serde/fee-strategy/platform address packages/rs-dpp/src/address_funds/fee_strategy/mod.rs, packages/rs-dpp/src/address_funds/platform_address.rs	Replaced derive-based serde for fee-step and platform address with explicit Serialize/Deserialize under serde-conversion, changed address wire form and added stricter validation; added feature-gated helpers and tests.
WASM proof-result wiring & SDK exports packages/wasm-sdk/src/lib.rs, .gitignore	Re-exported shielded query types from Wasm SDK and added .serena to .gitignore.

Sequence Diagram(s)

sequenceDiagram
    actor JS as JavaScript Client
    participant SDK as WASM SDK
    participant WASM as wasm-dpp2
    participant DPP as DPP Core
    participant Verifier as Proof Verifier

    JS->>SDK: getShieldedNullifiersWithProofInfo(nullifiers)
    SDK->>WASM: request nullifier statuses + proof metadata
    WASM->>DPP: fetch statuses + proof payload
    DPP-->>WASM: statuses + proof payload
    WASM->>Verifier: verify proof
    Verifier-->>WASM: verification result
    WASM->>SDK: assemble ProofMetadataResponseWasm (nullifier_map)
    SDK-->>JS: return ProofMetadataResponseWasm

Loading

sequenceDiagram
    actor JS as JavaScript Client
    participant WASM as wasm-dpp2
    participant DPP as DPP Core
    participant Crypto as Crypto Libs

    JS->>WASM: create ShieldTransitionWasm / call toBytes
    WASM->>DPP: construct ShieldTransition::V0
    WASM->>Crypto: validate proofs/signatures
    Crypto-->>WASM: validation outcome
    WASM->>DPP: PlatformSerializable -> bytes
    WASM-->>JS: return bytes / StateTransitionWasm

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 I hopped through bindings, bytes, and proofs tonight,
Wrapped shielded transitions so JS can see the light,
Nullifiers, anchors, maps all tucked in rows,
Wasm bridges secrets where the quiet river flows,
A rabbit’s soft cheer for private-data flight.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and specifically identifies the primary change: adding WASM bindings and query methods for shielded pool functionality to wasm-sdk.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/zk-wasm-js-bindings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (3)

packages/wasm-dpp2/src/shielded/mod.rs (1)

28-33: Consider consistent validation style.

The length validation and conversion on lines 28-34 is correct, but expect("checked length above") could be replaced with unwrap() for consistency, since the length is already validated. This is a minor style preference.

♻️ Optional: use unwrap() after validation

     if bundle_commitment.len() != 32 {
         return Err(crate::error::WasmDppError::invalid_argument(&format!(
             "bundleCommitment must be exactly 32 bytes, got {}",
             bundle_commitment.len()
         )));
     }
-    let commitment: &[u8; 32] = bundle_commitment.try_into().expect("checked length above");
+    let commitment: &[u8; 32] = bundle_commitment.try_into().unwrap();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/wasm-dpp2/src/shielded/mod.rs` around lines 28 - 33, Replace the
post-validation call that uses expect("checked length above") with unwrap() for
stylistic consistency: after you validate bundle_commitment.len() != 32 and
return an error, use unwrap() when converting/creating the fixed-size array (the
same spot currently calling expect) so the code relies on the prior check and
matches the project's validation style (refer to the bundle_commitment length
check and the conversion site where expect("checked length above") is used).

packages/wasm-dpp2/src/shielded/shielded_transfer_transition.rs (1)

88-116: Add a round-trip test for this wasm-facing ABI.

toBytes/fromBytes plus the JS-visible getters are new public surface, and this PR explicitly ships without test updates. A small round-trip/golden test here would catch wire-format drift and wrong-variant regressions before they reach the SDK.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/wasm-dpp2/src/shielded/shielded_transfer_transition.rs` around lines
88 - 116, Add a round-trip unit test exercising the wasm ABI: construct a
ShieldedTransferTransitionWasm instance, call to_bytes(), pass the bytes into
from_bytes() and assert the resulting ShieldedTransferTransitionWasm equals the
original; also include a JSON round-trip by calling to_json() and verifying the
serialized value round-trips (or matches expected golden JSON), and verify
to_state_transition() produces the expected StateTransition variant. Target the
wasm-facing symbols to_bytes, from_bytes, to_json, to_state_transition and
assert equality of the core inner StateTransition::ShieldedTransfer data to
catch wire-format or variant regressions.

packages/js-dash-sdk/src/SDK/Client/Platform/Platform.ts (1)

202-208: Consider a shared helper for the shielded broadcast path.

These five bindings fan out to near-identical modules today. Once you add the missing decoded-type guard or any future validation/logging tweak, you'll need to keep five copies in sync. A small helper that takes { expectedType, logPrefix } would make this surface much harder to drift.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/js-dash-sdk/src/SDK/Client/Platform/Platform.ts` around lines 202 -
208, The five near-identical bindings under this.shielded (shield,
shieldedTransfer, unshield, shieldFromAssetLock, shieldedWithdrawal) duplicate
broadcast/validation logic; extract a small helper (e.g.,
createShieldedBinder(expectedType, logPrefix)) that returns a bound function
given a method reference and metadata, then replace the direct .bind(this) calls
with calls to that helper (pass the original method function like shieldMethod,
shieldedTransferMethod, unshieldMethod, shieldFromAssetLockMethod,
shieldedWithdrawalMethod and the appropriate expectedType/logPrefix) so shared
decoded-type guards, validation and logging live in one place.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/js-dash-sdk/src/SDK/Client/Platform/methods/shielded/shield.ts`:
- Around line 24-31: After deserializing the transition with
dpp.stateTransition.createFromBuffer in shield(), add a type guard that checks
transition.getType() and ensure it matches the expected StateTransition type for
shield (e.g., ShieldedTransfer/Shield/ShieldFromAssetLock/ShieldedWithdrawal as
appropriate) before calling broadcastStateTransition(this, await transition,
...); if the type does not match, throw or return a clear error. Apply the same
guard/check (use .getType() on the result of
dpp.stateTransition.createFromBuffer) to all other shielded entrypoints so each
only accepts its intended StateTransition variant prior to calling
broadcastStateTransition.

In `@packages/js-dash-sdk/src/SDK/Client/Platform/Platform.ts`:
- Around line 37-41: The three new shielded module filenames use mixed/camel
casing; rename the files shieldFromAssetLock.ts, shieldedTransfer.ts, and
shieldedWithdrawal.ts to kebab-case (shield-from-asset-lock.ts,
shielded-transfer.ts, shielded-withdrawal.ts) and update all imports/exports
referencing them (e.g., the imports in Platform.ts: shieldFromAssetLockMethod,
shieldedTransferMethod, shieldedWithdrawalMethod) to use the new kebab-case
paths; also update any barrel/index exports and tests or references across the
repo to the new filenames so imports remain consistent.

In `@packages/wasm-dpp/src/state_transition/state_transition_factory.rs`:
- Around line 82-96: The Shield-related state transition variants
(StateTransition::Shield, ::ShieldedTransfer, ::Unshield, ::ShieldFromAssetLock,
::ShieldedWithdrawal) call serde_wasm_bindgen::to_value(&st) but fail to compile
because the Serialize impls are behind the rs-dpp "serde-conversion" feature;
fix this by enabling that feature for the dpp dependency in wasm-dpp's
Cargo.toml (add "serde-conversion" to the features list for dpp) so those types
derive Serialize and serde_wasm_bindgen::to_value(&st) will compile.

In `@packages/wasm-dpp2/src/shielded/mod.rs`:
- Around line 16-36: The doc comment for compute_platform_sighash_wasm
incorrectly states that extraData includes an amount for unshield and shielded
withdrawal transitions; update the comment to match the rs-dpp implementation by
removing references to amount and describing extraData as just the output
address bytes for unshield transitions and the output script bytes for shielded
withdrawal transitions (see the extra_sighash_data construction in unshield.rs
and shielded_withdrawal.rs for reference).

---

Nitpick comments:
In `@packages/js-dash-sdk/src/SDK/Client/Platform/Platform.ts`:
- Around line 202-208: The five near-identical bindings under this.shielded
(shield, shieldedTransfer, unshield, shieldFromAssetLock, shieldedWithdrawal)
duplicate broadcast/validation logic; extract a small helper (e.g.,
createShieldedBinder(expectedType, logPrefix)) that returns a bound function
given a method reference and metadata, then replace the direct .bind(this) calls
with calls to that helper (pass the original method function like shieldMethod,
shieldedTransferMethod, unshieldMethod, shieldFromAssetLockMethod,
shieldedWithdrawalMethod and the appropriate expectedType/logPrefix) so shared
decoded-type guards, validation and logging live in one place.

In `@packages/wasm-dpp2/src/shielded/mod.rs`:
- Around line 28-33: Replace the post-validation call that uses expect("checked
length above") with unwrap() for stylistic consistency: after you validate
bundle_commitment.len() != 32 and return an error, use unwrap() when
converting/creating the fixed-size array (the same spot currently calling
expect) so the code relies on the prior check and matches the project's
validation style (refer to the bundle_commitment length check and the conversion
site where expect("checked length above") is used).

In `@packages/wasm-dpp2/src/shielded/shielded_transfer_transition.rs`:
- Around line 88-116: Add a round-trip unit test exercising the wasm ABI:
construct a ShieldedTransferTransitionWasm instance, call to_bytes(), pass the
bytes into from_bytes() and assert the resulting ShieldedTransferTransitionWasm
equals the original; also include a JSON round-trip by calling to_json() and
verifying the serialized value round-trips (or matches expected golden JSON),
and verify to_state_transition() produces the expected StateTransition variant.
Target the wasm-facing symbols to_bytes, from_bytes, to_json,
to_state_transition and assert equality of the core inner
StateTransition::ShieldedTransfer data to catch wire-format or variant
regressions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2bdbcf86-45ca-47c6-ba00-fc5f4eeab635

📥 Commits

Reviewing files that changed from the base of the PR and between 037c387 and c5325c9.

📒 Files selected for processing (16)

• packages/js-dash-sdk/src/SDK/Client/Platform/Platform.ts
• packages/js-dash-sdk/src/SDK/Client/Platform/methods/shielded/shield.ts
• packages/js-dash-sdk/src/SDK/Client/Platform/methods/shielded/shieldFromAssetLock.ts
• packages/js-dash-sdk/src/SDK/Client/Platform/methods/shielded/shieldedTransfer.ts
• packages/js-dash-sdk/src/SDK/Client/Platform/methods/shielded/shieldedWithdrawal.ts
• packages/js-dash-sdk/src/SDK/Client/Platform/methods/shielded/unshield.ts
• packages/wasm-dpp/src/identity/state_transition/transition_types.rs
• packages/wasm-dpp/src/state_transition/state_transition_factory.rs
• packages/wasm-dpp2/src/lib.rs
• packages/wasm-dpp2/src/shielded/mod.rs
• packages/wasm-dpp2/src/shielded/shield_from_asset_lock_transition.rs
• packages/wasm-dpp2/src/shielded/shield_transition.rs
• packages/wasm-dpp2/src/shielded/shielded_transfer_transition.rs
• packages/wasm-dpp2/src/shielded/shielded_withdrawal_transition.rs
• packages/wasm-dpp2/src/shielded/unshield_transition.rs
• packages/wasm-dpp2/src/state_transitions/base/state_transition.rs

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/wasm-sdk/src/queries/shielded.rs`:
- Around line 175-185: The current nullifier_arrays mapping in functions using
`nullifiers.iter()` and `Uint8Array::new(&n)` (the `nullifier_arrays`
construction and the similar block at the second occurrence) silently truncates
or zero-pads inputs; change this to validate that `Uint8Array::length()` (or the
vector length produced by `to_vec()`) is exactly 32 and return an error
(propagate an Err/JsValue back to the caller) when length != 32 instead of using
`len().min(32)` and copying/padding; update both the `nullifier_arrays` mapping
and the analogous block at the other occurrence so invalid-length inputs are
rejected with a clear error message identifying the offending nullifier.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 482ab228-743d-48c4-a268-2e18dfc07964

📥 Commits

Reviewing files that changed from the base of the PR and between c5325c9 and 5d4ee20.

📒 Files selected for processing (8)

• packages/wasm-dpp2/src/shielded/mod.rs
• packages/wasm-dpp2/src/shielded/shield_from_asset_lock_transition.rs
• packages/wasm-dpp2/src/state_transitions/mod.rs
• packages/wasm-dpp2/src/state_transitions/proof_result.rs
• packages/wasm-dpp2/src/state_transitions/proof_result_shielded.rs
• packages/wasm-sdk/src/lib.rs
• packages/wasm-sdk/src/queries/mod.rs
• packages/wasm-sdk/src/queries/shielded.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• packages/wasm-dpp2/src/shielded/shield_from_asset_lock_transition.rs
• packages/wasm-dpp2/src/shielded/mod.rs

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/wasm-sdk/src/queries/shielded.rs`:
- Around line 175-190: The code uses Uint8Array::new(&n) which silently coerces
numeric JS values into zero-filled arrays; update the nullifier parsing (the
nullifiers -> nullifier_arrays mapping where Uint8Array::new(&n) is called) to
first dyn_into::<js_sys::Uint8Array>() the JsValue `n` and return a
WasmSdkError::invalid_argument if the cast fails, then call to_vec(), check
length == 32, copy into [u8;32], and propagate errors; apply the same
dyn_into-based validation to the second nullifier-parsing block referenced in
the review so both locations reject non-Uint8Array inputs instead of coercing
numbers.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a0485101-3553-4505-8ff4-25940161772c

📥 Commits

Reviewing files that changed from the base of the PR and between 5d4ee20 and a87c6ad.

📒 Files selected for processing (1)

• packages/wasm-sdk/src/queries/shielded.rs

[thepastaclaw]

Code Review

This PR adds WASM/JS bindings for shielded pool state transitions and query methods. The code follows established wrapper patterns and is structurally sound. The main concern is an inconsistency where getShieldedPoolState silently maps a missing pool to BigInt(0) while its WithProofInfo variant correctly returns null, which would cause different observable behavior depending on which API a consumer uses. Test coverage for this security-critical surface is also absent.

🔴 1 blocking | 🟡 3 suggestion(s)

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

In `packages/wasm-sdk/src/queries/shielded.rs`:
- [BLOCKING] lines 99-100: Inconsistent None-handling: getShieldedPoolState returns 0 but WithProofInfo variant returns null
  `get_shielded_pool_state` coerces `None` to `BigInt(0)` via `unwrap_or(0)` (line 99), while `get_shielded_pool_state_with_proof_info` (line 222) maps `None` to `JsValue::NULL`. This means the non-proof variant silently conflates "pool doesn't exist" with "pool has zero balance", while the proof variant correctly distinguishes the two. A JS consumer switching between APIs would observe different semantics for the same underlying state. The `WithProofInfo` behavior (returning null) is correct — the non-proof variant should match.
- [SUGGESTION] lines 1-362: No tests for any of the new shielded query methods or transition wrappers
  This PR adds 5 state transition wrapper types, 1 utility function (`computePlatformSighash`), 5 query methods, 5 `WithProofInfo` query variants, and 4 new proof result types — all without corresponding tests. The inconsistency flagged above (None → 0 vs None → null) is exactly the kind of behavioral bug that a test comparing both API variants would catch. Given that this is security-critical shielded pool functionality, at minimum serialization roundtrip tests for the transition wrappers and mock-based tests for the query methods would provide important coverage.
- [SUGGESTION] lines 175-189: Duplicated nullifier parsing and validation logic
  The nullifier parsing block (convert JS `Array<Uint8Array>` to `Vec<[u8; 32]>` with 32-byte validation) is copy-pasted identically between `get_shielded_nullifiers` (lines 175–189) and `get_shielded_nullifiers_with_proof_info` (lines 327–341). Extract to a shared helper to avoid the two copies drifting apart.

Additionally, `Uint8Array::new(&n)` doesn't type-check its argument — if a non-Uint8Array value is passed, it may silently produce a zero-length array rather than erroring. Using `n.dyn_into::<Uint8Array>()` would provide a clearer error message.

In `packages/wasm-dpp2/src/shielded/shield_transition.rs`:
- [SUGGESTION] lines 1-156: Four shielded transition wrappers lack JS constructors (intentional?)
  `ShieldFromAssetLockTransitionWasm` has a `#[wasm_bindgen(constructor)]` plus `impl_wasm_conversions_serde!` for `fromObject`/`fromJSON`, but `ShieldTransitionWasm`, `ShieldedTransferTransitionWasm`, `UnshieldTransitionWasm`, and `ShieldedWithdrawalTransitionWasm` can only be constructed via `fromBytes()`. This is likely intentional since ZK-proof-based transitions are built in Rust (where the proof is generated), not from plain JS objects. If so, a brief doc comment noting that `fromBytes` is the intended construction path would prevent confusion. If JS construction is eventually needed, the `Serialize`/`Deserialize` derives are missing from these four types (they'd need to be added for serde-based constructors to work).

[codecov]

Codecov Report

❌ Patch coverage is 81.00147% with 129 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.28%. Comparing base (c556a86) to head (607726e).
⚠️ Report is 16 commits behind head on v3.1-dev.

Files with missing lines	Patch %	Lines
...kages/rs-dpp/src/address_funds/platform_address.rs	41.07%	33 Missing ⚠️
packages/rs-dpp/src/withdrawal/mod.rs	75.71%	17 Missing ⚠️
packages/rs-dpp/src/serialization/serde_bytes.rs	79.22%	16 Missing ⚠️
packages/rs-dpp/src/identity/core_script.rs	25.00%	15 Missing ⚠️
...ackages/rs-platform-value/src/types/binary_data.rs	70.58%	15 Missing ⚠️
...ckages/rs-dpp/src/serialization/serde_bytes_var.rs	74.50%	13 Missing ⚠️
...erde_helpers/address_output_map_required_amount.rs	87.71%	7 Missing ⚠️
...kages/rs-dpp/src/address_funds/fee_strategy/mod.rs	93.18%	6 Missing ⚠️
...c/address_funds/serde_helpers/address_input_map.rs	96.42%	3 Missing ⚠️
...erde_helpers/address_output_map_optional_amount.rs	95.00%	3 Missing ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff              @@
##           v3.1-dev    #3235      +/-   ##
============================================
- Coverage     88.29%   88.28%   -0.01%     
============================================
  Files          2474     2479       +5     
  Lines        300927   301529     +602     
============================================
+ Hits         265707   266217     +510     
- Misses        35220    35312      +92     

Components	Coverage Δ
dpp	87.95% <81.84%> (-0.02%)	⬇️
drive	87.35% <ø> (ø)
drive-abci	90.25% <ø> (ø)
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	92.17% <70.58%> (-0.10%)	⬇️
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	55.66% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[thepastaclaw]

Code Review

This incremental push addresses all four prior findings: None→undefined consistency, duplicated nullifier parsing, missing constructors, and macro usage. The changes are well-structured. Two remaining issues: the TypeScript interfaces declare $formatVersion but the Rust shielded enums use serde(tag = "$version"), creating a mismatch; and the FeeStrategyStepObject interface shape doesn't match the actual externally-tagged serde output.

Reviewed commit: 7f216ec

🔴 1 blocking | 🟡 2 suggestion(s)

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

In `packages/wasm-dpp2/src/shielded/shield_transition.rs`:
- [BLOCKING] line 68: TS interfaces declare `$formatVersion` but Rust enums serialize `$version`
  All five shielded transition Rust enums (`ShieldTransition`, `ShieldedTransferTransition`, etc.) use `serde(tag = "$version")`, while every other versioned type in the codebase uses `serde(tag = "$formatVersion")`. The new TypeScript interfaces all declare `$formatVersion: string`, which doesn't match the actual serialization output (`$version`). This means:
- Objects from `toObject()`/`toJSON()` will have a `$version` key, not `$formatVersion`
- JS consumers constructing objects per the interface will use `$formatVersion`, causing deserialization failures

Either rename the Rust tag to `$formatVersion` for consistency, or update all TS interfaces to use `$version`.
- [SUGGESTION] lines 15-18: `FeeStrategyStepObject` doesn't match actual serde shape
  `AddressFundsFeeStrategyStep` is an externally-tagged enum with `rename_all = "camelCase"`, so it serializes as `{ "deductFromInput": 0 }` or `{ "reduceOutput": 1 }`. The TS interface declares `{ type: string; index: number }`, which is a different shape entirely. JS consumers following the interface will construct objects the deserializer rejects.
- [SUGGESTION] line 69: `inputs` typed as `Map` but serialization emits a plain object
  The `toObject` path uses `serialize_maps_as_objects(true)`, so `inputs` (a Rust `BTreeMap`) is emitted as a plain JS object with string keys, not a JS `Map`. The Object interface should use `Record<string, [number, bigint]>` instead of `Map<object, [number, bigint]>`. The JSON interface already correctly uses `Record<string, [number, number]>`.

[thepastaclaw]

Code Review

Three blocking issues: AssetLockProof::try_from(Value) is no longer aligned with the new internally-tagged serde wire shape (breaks IdentityTopUp/Create from_object round-trip), and two TypeScript interface declarations (UnshieldTransitionObject.outputAddress, ShieldedWithdrawalTransitionJSON.pooling) advertise types that the actual serializers do not emit or accept. Beyond these, the new shielded WASM surface has several allocation-DoS gaps at the JS→WASM boundary (unbounded encryptedNote, unbounded Array<...> lengths, unbounded signature/redeemScript in AddressWitness), missing structural validation in ShieldTransition::new, toJSON() paths that leak js_sys::Map instances, and fromJSON/fromObject paths that bypass the constructor size caps. A handful of quality and breaking-change callouts round out the review.

Reviewed commit: 321b18c

🔴 3 blocking | 🟡 9 suggestion(s)

2 additional findings

🔴 blocking: `AssetLockProof::try_from(Value)` no longer accepts the wire shape produced by the new serde derive

packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs (lines 191-260)

This PR changed AssetLockProof to #[serde(tag = "type", rename_all = "camelCase")], so platform_value::to_value(asset_lock_proof) (and the parent transition's to_object) now produces maps shaped like {"type": "instant", ...flattened inner fields}. But both TryFrom<&Value> and TryFrom<Value> (lines 191-260) still only accept either a numeric type (get_optional_integer("type")) or the pre-serde capitalized wrapper forms ({"Instant": ...}/{"Chain": ...}). With the new wire shape, get_optional_integer is called on a Value::Text("instant") and errors with a value-type mismatch (Error::StructureError), which is propagated as ProtocolError::ValueError. As a result, IdentityTopUpTransitionV0::from_object (packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/v0/value_conversion.rs:35) and IdentityCreateTransitionV0::from_object (.../identity_create_transition/v0/value_conversion.rs:44) now reject the value form their own to_object produces — the canonical to_object/from_object round-trip for these transitions is broken. The handwritten Value decoder must be updated alongside the serde change to accept the new internally-tagged shape (string type, no inner wrapper key).

💡 Suggested change

impl TryFrom<&Value> for AssetLockProof {
    type Error = ProtocolError;

    fn try_from(value: &Value) -> Result<Self, Self::Error> {
        platform_value::from_value(value.clone()).or_else(|serde_err| {
            let proof_type_int: Option<u8> = value
                .get_optional_integer("type")
                .map_err(ProtocolError::ValueError)?;
            if let Some(proof_type_int) = proof_type_int {
                let proof_type = AssetLockProofType::try_from(proof_type_int)?;
                return match proof_type {
                    AssetLockProofType::Instant => Ok(Self::Instant(value.clone().try_into()?)),
                    AssetLockProofType::Chain => Ok(Self::Chain(value.clone().try_into()?)),
                };
            }

            let map = value.as_map().ok_or_else(|| ProtocolError::DecodingError(format!(
                "error decoding asset lock proof: {}", serde_err
            )))?;
            let (key, asset_lock_value) = map.first().ok_or_else(|| ProtocolError::DecodingError(format!(
                "error decoding asset lock proof as it was empty: {}", serde_err
            )))?;
            match key.as_str().ok_or_else(|| ProtocolError::DecodingError(format!(
                "error decoding asset lock proof: {}", serde_err
            )))? {
                "Instant" => Ok(Self::Instant(asset_lock_value.clone().try_into()?)),
                "Chain" => Ok(Self::Chain(asset_lock_value.clone().try_into()?)),
                _ => Err(ProtocolError::DecodingError(format!(
                    "error decoding asset lock proof: {}", serde_err
                ))),
            }
        })
    }
}

impl TryFrom<Value> for AssetLockProof {
    type Error = ProtocolError;

    fn try_from(value: Value) -> Result<Self, Self::Error> {
        Self::try_from(&value)
    }
}

🟡 suggestion: Address-based transition setters changed from infallible to `WasmDppResult<()>` — breaking JS API

packages/wasm-dpp2/src/platform_address/transitions/address_credit_withdrawal_transition.rs (lines 224-235)

The inputs / outputs setters on the address-based transitions (this file plus address_funding_from_asset_lock_transition.rs, address_funds_transfer_transition.rs, identity_create_from_addresses_transition.rs, identity_top_up_from_addresses_transition.rs) changed return type from infallible to WasmDppResult<()>. Under wasm-bindgen this means JS assignments like transition.inputs = [...] now throw on duplicate addresses where they previously silently overwrote. The duplicate-detection in inputs_to_btree_map is the right safety call, but it's a breaking JS API change for anyone using the setter form — call out alongside the JSON wire-shape changes in release notes.

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

In `packages/rs-dpp/src/identity/state_transition/asset_lock_proof/mod.rs`:
- [BLOCKING] lines 191-260: `AssetLockProof::try_from(Value)` no longer accepts the wire shape produced by the new serde derive
  This PR changed `AssetLockProof` to `#[serde(tag = "type", rename_all = "camelCase")]`, so `platform_value::to_value(asset_lock_proof)` (and the parent transition's `to_object`) now produces maps shaped like `{"type": "instant", ...flattened inner fields}`. But both `TryFrom<&Value>` and `TryFrom<Value>` (lines 191-260) still only accept either a numeric `type` (`get_optional_integer("type")`) or the pre-serde capitalized wrapper forms (`{"Instant": ...}`/`{"Chain": ...}`). With the new wire shape, `get_optional_integer` is called on a `Value::Text("instant")` and errors with a value-type mismatch (`Error::StructureError`), which is propagated as `ProtocolError::ValueError`. As a result, `IdentityTopUpTransitionV0::from_object` (`packages/rs-dpp/src/state_transition/state_transitions/identity/identity_topup_transition/v0/value_conversion.rs:35`) and `IdentityCreateTransitionV0::from_object` (`.../identity_create_transition/v0/value_conversion.rs:44`) now reject the value form their own `to_object` produces — the canonical `to_object`/`from_object` round-trip for these transitions is broken. The handwritten `Value` decoder must be updated alongside the serde change to accept the new internally-tagged shape (string `type`, no inner wrapper key).
- [SUGGESTION] lines 37-43: Switching `AssetLockProof` from `untagged` to `tag = "type"` is a breaking JSON wire change
  `AssetLockProof` now serializes as internally-tagged `{ "type": "instant"|"chain", ...inner fields }` instead of the prior untagged shape (which emitted just the inner struct). Anywhere `AssetLockProof` shows up nested (`IdentityCreateTransition.assetLockProof`, `IdentityTopUpTransition.assetLockProof`, `AddressFundingFromAssetLockTransitionV0.asset_lock_proof`, `ShieldFromAssetLockTransitionV0.asset_lock_proof`), the JSON output now carries the new tag. Bincode `Encode`/`Decode` are unaffected, so the consensus binary format is unchanged, but external JSON consumers (DAPI client snapshots, fixtures, third-party tooling) that parsed the old untagged shape will break. Worth calling out as a deliberate breaking change in release notes / migration docs.

In `packages/wasm-dpp2/src/shielded/unshield_transition.rs`:
- [BLOCKING] lines 35-43: `UnshieldTransitionObject.outputAddress` advertises `string`, but `toObject()` emits `Uint8Array`
  The TypeScript interface declares `outputAddress: string`, but the underlying `PlatformAddress` Serialize impl emits raw bytes in non-human-readable formats (`packages/rs-dpp/src/address_funds/platform_address.rs:55-67`) and `to_object` routes through `platform_value_to_object` with `serialize_bytes_as_arrays(false)` (`packages/wasm-dpp2/src/serialization/conversions.rs:308-316`). So `toObject()` produces a `Uint8Array`, not a hex string. Worse, `fromObject()` goes through the non-human-readable deserializer path, so callers who follow the exported type and pass a hex string will hit a deserialization error. Either change the declared type to `Uint8Array` (matching the actual round-trip) or change the serde implementation to emit a hex string in this format too — the declared type and runtime behaviour need to agree.

In `packages/wasm-dpp2/src/shielded/shielded_withdrawal_transition.rs`:
- [BLOCKING] lines 56-66: `ShieldedWithdrawalTransitionJSON.pooling` advertises `string`, but serde only accepts/emits a number
  `Pooling` derives `Serialize_repr`/`Deserialize_repr` over `#[repr(u8)]` (`packages/rs-dpp/src/withdrawal/mod.rs:8-17`), so `toJSON()` (which routes through serde with `is_human_readable() == true`) emits `pooling` as the numeric discriminant 0/1/2, and `fromJSON()` will reject any string. The declared TS shape lies to callers: `ShieldedWithdrawalTransition.fromJSON({ pooling: "standard", ... })` errors at runtime. Match the declaration to the actual numeric representation.

In `packages/wasm-dpp2/src/shielded/orchard_action.rs`:
- [SUGGESTION] lines 100-102: `encryptedNote` is unbounded at the JS→WASM boundary while every other action field is fixed-size
  All other byte fields on `SerializedOrchardActionWasm` are validated immediately at `try_to_fixed_bytes::<32>` / `<64>`, but `encryptedNote` is read as an arbitrary `Vec<u8>` via `try_to_bytes`. The protocol specifies exactly 216 bytes (epk 32 + enc_ciphertext 104 + out_ciphertext 80 — see `packages/rs-dpp/src/shielded/mod.rs:133-142`, builder allocates `Vec::with_capacity(216)`), and the same DoS rationale that motivated `MAX_HALO2_PROOF_BYTES` / `MAX_CORE_SCRIPT_BYTES` in this PR applies here. Each `SerializedAction` flows into a transition that holds an unbounded `Vec<SerializedAction>`, so a single JS call can allocate arbitrary WASM linear memory before any consensus validation rejects the size mismatch. Reject any non-216-byte input at the constructor, mirroring the fixed-size checks already done for `nullifier`, `rk`, `cmx`, `cvNet`, and `spendAuthSig`.

In `packages/wasm-sdk/src/queries/shielded.rs`:
- [SUGGESTION] lines 91-110: Caller-controlled JS arrays iterated unbounded at the WASM boundary (`parse_nullifiers`, `actions_from_js_options`, `input_witnesses_from_js_options`)
  `parse_nullifiers` validates the per-element type and length but places no cap on the array length itself. The same issue affects `actions_from_js_options` (`packages/wasm-dpp2/src/shielded/orchard_action.rs:170-189`) and `input_witnesses_from_js_options` (`packages/wasm-dpp2/src/shielded/address_witness.rs:183-202`), both of which iterate caller-controlled JS arrays of WASM instances without a length cap. The protocol enforces `max_shielded_transition_actions = 16` (`packages/rs-platform-version/src/version/system_limits/v1.rs:14`), and `AddressWitnessWasm::p2sh` already demonstrates the right pattern by hard-checking `MAX_P2SH_SIGNATURES` before the per-element loop (`packages/wasm-dpp2/src/shielded/address_witness.rs:102-108`). Apply the same up-front length check to these helpers (a generous cap is sufficient — even 4096 prevents OOM) so the boundary-layer code rejects oversized inputs locally instead of allocating GB-scale buffers and forwarding malformed queries to the network.

In `packages/wasm-dpp2/src/shielded/shield_transition.rs`:
- [SUGGESTION] lines 117-157: `ShieldTransition::new` does not validate `inputs.len() == input_witnesses.len()`
  `ShieldTransitionWasm::new` reads `inputs` and `input_witnesses` independently, then collapses inputs through `inputs_to_btree_map(inputs)?` (`packages/wasm-dpp2/src/platform_address/...`), which deduplicates by address. The constructor never compares the resulting input count with `input_witnesses.len()`, so a JS caller who passes 3 inputs and 2 witnesses (or 3 inputs that collapse to 2 distinct addresses with 3 witnesses) constructs an in-memory transition that will only fail much later, with a less-actionable error from network round-trip / consensus. The same shape-mismatch hole likely exists in the other address-based wrappers that reuse `inputs_from_js_options` plus a separate witnesses extraction (`IdentityCreateFromAddressesTransition`, `IdentityTopUpFromAddressesTransition`, `AddressFundsTransferTransition`). Surface the mismatch at construction time so JS callers get a clear, immediate error.
- [SUGGESTION] lines 285-290: Shielded transition `fromJSON`/`fromObject` bypass the size caps the constructors enforce
  `ShieldTransitionWasm::new` enforces `MAX_HALO2_PROOF_BYTES` (`check_max_len` at lines 131-139), but `impl_wasm_conversions_serde!` also exposes `fromJSON`/`fromObject` for the same wrapper. Those routes go through the generic serde paths in `packages/wasm-dpp2/src/serialization/conversions.rs:263-300`, which deserialize attacker-controlled byte fields directly into `Vec<u8>` / fixed arrays without reapplying the constructor limits. The same pattern applies to the new `Unshield`, `ShieldedTransfer`, `ShieldedWithdrawal`, and `ShieldFromAssetLock` wrappers for `proof`, `outputScript`, and `signature`. A malicious dApp/imported payload that calls `ShieldTransition.fromJSON(...)` with a multi-GB base64 `proof` will exhaust WASM linear memory before any validation runs. Either apply the same size caps to the inner type's `Deserialize`, or have the macro-generated `from_*` re-validate via a post-deserialize check that mirrors the constructor's bounds.

In `packages/wasm-dpp2/src/state_transitions/proof_result_shielded.rs`:
- [SUGGESTION] lines 86-216: Shielded proof-result `toJSON()` returns embedded `js_sys::Map` instances — not JSON-safe
  `VerifiedShieldedNullifiersWasm::to_json` (line 106-109), `VerifiedShieldedNullifiersWithAddressInfosWasm::to_json` (line 149-152), and `VerifiedShieldedNullifiersWithWithdrawalDocumentWasm::to_json` (line 198-201) all delegate to `to_object`, which embeds `js_sys::Map` instances directly via `js_obj`. `JSON.stringify(result.toJSON())` produces `{"nullifiers":{}}` — Map entries silently disappear. The crate already has a `Map → plain-object` normalization path used elsewhere for exactly this reason (`packages/wasm-dpp2/src/serialization/conversions.rs:80-122`), but these new wrappers bypass it. Convert the maps to plain `{ [hex]: value }` objects in `to_json` (or back the structs with `BTreeMap<String, _>` and route through `impl_wasm_conversions_serde!`, the same pattern `VerifiedShieldedPoolStateWasm` uses).

In `packages/wasm-dpp2/src/shielded/address_witness.rs`:
- [SUGGESTION] lines 81-123: `AddressWitness::p2pkh`/`p2sh` byte fields are unbounded at the JS→WASM boundary
  `AddressWitness.p2pkh` accepts any-length `Vec<u8>` for `signature`, and `AddressWitness.p2sh` caps the *count* of signatures at `MAX_P2SH_SIGNATURES = 17` but applies no byte-length cap to each signature or to `redeem_script`. The same DoS rationale that motivated `MAX_HALO2_PROOF_BYTES` / `MAX_CORE_SCRIPT_BYTES` / `MAX_RECOVERABLE_ECDSA_SIGNATURE_BYTES` elsewhere in this PR applies here: a caller can hand over multi-megabyte signatures or scripts and force them into linear memory before consensus rejects them. Reject signatures longer than `MAX_RECOVERABLE_ECDSA_SIGNATURE_BYTES` (and empty/65-byte only, ideally) and bound `redeemScript` by the standard `MAX_CORE_SCRIPT_BYTES`-style cap before cloning into `BinaryData`.

In `packages/rs-dpp/src/address_funds/mod.rs`:
- [SUGGESTION] lines 5-6: `serde_helpers` parent gate is `serde-conversion` but the module gates itself on `json-conversion`
  `address_funds/mod.rs` exports `pub mod serde_helpers` under `#[cfg(feature = "serde-conversion")]`, but `serde_helpers/mod.rs` is itself gated `#![cfg(feature = "json-conversion")]` and depends on `crate::serialization::json_safe_fields`, which only exists with `json-conversion`. With `serde-conversion` enabled and `json-conversion` disabled, the parent re-exports an empty module. It compiles today only because the `#[serde(with = "...")]` attributes that reference these helpers are also gated on `json-conversion`. Aligning both gates on `json-conversion` (the actual prerequisite) prevents future helpers from silently failing to find the module.

In `packages/wasm-dpp2/src/platform_address/transitions/address_credit_withdrawal_transition.rs`:
- [SUGGESTION] lines 224-235: Address-based transition setters changed from infallible to `WasmDppResult<()>` — breaking JS API
  The `inputs` / `outputs` setters on the address-based transitions (this file plus `address_funding_from_asset_lock_transition.rs`, `address_funds_transfer_transition.rs`, `identity_create_from_addresses_transition.rs`, `identity_top_up_from_addresses_transition.rs`) changed return type from infallible to `WasmDppResult<()>`. Under wasm-bindgen this means JS assignments like `transition.inputs = [...]` now throw on duplicate addresses where they previously silently overwrote. The duplicate-detection in `inputs_to_btree_map` is the right safety call, but it's a breaking JS API change for anyone using the setter form — call out alongside the JSON wire-shape changes in release notes.