test(dpp): improve state transition coverage for batched and shielded#3359
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds comprehensive unit test coverage across multiple state transition validators (document, token, and shielded variants) and introduces versioned action ID calculation for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
QuantumExplorer
changed the title
Add ~110 new tests for state transition validation and dispatch logic: Batched token validate_structure (35 tests): - token_transfer: amount bounds, transfer-to-self, encrypted note limits - token_burn: amount bounds, note limits, non-proposer note check - token_mint: amount bounds, note limits, non-proposer note check - token_config_update: NoChange rejection, PerpetualDistribution unsupported Token/document dispatch methods (57 tests): - token_transition.rs: associated_token_event (all 11 variants), historical_document_type_name, can_calculate_action_id - document_transition.rs: get_dynamic_property, entropy, data, revision - token_mint recipient_id: explicit vs config fallback vs error Shielded transition validation (57 tests): - common_validation: actions count, proof not empty, anchor not zero - shield_transition: inputs, amounts, fee strategy, proof, anchor - shield_from_asset_lock: value_balance bounds, actions, proof - shielded_transfer: value_balance bounds, actions, proof, anchor - shielded_withdrawal: unshielding_amount bounds, actions, proof - unshield_transition: unshielding_amount bounds, actions, proof Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add 34 more tests for the remaining token transition validate_structure functions: freeze, unfreeze, emergency_action, destroy_frozen_funds, set_price_for_direct_purchase, claim, and direct_purchase. Each covers: valid transition, note too big, non-proposer note check, and amount bounds where applicable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hedule (#3357) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After #3346 merged, calculate_action_id takes platform_version as a second argument. Updated 5 test calls to pass PlatformVersion::latest(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
test/dpp-state-transition-coverage
branch
from
fed39f5 to
c6def50
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3359 +/- ##
============================================
+ Coverage 75.50% 75.87% +0.37%
============================================
Files 2912 2912
Lines 281065 283860 +2795
============================================
+ Hits 212211 215375 +3164
+ Misses 68854 68485 -369
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
packages/rs-dpp/src/state_transition/state_transitions/shielded/shielded_transfer_transition/v0/state_transition_validation.rs (1)
96-201: Optional: deduplicate platform version setup in tests
PlatformVersion::latest()is repeatedly initialized in each test. A tiny helper (or shared local binding pattern) would reduce repetition and improve readability without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/shielded/shielded_transfer_transition/v0/state_transition_validation.rs` around lines 96 - 201, Tests repeatedly call PlatformVersion::latest(); refactor by introducing a small helper or shared binding (e.g., a function like latest_platform_version() or a local let pv = PlatformVersion::latest() used via a test-level helper) and replace direct calls to PlatformVersion::latest() in tests such as should_accept_valid_transition, should_reject_empty_actions, should_reject_too_many_actions, should_reject_zero_value_balance, should_accept_value_balance_at_i64_max, should_reject_empty_proof, etc.; ensure the new helper returns the same PlatformVersion type and update each test to use that helper to remove duplication without changing test behavior.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_destroy_frozen_funds_transition/validate_structure/v0/mod.rs (2)
104-108: Consider asserting error cardinality before matching variant.A quick
assert_eq!(result.errors.len(), 1)makes these tests stricter and reduces ambiguous failures.Also applies to: 121-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_destroy_frozen_funds_transition/validate_structure/v0/mod.rs` around lines 104 - 108, Add an assertion that the errors vector contains exactly one item before matching its variant: check result.errors.len() == 1 (e.g., assert_eq!(result.errors.len(), 1)) immediately before grabbing result.errors.first().unwrap() and the matches! call; do the same change for the other similar test block that currently matches ConsensusError::BasicError(BasicError::InvalidTokenNoteTooBigError(_)) to ensure strict cardinality prior to variant matching.
79-84: Add an exact-boundary test forMAX_TOKEN_NOTE_LENto guard off-by-one regressions.You cover
< maxand> max, but not== max. Adding that case would complete the boundary matrix.➕ Suggested test addition
#[test] fn should_pass_with_short_public_note_and_no_group() { let transition = make_transition(Some("hello".to_string()), None); let result = transition.validate_structure_v0().unwrap(); assert!(result.is_valid()); } + +#[test] +fn should_pass_with_public_note_exactly_max_len() { + let note = "a".repeat(MAX_TOKEN_NOTE_LEN); + let transition = make_transition(Some(note), None); + let result = transition.validate_structure_v0().unwrap(); + assert!(result.is_valid()); +}Also applies to: 98-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_destroy_frozen_funds_transition/validate_structure/v0/mod.rs` around lines 79 - 84, Add tests that exercise the exact boundary of MAX_TOKEN_NOTE_LEN to prevent off-by-one regressions: create a note string exactly MAX_TOKEN_NOTE_LEN characters long (use the MAX_TOKEN_NOTE_LEN constant to build the string), call make_transition(Some(note), None) and assert validate_structure_v0().unwrap().is_valid() to cover the "no group" case (in the test around should_pass_with_short_public_note_and_no_group) and add the analogous exact-length test for the "with group" case referenced near lines 98-101 so both variants (no group and with group) include an == MAX_TOKEN_NOTE_LEN passing assertion.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/validate_structure/v0/mod.rs (1)
100-110: Add an explicit max-length boundary test (== MAX_TOKEN_NOTE_LEN).You already test
MAX_TOKEN_NOTE_LEN + 1; adding the exact boundary case would lock in the off-by-one contract.Suggested test addition
+ #[test] + fn should_pass_when_public_note_is_exactly_max_len() { + let boundary_note = "a".repeat(MAX_TOKEN_NOTE_LEN); + let transition = make_transition(Some(boundary_note), None); + let result = transition.validate_structure_v0().unwrap(); + assert!(result.is_valid()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/validate_structure/v0/mod.rs` around lines 100 - 110, Add a new unit test that verifies the exact boundary case where the public note length equals MAX_TOKEN_NOTE_LEN: create a string of length MAX_TOKEN_NOTE_LEN (instead of MAX_TOKEN_NOTE_LEN + 1) using the same helper make_transition(Some(note), None), call validate_structure_v0() on the transition and assert the result is valid (no errors). Place this alongside should_return_error_when_public_note_too_big and reference MAX_TOKEN_NOTE_LEN, make_transition, and validate_structure_v0 to lock in the off-by-one contract.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_burn_transition/validate_structure/v0/mod.rs (1)
104-205: Consider a tiny assertion helper to reduce repeated error-shape checks.Several tests repeat
errors.len() == 1+matches!boilerplate; extracting a helper would make future additions easier to maintain.♻️ Optional refactor sketch
+ fn assert_single_basic_error( + result: &SimpleConsensusValidationResult, + predicate: impl FnOnce(&ConsensusError) -> bool, + ) { + assert_eq!(result.errors.len(), 1); + assert!(predicate(&result.errors[0])); + } + #[test] fn zero_burn_amount_returns_invalid_token_amount_error() { let transition = TokenBurnTransition::V0(TokenBurnTransitionV0 { base: make_base(), burn_amount: 0, public_note: None, }); let result = transition.validate_structure_v0().unwrap(); - assert_eq!(result.errors.len(), 1); - assert!(matches!( - &result.errors[0], - ConsensusError::BasicError(BasicError::InvalidTokenAmountError(_)) - )); + assert_single_basic_error(&result, |e| { + matches!(e, ConsensusError::BasicError(BasicError::InvalidTokenAmountError(_))) + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_burn_transition/validate_structure/v0/mod.rs` around lines 104 - 205, Tests repeat the pattern of asserting result.errors.len() == 1 and then checking the error shape with matches! (e.g., in zero_burn_amount_returns_invalid_token_amount_error, burn_amount_exceeding_max_returns_invalid_token_amount_error, public_note_too_big_returns_error, note_on_non_proposer_returns_error); add a small test helper (e.g., fn assert_single_error_shape(result: ValidationResult, expected: fn(&ConsensusError)->bool)) in the test module that asserts there's exactly one error and that the sole error satisfies the provided predicate, then replace the duplicated assertions in those test functions with calls to this helper to keep intent clear and reduce repetition.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/validate_structure/v0/mod.rs (1)
75-129: LGTM! Tests provide good coverage of the validation logic.The test cases cover all main branches:
- Valid scenarios (no note, short note without group, note with proposer group)
- Error scenarios (note too big, note with non-proposer in group)
Consider adding a boundary test for exactly
MAX_TOKEN_NOTE_LENcharacters to confirm it passes (edge of the valid range):🧪 Optional boundary test
+ #[test] + fn should_pass_with_public_note_at_max_length() { + let max_note = "a".repeat(MAX_TOKEN_NOTE_LEN); + let transition = make_transition(Some(max_note), None); + let result = transition.validate_structure_v0().unwrap(); + assert!(result.is_valid()); + } + #[test] fn should_return_error_when_public_note_too_big() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/validate_structure/v0/mod.rs` around lines 75 - 129, Add a boundary test that constructs a public note exactly MAX_TOKEN_NOTE_LEN characters long (use MAX_TOKEN_NOTE_LEN), create the transition via make_transition(Some(long_note), None) and assert validate_structure_v0() is valid; also add a similar case with group_info.action_is_proposer = true to ensure a MAX_TOKEN_NOTE_LEN note passes when proposer. Use the existing test helpers (make_transition, GroupStateTransitionInfo, validate_structure_v0) and mirror the style of the other tests in this file.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/validate_structure/v0/mod.rs (1)
131-138: Consider extracting a small transition builder helper to reduce repetitive test setup.The repeated
TokenTransferTransition::V0(TokenTransferTransitionV0 { ... })blocks make the suite harder to scan and maintain. A tiny helper/fixture would keep each test focused on the one field it mutates.Also applies to: 150-157, 169-176, 184-191, 203-210, 222-229, 237-244, 256-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/validate_structure/v0/mod.rs` around lines 131 - 138, Extract a small test fixture function that returns a default TokenTransferTransition::V0 with TokenTransferTransitionV0 populated (use make_base() for base, amount: 0, recipient_id: Identifier::new([2u8;32]), and None for note fields) and replace the repeated inline constructions in the tests with calls to that helper; update each test to clone the fixture and only mutate the specific field under test (references: TokenTransferTransition::V0, TokenTransferTransitionV0, make_base, recipient_id, amount, public_note/shared_encrypted_note/private_encrypted_note).packages/rs-dpp/src/state_transition/state_transitions/shielded/unshield_transition/v0/state_transition_validation.rs (1)
95-212: Comprehensive validation coverage.The test suite thoroughly covers all validation paths:
- Positive case validation
- Empty and excessive actions count
- Amount bounds (zero, i64::MAX boundary, overflow)
- Empty proof and zero anchor
The tests are well-named and use
assert_matches!appropriately for error type verification.Consider adding a boundary test that verifies exactly
max_shielded_transition_actionsis accepted:🧪 Optional boundary test
+ #[test] + fn should_accept_exactly_max_actions() { + let platform_version = PlatformVersion::latest(); + let max = platform_version + .system_limits + .max_shielded_transition_actions; + let mut transition = valid_unshield_transition(); + transition.actions = vec![dummy_action(); max as usize]; + + let result = transition.validate_structure(platform_version); + assert!( + result.is_valid(), + "Expected valid result for exactly max actions, got errors: {:?}", + result.errors + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/shielded/unshield_transition/v0/state_transition_validation.rs` around lines 95 - 212, Add a boundary test that ensures exactly max_shielded_transition_actions is accepted: create a new test (e.g., should_accept_max_actions) that fetches PlatformVersion::latest(), builds a transition from valid_unshield_transition(), sets transition.actions = vec![dummy_action(); max as usize] where max = platform_version.system_limits.max_shielded_transition_actions, then calls transition.validate_structure(platform_version) and asserts result.is_valid() (similar to should_validate_a_valid_transition). Place the test alongside the other tests in state_transition_validation.rs and reference the existing helpers valid_unshield_transition() and dummy_action().packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_freeze_transition/validate_structure/v0/mod.rs (1)
98-127: Add a max-length acceptance test forpublic_note.You already test overflow at Line 100; adding the
MAX_TOKEN_NOTE_LENexact-boundary pass case would fully lock the length boundary behavior.Proposed test addition
#[test] + fn should_pass_with_public_note_at_max_length() { + let note = "a".repeat(MAX_TOKEN_NOTE_LEN); + let transition = make_transition(Some(note), None); + let result = transition.validate_structure_v0().unwrap(); + assert!(result.is_valid()); + } + + #[test] fn should_return_error_when_note_present_but_non_proposer_in_group() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_freeze_transition/validate_structure/v0/mod.rs` around lines 98 - 127, Add a new unit test that asserts a public_note with length exactly MAX_TOKEN_NOTE_LEN is accepted: create a string of "a".repeat(MAX_TOKEN_NOTE_LEN), build the transition via make_transition(Some(long_note), ...) (mirror the other tests), call validate_structure_v0(), and assert result.is_valid() with no TokenNote errors; place this alongside should_return_error_when_public_note_too_big to lock the boundary behavior for MAX_TOKEN_NOTE_LEN.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/validate_structure/v0/mod.rs (1)
98-127: Add an exact-length boundary test forpublic_note.Current coverage validates
> MAX_TOKEN_NOTE_LEN(Line 100) but not== MAX_TOKEN_NOTE_LEN. Adding this closes an off-by-one gap around the length check at Line 18.Proposed test addition
#[test] + fn should_pass_with_public_note_at_max_length() { + let note = "a".repeat(MAX_TOKEN_NOTE_LEN); + let transition = make_transition(Some(note), None); + let result = transition.validate_structure_v0().unwrap(); + assert!(result.is_valid()); + } + + #[test] fn should_return_error_when_note_present_but_non_proposer_in_group() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/validate_structure/v0/mod.rs` around lines 98 - 127, Add a boundary unit test that verifies a public_note with length == MAX_TOKEN_NOTE_LEN is accepted: create a test (e.g., should_accept_public_note_at_max_length) that builds a note using "a".repeat(MAX_TOKEN_NOTE_LEN), calls make_transition(Some(long_note), None), invokes validate_structure_v0() on the transition, and asserts result.is_valid(); reference MAX_TOKEN_NOTE_LEN, make_transition and validate_structure_v0 to locate where to add the test.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition.rs (2)
453-621: Consider consolidating repeated per-variantNoneassertions.The
entropy,data,revision, anddata_mutsections repeat the same pattern by variant. A small table-driven helper would keep this coverage 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/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition.rs` around lines 453 - 621, Tests repeat identical per-variant assertions for methods entropy(), data(), revision(), and data_mut(); replace these with table-driven helpers that iterate over transition constructors (make_create_transition, make_replace_transition, make_delete_transition, make_transfer_transition, make_update_price_transition, make_purchase_transition) and their expected results, then assert each method (entropy(), data(), revision(), data_mut()) against the expected value inside a single loop; add a small helper function in the test module (e.g., assert_transition_property<T>(Vec<(Fn() -> DocumentTransition, expected)>) or a Vec of tuples pairing constructor functions with expected Option values) to consolidate the repeated tests while preserving coverage and keep existing test names by replacing body with calls to the helper.
391-439: Add one dot-syntax test forget_dynamic_propertycontract alignment.The trait documentation says
pathsupports dot-syntax, but these tests currently validate only flat keys. Please add one nested-path assertion (or explicitly narrow the contract in docs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition.rs` around lines 391 - 439, Tests only cover flat keys but the get_dynamic_property contract supports dot-syntax; add one test that inserts a nested value (e.g., a map/object with a child field) into a create or replace transition and assert that transition.get_dynamic_property("parent.child") returns the inner Value (use existing helpers like make_create_transition or make_replace_transition and the get_dynamic_property method name to locate where to add the test). Ensure the nested structure uses the same Value variants as other tests so the assertion mirrors the existing style.packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs (1)
286-294: Derive “below minimum” input from platform config instead of hard-coding it.This test currently bakes in a protocol assumption. Deriving from
min_input_amountwill keep it stable across version changes.♻️ Proposed test hardening
#[test] fn should_reject_input_below_minimum() { let platform_version = PlatformVersion::latest(); let mut transition = valid_shield_transition(); - // Set the input amount to 1 (below minimum of 100_000) + let min_input_amount = platform_version + .dpp + .state_transitions + .address_funds + .min_input_amount; + let below_min = min_input_amount.saturating_sub(1); + assert!(below_min < min_input_amount, "test requires min_input_amount > 0"); + transition.inputs.clear(); transition .inputs - .insert(PlatformAddress::P2pkh([1u8; 20]), (0u32, 1u64)); + .insert(PlatformAddress::P2pkh([1u8; 20]), (0u32, below_min));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs` around lines 286 - 294, The test should_reject_input_below_minimum currently hard-codes a below-minimum input value; instead query the min_input_amount from PlatformVersion::latest() and set the test input to (min_input_amount - 1) so it follows protocol config changes. Update the test (around valid_shield_transition and PlatformVersion usage) to obtain the min_input_amount from the platform_version instance (e.g., access the protocol/drive config field or helper like get_min_input_amount()) and use that derived value to build the inputs map rather than the literal 1 or 100_000.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0_methods.rs (1)
89-112: Add a unit test for the unsupported-version error pathPlease add a test that sets
token_set_price_action_id_versionto an unsupported value and assertsProtocolError::UnknownVersionMismatch. This locks in the dispatch guard behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0_methods.rs` around lines 89 - 112, Add a unit test that sets platform_version.dpp.token_versions.token_set_price_action_id_version to an unsupported value (e.g. 2) and calls calculate_action_id_with_fields (the dispatch in v0_methods.rs that matches on token_set_price_action_id_version). Assert the result is Err(ProtocolError::UnknownVersionMismatch) and verify its fields (method == "calculate_action_id_with_fields", known_versions contains [0,1], and received == the unsupported value). Use the same parameter types passed into calculate_action_id_with_fields (token_id, owner_id, identity_contract_nonce, price_per_token) and construct a minimal PlatformVersion/DPP TokenVersions instance for the call.
🤖 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/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs`:
- Around line 879-891: The test calculate_action_id_returns_some_for_burn (and
the similar test at lines 894-907) only asserts result.is_some(), which allows
Some(Err(_)) to pass; change the assertions to ensure successful computation by
matching Some(Ok(_)) (e.g., assert!(matches!(burn.calculate_action_id(owner_id,
pv), Some(Ok(_)))) or unwrap and assert_ok) so the
TokenTransition::Burn.calculate_action_id (and the corresponding test for the
other transition) must be validated to return Ok on success rather than any Some
variant.
---
Nitpick comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition.rs`:
- Around line 453-621: Tests repeat identical per-variant assertions for methods
entropy(), data(), revision(), and data_mut(); replace these with table-driven
helpers that iterate over transition constructors (make_create_transition,
make_replace_transition, make_delete_transition, make_transfer_transition,
make_update_price_transition, make_purchase_transition) and their expected
results, then assert each method (entropy(), data(), revision(), data_mut())
against the expected value inside a single loop; add a small helper function in
the test module (e.g., assert_transition_property<T>(Vec<(Fn() ->
DocumentTransition, expected)>) or a Vec of tuples pairing constructor functions
with expected Option values) to consolidate the repeated tests while preserving
coverage and keep existing test names by replacing body with calls to the
helper.
- Around line 391-439: Tests only cover flat keys but the get_dynamic_property
contract supports dot-syntax; add one test that inserts a nested value (e.g., a
map/object with a child field) into a create or replace transition and assert
that transition.get_dynamic_property("parent.child") returns the inner Value
(use existing helpers like make_create_transition or make_replace_transition and
the get_dynamic_property method name to locate where to add the test). Ensure
the nested structure uses the same Value variants as other tests so the
assertion mirrors the existing style.
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_burn_transition/validate_structure/v0/mod.rs`:
- Around line 104-205: Tests repeat the pattern of asserting result.errors.len()
== 1 and then checking the error shape with matches! (e.g., in
zero_burn_amount_returns_invalid_token_amount_error,
burn_amount_exceeding_max_returns_invalid_token_amount_error,
public_note_too_big_returns_error, note_on_non_proposer_returns_error); add a
small test helper (e.g., fn assert_single_error_shape(result: ValidationResult,
expected: fn(&ConsensusError)->bool)) in the test module that asserts there's
exactly one error and that the sole error satisfies the provided predicate, then
replace the duplicated assertions in those test functions with calls to this
helper to keep intent clear and reduce repetition.
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_destroy_frozen_funds_transition/validate_structure/v0/mod.rs`:
- Around line 104-108: Add an assertion that the errors vector contains exactly
one item before matching its variant: check result.errors.len() == 1 (e.g.,
assert_eq!(result.errors.len(), 1)) immediately before grabbing
result.errors.first().unwrap() and the matches! call; do the same change for the
other similar test block that currently matches
ConsensusError::BasicError(BasicError::InvalidTokenNoteTooBigError(_)) to ensure
strict cardinality prior to variant matching.
- Around line 79-84: Add tests that exercise the exact boundary of
MAX_TOKEN_NOTE_LEN to prevent off-by-one regressions: create a note string
exactly MAX_TOKEN_NOTE_LEN characters long (use the MAX_TOKEN_NOTE_LEN constant
to build the string), call make_transition(Some(note), None) and assert
validate_structure_v0().unwrap().is_valid() to cover the "no group" case (in the
test around should_pass_with_short_public_note_and_no_group) and add the
analogous exact-length test for the "with group" case referenced near lines
98-101 so both variants (no group and with group) include an ==
MAX_TOKEN_NOTE_LEN passing assertion.
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/validate_structure/v0/mod.rs`:
- Around line 100-110: Add a new unit test that verifies the exact boundary case
where the public note length equals MAX_TOKEN_NOTE_LEN: create a string of
length MAX_TOKEN_NOTE_LEN (instead of MAX_TOKEN_NOTE_LEN + 1) using the same
helper make_transition(Some(note), None), call validate_structure_v0() on the
transition and assert the result is valid (no errors). Place this alongside
should_return_error_when_public_note_too_big and reference MAX_TOKEN_NOTE_LEN,
make_transition, and validate_structure_v0 to lock in the off-by-one contract.
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_freeze_transition/validate_structure/v0/mod.rs`:
- Around line 98-127: Add a new unit test that asserts a public_note with length
exactly MAX_TOKEN_NOTE_LEN is accepted: create a string of
"a".repeat(MAX_TOKEN_NOTE_LEN), build the transition via
make_transition(Some(long_note), ...) (mirror the other tests), call
validate_structure_v0(), and assert result.is_valid() with no TokenNote errors;
place this alongside should_return_error_when_public_note_too_big to lock the
boundary behavior for MAX_TOKEN_NOTE_LEN.
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0_methods.rs`:
- Around line 89-112: Add a unit test that sets
platform_version.dpp.token_versions.token_set_price_action_id_version to an
unsupported value (e.g. 2) and calls calculate_action_id_with_fields (the
dispatch in v0_methods.rs that matches on token_set_price_action_id_version).
Assert the result is Err(ProtocolError::UnknownVersionMismatch) and verify its
fields (method == "calculate_action_id_with_fields", known_versions contains
[0,1], and received == the unsupported value). Use the same parameter types
passed into calculate_action_id_with_fields (token_id, owner_id,
identity_contract_nonce, price_per_token) and construct a minimal
PlatformVersion/DPP TokenVersions instance for the call.
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/validate_structure/v0/mod.rs`:
- Around line 75-129: Add a boundary test that constructs a public note exactly
MAX_TOKEN_NOTE_LEN characters long (use MAX_TOKEN_NOTE_LEN), create the
transition via make_transition(Some(long_note), None) and assert
validate_structure_v0() is valid; also add a similar case with
group_info.action_is_proposer = true to ensure a MAX_TOKEN_NOTE_LEN note passes
when proposer. Use the existing test helpers (make_transition,
GroupStateTransitionInfo, validate_structure_v0) and mirror the style of the
other tests in this file.
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/validate_structure/v0/mod.rs`:
- Around line 131-138: Extract a small test fixture function that returns a
default TokenTransferTransition::V0 with TokenTransferTransitionV0 populated
(use make_base() for base, amount: 0, recipient_id: Identifier::new([2u8;32]),
and None for note fields) and replace the repeated inline constructions in the
tests with calls to that helper; update each test to clone the fixture and only
mutate the specific field under test (references: TokenTransferTransition::V0,
TokenTransferTransitionV0, make_base, recipient_id, amount,
public_note/shared_encrypted_note/private_encrypted_note).
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/validate_structure/v0/mod.rs`:
- Around line 98-127: Add a boundary unit test that verifies a public_note with
length == MAX_TOKEN_NOTE_LEN is accepted: create a test (e.g.,
should_accept_public_note_at_max_length) that builds a note using
"a".repeat(MAX_TOKEN_NOTE_LEN), calls make_transition(Some(long_note), None),
invokes validate_structure_v0() on the transition, and asserts
result.is_valid(); reference MAX_TOKEN_NOTE_LEN, make_transition and
validate_structure_v0 to locate where to add the test.
In
`@packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs`:
- Around line 286-294: The test should_reject_input_below_minimum currently
hard-codes a below-minimum input value; instead query the min_input_amount from
PlatformVersion::latest() and set the test input to (min_input_amount - 1) so it
follows protocol config changes. Update the test (around valid_shield_transition
and PlatformVersion usage) to obtain the min_input_amount from the
platform_version instance (e.g., access the protocol/drive config field or
helper like get_min_input_amount()) and use that derived value to build the
inputs map rather than the literal 1 or 100_000.
In
`@packages/rs-dpp/src/state_transition/state_transitions/shielded/shielded_transfer_transition/v0/state_transition_validation.rs`:
- Around line 96-201: Tests repeatedly call PlatformVersion::latest(); refactor
by introducing a small helper or shared binding (e.g., a function like
latest_platform_version() or a local let pv = PlatformVersion::latest() used via
a test-level helper) and replace direct calls to PlatformVersion::latest() in
tests such as should_accept_valid_transition, should_reject_empty_actions,
should_reject_too_many_actions, should_reject_zero_value_balance,
should_accept_value_balance_at_i64_max, should_reject_empty_proof, etc.; ensure
the new helper returns the same PlatformVersion type and update each test to use
that helper to remove duplication without changing test behavior.
In
`@packages/rs-dpp/src/state_transition/state_transitions/shielded/unshield_transition/v0/state_transition_validation.rs`:
- Around line 95-212: Add a boundary test that ensures exactly
max_shielded_transition_actions is accepted: create a new test (e.g.,
should_accept_max_actions) that fetches PlatformVersion::latest(), builds a
transition from valid_unshield_transition(), sets transition.actions =
vec![dummy_action(); max as usize] where max =
platform_version.system_limits.max_shielded_transition_actions, then calls
transition.validate_structure(platform_version) and asserts result.is_valid()
(similar to should_validate_a_valid_transition). Place the test alongside the
other tests in state_transition_validation.rs and reference the existing helpers
valid_unshield_transition() and dummy_action().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53510d49-e9b5-437c-911a-6824ff78bd48
📒 Files selected for processing (26)
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_burn_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_claim_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_destroy_frozen_funds_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_direct_purchase_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_freeze_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/validate_structure/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/common_validation.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/v0/state_transition_validation.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shielded_transfer_transition/v0/state_transition_validation.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shielded_withdrawal_transition/v0/state_transition_validation.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/unshield_transition/v0/state_transition_validation.rspackages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/v1.rspackages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/v2.rspackages/rs-platform-version/src/version/v12.rs
calculate_action_id returns Option<Result<...>>, so is_some() would pass even on Some(Err(_)). Use matches!(result, Some(Ok(_))) to verify successful computation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Test-heavy PR that also carries a consensus-critical action_id fix (commit 9ff7e84 / #3357). Test quality is solid overall; the action_id assertion weakness was self-corrected in 715558f. Main concerns are a missing bounds check in shield fee-strategy validation (production code gap), incomplete boundary tests for encrypted notes, and untested dispatch paths for ConfigUpdate/SetPriceForDirectPurchase at the wrapper level.
Reviewed commit: 715558f
🟡 5 suggestion(s) | 💬 1 nitpick(s)
2 additional finding(s) omitted (not in diff).
🤖 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/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs`:
- [SUGGESTION] lines 157-164: Shield fee-strategy validation missing out-of-bounds index check
The fee_strategy loop (lines 157-164) checks for duplicates but never validates that `DeductFromInput(index)` or `AddToOutput(index)` indices are within `self.inputs.len()`. All three address_funds validators (`address_funds_transfer`, `address_credit_withdrawal`, `address_funding_from_asset_lock`) include `FeeStrategyIndexOutOfBoundsError` checks for both step types. This is a production code gap — an attacker could submit a shield transition referencing a nonexistent input index, and the error would only surface later (or not at all) during execution rather than at structural validation time.
In `packages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/v2.rs`:
- [SUGGESTION] line 1: Consensus-critical fix included under test(dpp) PR title
Commit 9ff7e84c7 adds `token_set_price_action_id_version` to `DPPTokenVersions`, sets it to 1 in TOKEN_VERSIONS_V2 (used by protocol v12), and changes how `SetPriceForDirectPurchase` action_ids are hashed — binding the full `TokenPricingSchedule` instead of just the price scalar. This is a consensus-critical behavioral change: nodes running v12 will accept/reject different action_id hashes than before. The PR description does mention this ('Also includes consensus fix for SetPriceForDirectPurchase action_id binding') and the commit is properly labeled `fix(dpp)`, but the PR title `test(dpp): improve state transition coverage…` obscures the consensus nature of the change. Consider either (a) splitting the fix into its own PR, or (b) retitling to `fix(dpp)!:` to match the most impactful change.
In `packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/validate_structure/v0/mod.rs`:
- [SUGGESTION] lines 235-270: Missing boundary-pass tests for shared/private encrypted notes at MAX length
The `public_note` field has both a boundary-pass test (`public_note_at_max_length_passes`, line 220) and a boundary-fail test (`public_note_too_big_returns_error`, line 201). However, `shared_encrypted_note` (line 235) and `private_encrypted_note` (line 254) only have boundary-fail tests at `MAX_TOKEN_NOTE_LEN + 1`. There are no tests verifying that exactly `MAX_TOKEN_NOTE_LEN` (2048) bytes are accepted for these fields. Since the validation uses `>` (not `>=`), the boundary-pass case is important to lock in.
In `packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs`:
- [SUGGESTION] lines 895-911: calculate_action_id dispatch tests only cover Burn and Mint variants
The `calculate_action_id` wrapper dispatches to 8 variants (Burn, Mint, Freeze, Unfreeze, DestroyFrozenFunds, EmergencyAction, ConfigUpdate, SetPriceForDirectPurchase), but the dispatch-level tests only verify Burn (line 878) and Mint (line 895). ConfigUpdate and SetPriceForDirectPurchase are especially important to test at the wrapper level since they have version-gated behavior (v0 vs v1 hashing). The `can_calculate_action_id` tests cover all variants but only test the boolean predicate, not the actual dispatch.
- [SUGGESTION] lines 878-911: Version-gating tests pinned to latest() instead of explicit version boundaries
Tests throughout the new test files use `PlatformVersion::latest()` or `PlatformVersion::first()` for version-sensitive behavior (e.g., action_id hashing that differs between v11/v12). If `latest()` changes in the future, these tests may silently start testing different version behavior. For version-boundary testing, explicit version constants make the test intent clearer and more resilient to version bumps.
Comment on lines
+235
to
+270
| fn shared_encrypted_note_too_big_returns_error() { | ||
| let owner_id = Identifier::new([3u8; 32]); | ||
| let transition = TokenTransferTransition::V0(TokenTransferTransitionV0 { | ||
| base: make_base(), | ||
| amount: 100, | ||
| recipient_id: Identifier::new([2u8; 32]), | ||
| public_note: None, | ||
| shared_encrypted_note: Some((0, 0, vec![0u8; MAX_TOKEN_NOTE_LEN + 1])), | ||
| private_encrypted_note: None, | ||
| }); | ||
| let result = transition.validate_structure_v0(owner_id).unwrap(); | ||
| assert_eq!(result.errors.len(), 1); | ||
| assert!(matches!( | ||
| &result.errors[0], | ||
| ConsensusError::BasicError(BasicError::InvalidTokenNoteTooBigError(_)) | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn private_encrypted_note_too_big_returns_error() { | ||
| let owner_id = Identifier::new([3u8; 32]); | ||
| let transition = TokenTransferTransition::V0(TokenTransferTransitionV0 { | ||
| base: make_base(), | ||
| amount: 100, | ||
| recipient_id: Identifier::new([2u8; 32]), | ||
| public_note: None, | ||
| shared_encrypted_note: None, | ||
| private_encrypted_note: Some((0, 0, vec![0u8; MAX_TOKEN_NOTE_LEN + 1])), | ||
| }); | ||
| let result = transition.validate_structure_v0(owner_id).unwrap(); | ||
| assert_eq!(result.errors.len(), 1); | ||
| assert!(matches!( | ||
| &result.errors[0], | ||
| ConsensusError::BasicError(BasicError::InvalidTokenNoteTooBigError(_)) | ||
| )); | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Missing boundary-pass tests for shared/private encrypted notes at MAX length
The public_note field has both a boundary-pass test (public_note_at_max_length_passes, line 220) and a boundary-fail test (public_note_too_big_returns_error, line 201). However, shared_encrypted_note (line 235) and private_encrypted_note (line 254) only have boundary-fail tests at MAX_TOKEN_NOTE_LEN + 1. There are no tests verifying that exactly MAX_TOKEN_NOTE_LEN (2048) bytes are accepted for these fields. Since the validation uses > (not >=), the boundary-pass case is important to lock in.
source: ['claude-general', 'claude-rust-quality']
🤖 Fix this 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/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/validate_structure/v0/mod.rs`:
- [SUGGESTION] lines 235-270: Missing boundary-pass tests for shared/private encrypted notes at MAX length
The `public_note` field has both a boundary-pass test (`public_note_at_max_length_passes`, line 220) and a boundary-fail test (`public_note_too_big_returns_error`, line 201). However, `shared_encrypted_note` (line 235) and `private_encrypted_note` (line 254) only have boundary-fail tests at `MAX_TOKEN_NOTE_LEN + 1`. There are no tests verifying that exactly `MAX_TOKEN_NOTE_LEN` (2048) bytes are accepted for these fields. Since the validation uses `>` (not `>=`), the boundary-pass case is important to lock in.
Comment on lines
+895
to
+911
| #[test] | ||
| fn calculate_action_id_returns_some_for_mint() { | ||
| let base = TokenBaseTransition::default(); | ||
| let owner_id = Identifier::from([3u8; 32]); | ||
|
|
||
| let mint = TokenTransition::Mint(TokenMintTransition::V0(TokenMintTransitionV0 { | ||
| base: base.clone(), | ||
| issued_to_identity_id: None, | ||
| amount: 100, | ||
| public_note: None, | ||
| })); | ||
| let pv = PlatformVersion::latest(); | ||
| assert!(matches!( | ||
| mint.calculate_action_id(owner_id, pv), | ||
| Some(Ok(_)) | ||
| )); | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: calculate_action_id dispatch tests only cover Burn and Mint variants
The calculate_action_id wrapper dispatches to 8 variants (Burn, Mint, Freeze, Unfreeze, DestroyFrozenFunds, EmergencyAction, ConfigUpdate, SetPriceForDirectPurchase), but the dispatch-level tests only verify Burn (line 878) and Mint (line 895). ConfigUpdate and SetPriceForDirectPurchase are especially important to test at the wrapper level since they have version-gated behavior (v0 vs v1 hashing). The can_calculate_action_id tests cover all variants but only test the boolean predicate, not the actual dispatch.
source: ['codex-general']
🤖 Fix this 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/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs`:
- [SUGGESTION] lines 895-911: calculate_action_id dispatch tests only cover Burn and Mint variants
The `calculate_action_id` wrapper dispatches to 8 variants (Burn, Mint, Freeze, Unfreeze, DestroyFrozenFunds, EmergencyAction, ConfigUpdate, SetPriceForDirectPurchase), but the dispatch-level tests only verify Burn (line 878) and Mint (line 895). ConfigUpdate and SetPriceForDirectPurchase are especially important to test at the wrapper level since they have version-gated behavior (v0 vs v1 hashing). The `can_calculate_action_id` tests cover all variants but only test the boolean predicate, not the actual dispatch.
Comment on lines
+878
to
+911
| #[test] | ||
| fn calculate_action_id_returns_some_for_burn() { | ||
| let base = TokenBaseTransition::default(); | ||
| let owner_id = Identifier::from([2u8; 32]); | ||
|
|
||
| let burn = TokenTransition::Burn(TokenBurnTransition::V0(TokenBurnTransitionV0 { | ||
| base: base.clone(), | ||
| burn_amount: 50, | ||
| public_note: None, | ||
| })); | ||
| let pv = PlatformVersion::latest(); | ||
| assert!(matches!( | ||
| burn.calculate_action_id(owner_id, pv), | ||
| Some(Ok(_)) | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn calculate_action_id_returns_some_for_mint() { | ||
| let base = TokenBaseTransition::default(); | ||
| let owner_id = Identifier::from([3u8; 32]); | ||
|
|
||
| let mint = TokenTransition::Mint(TokenMintTransition::V0(TokenMintTransitionV0 { | ||
| base: base.clone(), | ||
| issued_to_identity_id: None, | ||
| amount: 100, | ||
| public_note: None, | ||
| })); | ||
| let pv = PlatformVersion::latest(); | ||
| assert!(matches!( | ||
| mint.calculate_action_id(owner_id, pv), | ||
| Some(Ok(_)) | ||
| )); | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Version-gating tests pinned to latest() instead of explicit version boundaries
Tests throughout the new test files use PlatformVersion::latest() or PlatformVersion::first() for version-sensitive behavior (e.g., action_id hashing that differs between v11/v12). If latest() changes in the future, these tests may silently start testing different version behavior. For version-boundary testing, explicit version constants make the test intent clearer and more resilient to version bumps.
Suggested change
| #[test] | |
| fn calculate_action_id_returns_some_for_burn() { | |
| let base = TokenBaseTransition::default(); | |
| let owner_id = Identifier::from([2u8; 32]); | |
| let burn = TokenTransition::Burn(TokenBurnTransition::V0(TokenBurnTransitionV0 { | |
| base: base.clone(), | |
| burn_amount: 50, | |
| public_note: None, | |
| })); | |
| let pv = PlatformVersion::latest(); | |
| assert!(matches!( | |
| burn.calculate_action_id(owner_id, pv), | |
| Some(Ok(_)) | |
| )); | |
| } | |
| #[test] | |
| fn calculate_action_id_returns_some_for_mint() { | |
| let base = TokenBaseTransition::default(); | |
| let owner_id = Identifier::from([3u8; 32]); | |
| let mint = TokenTransition::Mint(TokenMintTransition::V0(TokenMintTransitionV0 { | |
| base: base.clone(), | |
| issued_to_identity_id: None, | |
| amount: 100, | |
| public_note: None, | |
| })); | |
| let pv = PlatformVersion::latest(); | |
| assert!(matches!( | |
| mint.calculate_action_id(owner_id, pv), | |
| Some(Ok(_)) | |
| )); | |
| } | |
| Where tests exercise version-gated behavior (especially action_id v0 vs v1), consider using explicit platform version constants instead of `latest()`/`first()`. |
source: ['codex-rust-quality']
🤖 Fix this 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/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs`:
- [SUGGESTION] lines 878-911: Version-gating tests pinned to latest() instead of explicit version boundaries
Tests throughout the new test files use `PlatformVersion::latest()` or `PlatformVersion::first()` for version-sensitive behavior (e.g., action_id hashing that differs between v11/v12). If `latest()` changes in the future, these tests may silently start testing different version behavior. For version-boundary testing, explicit version constants make the test intent clearer and more resilient to version bumps.
| fn should_reject_input_below_minimum() { | ||
| let platform_version = PlatformVersion::latest(); | ||
| let mut transition = valid_shield_transition(); | ||
| // Set the input amount to 1 (below minimum of 100_000) |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Hardcoded magic numbers in test comments
Comments like // Set the input amount to 1 (below minimum of 100_000) hardcode values that come from platform_version.dpp.state_transitions.address_funds.min_input_amount. If the constant changes, the comment becomes misleading. This also appears in address_funding_from_asset_lock_transition (line 491) and address_credit_withdrawal_transition (line 622).
Suggested change
| // Set the input amount to 1 (below minimum of 100_000) | |
| Reference the constant name instead: `// below min_input_amount from platform_version`. |
source: ['claude-general']
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
rs-dppstate transitionsassociated_token_event()across all 11 token variants,historical_document_type_name,can_calculate_action_id, document transitionentropy()/data()/revision(), mintrecipient_id()fallback logiccommon_validationhelpers, all 5 shielded transitions (shield, shield_from_asset_lock, shielded_transfer, shielded_withdrawal, unshield) with bounds checking, empty proof/anchor, fee strategy validationTest plan
cargo test -p dpp --lib --all-features— 758 passed, 6 ignored, 0 failedcargo fmt -p dppclean🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Tests
New Features