test(dpp): add deserialization failure tests and fix stale test structs

[thepastaclaw]

Summary

Add negative deserialization tests, fix stale test struct definitions, and complete discarded error round-trips in rs-dpp.

Changes

C-08: Complete discarded round-trips in error serialization tests

• unsupported_protocol_version_error.rs and unsupported_version_error.rs had tests that serialized a ConsensusError but assigned the result to _cbor (discarding it). The commented-out deserialization used an old Value-based API.
• Replaced with working PlatformDeserializable round-trip: serialize → deserialize → assert equality.

C-14: Fix stale IdentityCreditWithdrawalTransition test structs

• The test module defined 10 progressively-growing structs (V01–V010) with fields (protocol_version, transition_type, revision) that don't exist in the actual IdentityCreditWithdrawalTransitionV0.
• Replaced with 9 structs (V01–V09) that progressively add the real fields: identity_id, amount, core_fee_per_byte, pooling, output_script, nonce, user_fee_increase, signature_public_key_id, signature.

C-10: Add negative deserialization tests for StateTransition

• deserialize_empty_bytes_should_fail — empty slice returns Err
• deserialize_single_byte_should_fail — single 0xFF byte returns Err
• deserialize_truncated_bytes_should_fail — truncated to half, minus last byte, first byte only
• deserialize_corrupted_bytes_should_not_panic — bit-flip in middle of payload
• deserialize_many_empty_list — empty vec returns Ok(vec![])
• deserialize_many_with_invalid_entry — invalid bytes returns Err
• deserialize_many_with_valid_entries — round-trip 3 valid transitions

Testing

All 487 tests pass with cargo test -p dpp --features random-identities,state-transition-signing.

Summary by CodeRabbit

• Tests
  • Added full serialize/deserialize round-trip checks to ensure serialized bytes recover original errors exactly.
  • Expanded state-transition deserialization tests (empty, truncated, corrupted, multiple entries) to validate failure modes and ordering.
  • Reworked identity withdrawal test variants and generation logic to broaden coverage and ensure consistent serialization across variant shapes.

Validation

1. What was tested

   • CI checks on this PR (Rust packages linting, tests, builds)

2. Results

   • All Rust CI checks passing

3. Environment

   • GitHub Actions CI for dashpay/platform

[coderabbitai]

📝 Walkthrough

Walkthrough

Tests enhanced across consensus error and state-transition modules: consensus error tests now perform full serialize/deserialize byte round-trips; extensive state transition deserialization tests added for edge cases; identity withdrawal test scaffolding updated to use identity_id and new fields in internal test variants.

Changes

Cohort / File(s)	Summary
Consensus Error Tests packages/rs-dpp/src/errors/consensus/basic/unsupported_protocol_version_error.rs, packages/rs-dpp/src/errors/consensus/basic/unsupported_version_error.rs	Refactored tests to serialize ConsensusError to bytes with platform version and deserialize via ConsensusError::deserialize_from_bytes. Removed CBOR-only recovery path and added equality assertions against deserialized errors.
State Transition Deserialization Tests packages/rs-dpp/src/state_transition/serialization.rs	Added comprehensive unit tests covering empty/truncated/corrupted inputs, feature-gated (random-identities) failure modes, and deserialize_many behavior for empty, invalid, and multiple-entry streams.
Identity Withdrawal Transition Tests packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_withdrawal_transition/v0/mod.rs	Test-only internal variant structs reworked: removed protocol_version/transition_type, introduced identity_id plus progressive fields (amount, core_fee_per_byte, pooling, output_script, nonce, user_fee_increase, signature_public_key_id, signature). Updated test initialization and imports accordingly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through bytes both near and far,

I packed each error, then unboxed its star.
Corrupt or truncated, I sniff out the fray,
I stitch identity fields in my playful way.
Tests pass — I twitch my nose and sway.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the main changes: adding deserialization failure tests and fixing stale test struct definitions in the dpp package.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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]

🧹 Nitpick comments (1)

packages/rs-dpp/src/state_transition/serialization.rs (1)

433-511: Optional: drop the random-identities feature gate by not requiring a full Identity.

Both deserialize_truncated_bytes_should_fail and deserialize_corrupted_bytes_should_not_panic use Identity::random_identity exclusively to obtain identity.id(). The Identifier::random() helper in platform_value has no random-identities dependency and would produce an equally valid Identifier. Removing the full identity construction lets these tests run in every CI configuration.

♻️ Proposed simplification for both tests

-    #[test]
-    #[cfg(feature = "random-identities")]
+    #[test]
     fn deserialize_truncated_bytes_should_fail() {
-        let platform_version = PlatformVersion::latest();
-        let identity = Identity::random_identity(5, Some(5), platform_version)
-            .expect("expected a random identity");
         let transition = IdentityCreditWithdrawalTransitionV0 {
-            identity_id: identity.id(),
+            identity_id: platform_value::Identifier::random(),
             amount: 5000000,

Apply the same substitution to deserialize_corrupted_bytes_should_not_panic.

🤖 Prompt for AI Agents

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

In `@packages/rs-dpp/src/state_transition/serialization.rs` around lines 433 -
511, Tests deserialize_truncated_bytes_should_fail and
deserialize_corrupted_bytes_should_not_panic are gated on the random-identities
feature only to obtain identity.id(); replace that by generating a lightweight
Identifier via Identifier::random() (from platform_value) and use its value for
identity_id in IdentityCreditWithdrawalTransitionV0 so the tests no longer need
the random-identities feature; remove the #[cfg(feature = "random-identities")]
annotation from both test functions and update imports to include
platform_value::Identifier (or the crate path used) so
Identity::random_identity(...) calls and the feature gate can be dropped while
keeping IdentityCreditWithdrawalTransitionV0, StateTransition,
serialize_to_bytes and deserialize_from_bytes usage unchanged.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@packages/rs-dpp/src/state_transition/serialization.rs`:
- Around line 433-511: Tests deserialize_truncated_bytes_should_fail and
deserialize_corrupted_bytes_should_not_panic are gated on the random-identities
feature only to obtain identity.id(); replace that by generating a lightweight
Identifier via Identifier::random() (from platform_value) and use its value for
identity_id in IdentityCreditWithdrawalTransitionV0 so the tests no longer need
the random-identities feature; remove the #[cfg(feature = "random-identities")]
annotation from both test functions and update imports to include
platform_value::Identifier (or the crate path used) so
Identity::random_identity(...) calls and the feature gate can be dropped while
keeping IdentityCreditWithdrawalTransitionV0, StateTransition,
serialize_to_bytes and deserialize_from_bytes usage unchanged.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[PastaPastaPasta]

seems useful to have over not

[coderabbitai]

🧹 Nitpick comments (1)

packages/rs-dpp/src/state_transition/serialization.rs (1)

439-449: Optional: extract a test fixture helper to reduce construction duplication.

The IdentityCreditWithdrawalTransitionV0 struct literal with the same fixed field values appears in three places within the same file:

• existing identity_credit_withdrawal_transition_ser_de (lines 298–308)
• new deserialize_truncated_bytes_should_fail (lines 439–449)
• new deserialize_corrupted_bytes_should_not_panic (lines 483–493)

A small private helper inside the test module would eliminate the repetition.

♻️ Proposed helper

+    #[cfg(feature = "random-identities")]
+    fn make_credit_withdrawal_transition(identity: &Identity) -> StateTransition {
+        IdentityCreditWithdrawalTransitionV0 {
+            identity_id: identity.id(),
+            amount: 5_000_000,
+            core_fee_per_byte: 34,
+            pooling: Pooling::Standard,
+            output_script: CoreScript::from_bytes((0..23).collect()),
+            nonce: 1,
+            user_fee_increase: 0,
+            signature_public_key_id: 0,
+            signature: [1u8; 65].to_vec().into(),
+        }
+        .into()
+    }

Each of the three tests then becomes:

-        let transition = IdentityCreditWithdrawalTransitionV0 { ... };
-        let state_transition: StateTransition = transition.into();
+        let state_transition = make_credit_withdrawal_transition(&identity);

Also applies to: 483-493

🤖 Prompt for AI Agents

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

In `@packages/rs-dpp/src/state_transition/serialization.rs` around lines 439 -
449, Extract a private test helper (e.g.,
make_identity_credit_withdrawal_transition_fixture) inside the test module that
constructs and returns an IdentityCreditWithdrawalTransitionV0 with the fixed
values (identity.id(), amount 5000000, core_fee_per_byte 34, Pooling::Standard,
output_script from bytes 0..23, nonce 1, user_fee_increase 0,
signature_public_key_id 0, signature [1u8;65]). Replace the duplicated inline
struct literals in the tests identity_credit_withdrawal_transition_ser_de,
deserialize_truncated_bytes_should_fail, and
deserialize_corrupted_bytes_should_not_panic to call this helper so the
construction is centralized and the tests are DRY.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@packages/rs-dpp/src/state_transition/serialization.rs`:
- Around line 439-449: Extract a private test helper (e.g.,
make_identity_credit_withdrawal_transition_fixture) inside the test module that
constructs and returns an IdentityCreditWithdrawalTransitionV0 with the fixed
values (identity.id(), amount 5000000, core_fee_per_byte 34, Pooling::Standard,
output_script from bytes 0..23, nonce 1, user_fee_increase 0,
signature_public_key_id 0, signature [1u8;65]). Replace the duplicated inline
struct literals in the tests identity_credit_withdrawal_transition_ser_de,
deserialize_truncated_bytes_should_fail, and
deserialize_corrupted_bytes_should_not_panic to call this helper so the
construction is centralized and the tests are DRY.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3c69e75 and c869e06.

📒 Files selected for processing (4)

• packages/rs-dpp/src/errors/consensus/basic/unsupported_protocol_version_error.rs
• packages/rs-dpp/src/errors/consensus/basic/unsupported_version_error.rs
• packages/rs-dpp/src/state_transition/serialization.rs
• packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_withdrawal_transition/v0/mod.rs

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

• packages/rs-dpp/src/errors/consensus/basic/unsupported_protocol_version_error.rs
• packages/rs-dpp/src/errors/consensus/basic/unsupported_version_error.rs