test(dpp): improve coverage for epoch distribution, JSON safe serialization, and address witness#3440
Conversation
…zation, and address witness Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds comprehensive unit test coverage across four modules: bincode serialization for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Review GateCommit:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3440 +/- ##
============================================
+ Coverage 80.66% 80.73% +0.07%
============================================
Files 2852 2852
Lines 285371 286061 +690
============================================
+ Hits 230190 230963 +773
+ Misses 55181 55098 -83
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/rs-dpp/src/serialization/json/safe_integer.rs (1)
554-754: Consider table-driven tests to reduce repetition.Many boundary tests follow identical setup/assert patterns. A small helper or macro-based table would keep intent while reducing maintenance overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/serialization/json/safe_integer.rs` around lines 554 - 754, Tests repeat the same pattern for many integer boundary cases; replace the duplicated assertions by creating a small table-driven helper (or macro) that takes the test value, a flag/enum for expected JSON type (number vs string), and the target type, then calls serde_json::to_value/from_value and asserts both the JSON type and round-trip equality; locate uses of TestU64, TestI64, TestOptionU64, TestOptionI64, JS_MAX_SAFE_INTEGER and the serde_json::to_value/from_value calls and refactor those test functions to call the helper with rows for each boundary (exact, above/below, zero, None/Some) to remove duplicated setup/assert logic.packages/rs-dpp/src/fee/epoch/distribution.rs (3)
1150-1205: Consider consolidating with the existing test module.The test coverage is excellent (zero epochs, monotonicity, era boundaries, parameterization, and offset independence). However, the
additional_prefix creates a separate module for tests that logically belong with the existingoriginal_removed_credits_multiplier_frommodule at line 341. Consolidating these tests into the original module would improve code organization and discoverability.♻️ Suggested consolidation
Move tests from
mod additional_original_removed_credits_multiplier_frominto the existingmod original_removed_credits_multiplier_fromat line 341, removing theadditional_prefix and keeping all tests for the same function together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/fee/epoch/distribution.rs` around lines 1150 - 1205, Tests for original_removed_credits_multiplier_from are duplicated into a separate module named additional_original_removed_credits_multiplier_from; move those tests into the existing mod original_removed_credits_multiplier_from (remove the additional_ prefix) so all tests for original_removed_credits_multiplier_from live together, keeping each test function (should_create_multiplier_of_one_when_no_epochs_have_passed, should_increase_multiplier_as_more_epochs_pass, should_handle_era_boundary_crossing, should_handle_different_epochs_per_era, should_produce_same_multiplier_regardless_of_absolute_epoch_offset) intact and deleting the now-empty additional_ module.
1236-1323: Remove misleading "additional_" prefix; otherwise excellent tests.Like the previous module,
additional_refund_storage_fee_to_epochs_maphas no prior test module to supplement, making theadditional_prefix misleading. The tests themselves are well-crafted, properly verifying skip logic, era boundaries, and epoch counting with clear assertions and helpful comments.♻️ Suggested fix
- mod additional_refund_storage_fee_to_epochs_map { + mod refund_storage_fee_to_epochs_map {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/fee/epoch/distribution.rs` around lines 1236 - 1323, The module name is misleadingly prefixed with "additional_"; rename the test module additional_refund_storage_fee_to_epochs_map to refund_storage_fee_to_epochs_map (or another clear name without the "additional_" prefix) so it matches the tested function refund_storage_fee_to_epochs_map and project naming conventions; update the mod declaration and any references to the module name (if present) so the tests still compile and keep the test bodies unchanged.
1207-1234: Remove misleading "additional_" prefix.The module name
additional_restore_original_removed_credits_amountsuggests supplementary tests, but no prior test module exists forrestore_original_removed_credits_amount. The prefix should be removed for clarity.♻️ Suggested fix
- mod additional_restore_original_removed_credits_amount { + mod restore_original_removed_credits_amount {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/fee/epoch/distribution.rs` around lines 1207 - 1234, Rename the test module additional_restore_original_removed_credits_amount to restore_original_removed_credits_amount to remove the misleading "additional_" prefix; update the module declaration (mod additional_restore_original_removed_credits_amount) and any references to that module so the tests (which call restore_original_removed_credits_amount) keep the same behavior and compile cleanly, leaving test bodies and function calls unchanged.packages/rs-dpp/src/address_funds/witness.rs (1)
670-731: Tests usedecode_from_slice, which exercisesDecode, not theBorrowDecodeimplementation.The comment on line 679 ("borrow_decode is exercised through decode_from_slice") is inaccurate. In bincode 2.x,
decode_from_sliceinvokes theDecodetrait. To test theBorrowDecodeimplementation (lines 91-122), usebincode::borrow_decode_from_sliceinstead.♻️ Suggested fix to exercise BorrowDecode
#[test] fn test_borrow_decode_p2pkh_round_trip() { let witness = AddressWitness::P2pkh { signature: BinaryData::new(vec![0xAB, 0xCD, 0xEF]), }; let encoded = bincode::encode_to_vec(&witness, config::standard()).unwrap(); - // borrow_decode is exercised through decode_from_slice - let decoded: AddressWitness = bincode::decode_from_slice(&encoded, config::standard()) + // Use borrow_decode_from_slice to exercise the BorrowDecode impl + let decoded: AddressWitness = bincode::borrow_decode_from_slice(&encoded, config::standard()) .unwrap() .0; assert_eq!(witness, decoded); }Apply similar changes to the other
test_borrow_decode_*functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/address_funds/witness.rs` around lines 670 - 731, The tests claiming to exercise BorrowDecode actually call bincode::decode_from_slice (which uses Decode) — update each test named test_borrow_decode_p2pkh_round_trip, test_borrow_decode_p2sh_round_trip, test_borrow_decode_rejects_excessive_signatures, and test_borrow_decode_invalid_discriminant_fails to call bincode::borrow_decode_from_slice so the BorrowDecode impl (lines ~91-122) is exercised; also update or remove the inaccurate comment ("borrow_decode is exercised through decode_from_slice") to reflect the correct usage.
🤖 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/rs-dpp/src/serialization/json/safe_integer_map.rs`:
- Around line 775-779: The test nested_map_invalid_inner_value_type_fails (and
its target type TestNestedMap) is currently asserting a format error by passing
the string "not_a_number" — rename the test to something like
nested_map_invalid_inner_value_string_fails if you intend to detect invalid
numeric strings, or change the JSON payload to produce an actual JSON type
mismatch (e.g., replace "not_a_number" with true, null, or an array) so the test
truly covers an inner value type mismatch; update the test name and/or the
serde_json::json! payload accordingly to keep intent and implementation
consistent with TestNestedMap.
---
Nitpick comments:
In `@packages/rs-dpp/src/address_funds/witness.rs`:
- Around line 670-731: The tests claiming to exercise BorrowDecode actually call
bincode::decode_from_slice (which uses Decode) — update each test named
test_borrow_decode_p2pkh_round_trip, test_borrow_decode_p2sh_round_trip,
test_borrow_decode_rejects_excessive_signatures, and
test_borrow_decode_invalid_discriminant_fails to call
bincode::borrow_decode_from_slice so the BorrowDecode impl (lines ~91-122) is
exercised; also update or remove the inaccurate comment ("borrow_decode is
exercised through decode_from_slice") to reflect the correct usage.
In `@packages/rs-dpp/src/fee/epoch/distribution.rs`:
- Around line 1150-1205: Tests for original_removed_credits_multiplier_from are
duplicated into a separate module named
additional_original_removed_credits_multiplier_from; move those tests into the
existing mod original_removed_credits_multiplier_from (remove the additional_
prefix) so all tests for original_removed_credits_multiplier_from live together,
keeping each test function
(should_create_multiplier_of_one_when_no_epochs_have_passed,
should_increase_multiplier_as_more_epochs_pass,
should_handle_era_boundary_crossing, should_handle_different_epochs_per_era,
should_produce_same_multiplier_regardless_of_absolute_epoch_offset) intact and
deleting the now-empty additional_ module.
- Around line 1236-1323: The module name is misleadingly prefixed with
"additional_"; rename the test module
additional_refund_storage_fee_to_epochs_map to refund_storage_fee_to_epochs_map
(or another clear name without the "additional_" prefix) so it matches the
tested function refund_storage_fee_to_epochs_map and project naming conventions;
update the mod declaration and any references to the module name (if present) so
the tests still compile and keep the test bodies unchanged.
- Around line 1207-1234: Rename the test module
additional_restore_original_removed_credits_amount to
restore_original_removed_credits_amount to remove the misleading "additional_"
prefix; update the module declaration (mod
additional_restore_original_removed_credits_amount) and any references to that
module so the tests (which call restore_original_removed_credits_amount) keep
the same behavior and compile cleanly, leaving test bodies and function calls
unchanged.
In `@packages/rs-dpp/src/serialization/json/safe_integer.rs`:
- Around line 554-754: Tests repeat the same pattern for many integer boundary
cases; replace the duplicated assertions by creating a small table-driven helper
(or macro) that takes the test value, a flag/enum for expected JSON type (number
vs string), and the target type, then calls serde_json::to_value/from_value and
asserts both the JSON type and round-trip equality; locate uses of TestU64,
TestI64, TestOptionU64, TestOptionI64, JS_MAX_SAFE_INTEGER and the
serde_json::to_value/from_value calls and refactor those test functions to call
the helper with rows for each boundary (exact, above/below, zero, None/Some) to
remove duplicated setup/assert logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebf2f0c7-fc9c-4d58-b0e0-10cd8628cada
📒 Files selected for processing (4)
packages/rs-dpp/src/address_funds/witness.rspackages/rs-dpp/src/fee/epoch/distribution.rspackages/rs-dpp/src/serialization/json/safe_integer.rspackages/rs-dpp/src/serialization/json/safe_integer_map.rs
2 participants
Summary
Adds 75 tests across 4 files in rs-dpp covering real logic.
fee/epoch/distribution.rsserialization/json/safe_integer.rsserialization/json/safe_integer_map.rsaddress_funds/witness.rsTest plan
cargo test -p dpp --lib— 1832 passedcargo fmt --allclean🤖 Generated with Claude Code
Summary by CodeRabbit