test(dpp): add validate_structure error path tests for withdrawal and transfer transitions#3124
Open
thepastaclaw wants to merge 5 commits intodashpay:v3.1-devfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive unit tests for validation functions across two state transition modules: address credit withdrawal and identity credit transfer. The test suites cover numerous validation scenarios including edge cases, failure paths, and success cases, with no changes to production code. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs (3)
257-273: Remove redundantuse rand::SeedableRng;inside the function body.
rand::SeedableRngis already imported at the enclosing module level (line 254), so the inneruseat line 258 is redundant.🧹 Suggested cleanup
fn valid_withdrawal_transition() -> AddressCreditWithdrawalTransitionV0 { - use rand::SeedableRng; - let input_amount = 1_000_000_000u64;🤖 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/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs` around lines 257 - 273, In the function valid_withdrawal_transition() remove the redundant inner import "use rand::SeedableRng;" — SeedableRng is already imported at the module level, so delete that line inside the function and keep the existing use of rand::rngs::StdRng::seed_from_u64 unchanged (no other logic needs modification).
571-603:WithdrawalBalanceMismatchErrorbranch is not tested.Production code lines 217–226 return
WithdrawalBalanceMismatchErrorwheninput_sum <= output_amount. None of the new or existing tests exercise this path. A minimal case: setoutputtoSome((different_addr, input_amount))soinput_sum == output_amount.✅ Suggested test to add
#[test] fn should_return_invalid_result_if_output_equals_input_sum() { let platform_version = PlatformVersion::latest(); let mut transition = valid_withdrawal_transition(); // output_amount == input_sum → withdrawal would be zero → balance mismatch let input_amount = *transition.inputs.values().next().unwrap(); transition.output = Some((PlatformAddress::P2pkh([2u8; 20]), input_amount.1)); let result = transition.validate_structure(platform_version); assert_matches!( result.errors.as_slice(), [crate::consensus::ConsensusError::BasicError( BasicError::WithdrawalBalanceMismatchError(_) )] ); }🤖 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/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs` around lines 571 - 603, Add a unit test that exercises the WithdrawalBalanceMismatchError branch by creating a withdrawal transition via valid_withdrawal_transition(), then set transition.output to Some((different address than the input, input_amount)) so that input_sum == output_amount and validate_structure(platform_version) returns the error; assert that result.errors contains a BasicError::WithdrawalBalanceMismatchError coming from validate_structure. Use the existing pattern from other tests (PlatformVersion::latest(), assert_matches! on result.errors.as_slice()) and reference transition.inputs to obtain the input_amount to assign to transition.output.
392-417: The test currently works correctly, but the concern about validation ordering is worth addressing for future robustness.The test uses
PlatformVersion::latest()which returns v12, usingSTATE_TRANSITION_VERSIONS_V3withmax_address_inputs = 16andmax_address_fee_strategies = 4. Since the test creates 5 inputs (step_count = 4 + 1), it safely passes the inputs check (5 ≤ 16) and correctly triggersFeeStrategyTooManyStepsErrorat the fee-strategy check.However, your concern is valid from a maintainability perspective. The validation order at lines 39 and 82 means that if these constants ever become misaligned (i.e.,
max_address_fee_strategies >= max_address_inputs), the test would silently pass the wrong error. While all current versions (v1, v2, v3) are safe, consider adding an explicit assertion or comment documenting the required invariant:max_address_fee_strategies < max_address_inputs.🤖 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/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs` around lines 392 - 417, The test should assert the invariant that max_address_fee_strategies < max_address_inputs so the fee-strategy check runs after the inputs-count check; inside should_return_invalid_result_if_fee_strategy_too_many_steps, after computing max_fee_strategies and step_count, read max_address_inputs from PlatformVersion::latest().dpp.state_transitions.max_address_inputs and add a short assertion like debug_assert!(max_fee_strategies < max_address_inputs) (or a plain assert!() in the test) and/or a clarifying comment referencing max_address_fee_strategies and max_address_inputs to prevent future regressions where validation ordering could make the test assert the wrong error.packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_to_addresses_transition/v0/state_transition_validation.rs (2)
100-117: Address encoding only covers 2 bytes — collapses ifmax_address_outputsever reaches 65 536.
(i as u16)silently truncates, so entries withi = 0andi = 65 536(etc.) produce the same 20-byte key.BTreeMapdeduplicates them, the map ends up with fewer thanmax + 1entries, and the test would never reachTransitionOverMaxOutputsError. The withdrawal file'sp2pkh_addresshelper avoids this by spreading the index across 4 bytes; the same pattern should be used here.🛡️ Proposed fix
- for i in 0..=max { - let mut addr = [0u8; 20]; - addr[0..2].copy_from_slice(&(i as u16).to_le_bytes()); - recipient_addresses.insert(PlatformAddress::P2pkh(addr), 1_000_000_000); - } + for i in 0..=max { + let mut addr = [0u8; 20]; + let b = (i as u32).to_le_bytes(); + addr[0..4].copy_from_slice(&b); + recipient_addresses.insert(PlatformAddress::P2pkh(addr), 1_000_000_000); + }🤖 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/identity/identity_credit_transfer_to_addresses_transition/v0/state_transition_validation.rs` around lines 100 - 117, The test builds recipient_addresses with PlatformAddress::P2pkh keys but encodes the loop index into only 2 bytes (addr[0..2].copy_from_slice(&(i as u16).to_le_bytes())), which will wrap at 65_536 and cause BTreeMap deduplication; update the address encoding to use 4 bytes (e.g., addr[0..4].copy_from_slice(&(i as u32).to_le_bytes())) or reuse the existing p2pkh_address helper so each i produces a unique 20-byte key; update the construction in the IdentityCreditTransferToAddressesTransitionV0 test to use the widened index encoding for recipient_addresses so the map actually grows to max + 1 entries and triggers TransitionOverMaxOutputsError.
135-149: Useassert_matches!consistently with every other test in the module.All other tests (and the parallel withdrawal suite) use
assert_matches!(result.errors.as_slice(), [...])for both error and success assertions. The happy-path test here usesassert!with a manual{:?}format, which is inconsistent.♻️ Proposed refactor
- assert!( - result.errors.is_empty(), - "expected valid result, got: {:?}", - result.errors - ); + assert_matches!(result.errors.as_slice(), []);🤖 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/identity/identity_credit_transfer_to_addresses_transition/v0/state_transition_validation.rs` around lines 135 - 149, The test should use the same assert_matches! style as the other tests: in should_return_valid_result_for_valid_transition replace the assert!(result.errors.is_empty(), ...) with assert_matches!(result.errors.as_slice(), []), ensuring the module imports/uses the assert_matches! macro and matches the empty-slice pattern for success; locate the test function should_return_valid_result_for_valid_transition and update its assertion to assert_matches!(result.errors.as_slice(), []) to keep consistency with the other tests that assert errors via result.errors.as_slice().
🤖 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/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs`:
- Around line 257-273: In the function valid_withdrawal_transition() remove the
redundant inner import "use rand::SeedableRng;" — SeedableRng is already
imported at the module level, so delete that line inside the function and keep
the existing use of rand::rngs::StdRng::seed_from_u64 unchanged (no other logic
needs modification).
- Around line 571-603: Add a unit test that exercises the
WithdrawalBalanceMismatchError branch by creating a withdrawal transition via
valid_withdrawal_transition(), then set transition.output to Some((different
address than the input, input_amount)) so that input_sum == output_amount and
validate_structure(platform_version) returns the error; assert that
result.errors contains a BasicError::WithdrawalBalanceMismatchError coming from
validate_structure. Use the existing pattern from other tests
(PlatformVersion::latest(), assert_matches! on result.errors.as_slice()) and
reference transition.inputs to obtain the input_amount to assign to
transition.output.
- Around line 392-417: The test should assert the invariant that
max_address_fee_strategies < max_address_inputs so the fee-strategy check runs
after the inputs-count check; inside
should_return_invalid_result_if_fee_strategy_too_many_steps, after computing
max_fee_strategies and step_count, read max_address_inputs from
PlatformVersion::latest().dpp.state_transitions.max_address_inputs and add a
short assertion like debug_assert!(max_fee_strategies < max_address_inputs) (or
a plain assert!() in the test) and/or a clarifying comment referencing
max_address_fee_strategies and max_address_inputs to prevent future regressions
where validation ordering could make the test assert the wrong error.
In
`@packages/rs-dpp/src/state_transition/state_transitions/identity/identity_credit_transfer_to_addresses_transition/v0/state_transition_validation.rs`:
- Around line 100-117: The test builds recipient_addresses with
PlatformAddress::P2pkh keys but encodes the loop index into only 2 bytes
(addr[0..2].copy_from_slice(&(i as u16).to_le_bytes())), which will wrap at
65_536 and cause BTreeMap deduplication; update the address encoding to use 4
bytes (e.g., addr[0..4].copy_from_slice(&(i as u32).to_le_bytes())) or reuse the
existing p2pkh_address helper so each i produces a unique 20-byte key; update
the construction in the IdentityCreditTransferToAddressesTransitionV0 test to
use the widened index encoding for recipient_addresses so the map actually grows
to max + 1 entries and triggers TransitionOverMaxOutputsError.
- Around line 135-149: The test should use the same assert_matches! style as the
other tests: in should_return_valid_result_for_valid_transition replace the
assert!(result.errors.is_empty(), ...) with
assert_matches!(result.errors.as_slice(), []), ensuring the module imports/uses
the assert_matches! macro and matches the empty-slice pattern for success;
locate the test function should_return_valid_result_for_valid_transition and
update its assertion to assert_matches!(result.errors.as_slice(), []) to keep
consistency with the other tests that assert errors via
result.errors.as_slice().
thepastaclaw
force-pushed
the
test/validation-error-paths-transitions
branch
from
4a849f1 to
d0c6914
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced Feb 20, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs (3)
257-295:p2pkh_address: preferto_le_bytes()over manual bit extraction.The same address-generation pattern in the transfer file (line 103) uses
(i as u32).to_le_bytes(), which is cleaner and less error-prone.♻️ Suggested simplification
fn p2pkh_address(index: usize) -> PlatformAddress { let mut bytes = [0u8; 20]; - bytes[0] = (index & 0xff) as u8; - bytes[1] = ((index >> 8) & 0xff) as u8; - bytes[2] = ((index >> 16) & 0xff) as u8; - bytes[3] = ((index >> 24) & 0xff) as u8; + bytes[0..4].copy_from_slice(&(index as u32).to_le_bytes()); PlatformAddress::P2pkh(bytes) }🤖 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/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs` around lines 257 - 295, Replace the manual bit extraction in p2pkh_address with a little-endian byte conversion to simplify and make it consistent with the transfer code: compute the 4-byte little-endian representation of index (e.g., (index as u32).to_le_bytes()) and copy those bytes into the first four slots of the 20-byte array in p2pkh_address; ensure behavior of make_inputs_and_witnesses (which relies on p2pkh_address and casts i to u32) remains unchanged so tests continue to generate the same addresses.
430-448: Missing defensive ordering assertion inshould_return_invalid_result_if_fee_strategy_duplicate.The test implicitly requires
max_address_fee_strategies >= 2. If that constant is ever set to1, theFeeStrategyTooManyStepsErrorcheck fires first, and the test would pass for the wrong reason. The sibling testshould_return_invalid_result_if_fee_strategy_too_many_steps(lines 403–409) already establishes the pattern of making this class of ordering invariant explicit with anassert!. Add the same guard here for consistency.🛡️ Suggested fix
#[test] fn should_return_invalid_result_if_fee_strategy_duplicate() { let platform_version = PlatformVersion::latest(); + let max_fee_strategies = platform_version + .dpp + .state_transitions + .max_address_fee_strategies as usize; + assert!( + max_fee_strategies >= 2, + "test requires max_address_fee_strategies >= 2, got {}", + max_fee_strategies + ); let mut transition = valid_withdrawal_transition();🤖 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/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs` around lines 430 - 448, The test should_return_invalid_result_if_fee_strategy_duplicate can spuriously pass if max_address_fee_strategies == 1 because FeeStrategyTooManyStepsError would trigger first; add a defensive assert to ensure max_address_fee_strategies >= 2 before building the duplicate fee strategy. Locate the test function should_return_invalid_result_if_fee_strategy_duplicate and insert an assert!(max_address_fee_strategies() >= 2) (or assert!(MAX_ADDRESS_FEE_STRATEGIES >= 2) matching the constant/accessor used elsewhere) at the top of the test so the test verifies the duplicate-step error rather than the too-many-steps error.
586-608:WithdrawalBelowMinAmountErrorabove-max path not covered.The production code at lines 230–231 uses a combined condition:
if withdrawal_amount < MIN_WITHDRAWAL_AMOUNT || withdrawal_amount > platform_version.system_limits.max_withdrawal_amount
should_return_invalid_result_if_withdrawal_below_minexercises the< MIN_WITHDRAWAL_AMOUNTbranch. The> max_withdrawal_amountbranch has no test. Consider adding:#[test] fn should_return_invalid_result_if_withdrawal_above_max() { let platform_version = PlatformVersion::latest(); let max = platform_version.system_limits.max_withdrawal_amount; let mut transition = valid_withdrawal_transition(); // Use a single input just over max; no change output so withdrawal == input. transition.inputs = [(PlatformAddress::P2pkh([1u8; 20]), (0, max + 1))].into(); transition.output = None; let result = transition.validate_structure(platform_version); assert_matches!( result.errors.as_slice(), [crate::consensus::ConsensusError::BasicError( BasicError::WithdrawalBelowMinAmountError(_) )] ); }🤖 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/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs` around lines 586 - 608, Add a new unit test mirroring should_return_invalid_result_if_withdrawal_below_min that exercises the > max_withdrawal_amount branch: create a PlatformVersion::latest(), read max from platform_version.system_limits.max_withdrawal_amount, build a transition via valid_withdrawal_transition(), set transition.inputs to a single input with amount = max + 1 and transition.output = None, call transition.validate_structure(platform_version) and assert the returned errors slice contains the same crate::consensus::ConsensusError::BasicError(BasicError::WithdrawalBelowMinAmountError(_)) variant (use the existing valid_withdrawal_transition and validate_structure symbols to locate code).
🤖 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/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs`:
- Around line 257-295: Replace the manual bit extraction in p2pkh_address with a
little-endian byte conversion to simplify and make it consistent with the
transfer code: compute the 4-byte little-endian representation of index (e.g.,
(index as u32).to_le_bytes()) and copy those bytes into the first four slots of
the 20-byte array in p2pkh_address; ensure behavior of make_inputs_and_witnesses
(which relies on p2pkh_address and casts i to u32) remains unchanged so tests
continue to generate the same addresses.
- Around line 430-448: The test
should_return_invalid_result_if_fee_strategy_duplicate can spuriously pass if
max_address_fee_strategies == 1 because FeeStrategyTooManyStepsError would
trigger first; add a defensive assert to ensure max_address_fee_strategies >= 2
before building the duplicate fee strategy. Locate the test function
should_return_invalid_result_if_fee_strategy_duplicate and insert an
assert!(max_address_fee_strategies() >= 2) (or
assert!(MAX_ADDRESS_FEE_STRATEGIES >= 2) matching the constant/accessor used
elsewhere) at the top of the test so the test verifies the duplicate-step error
rather than the too-many-steps error.
- Around line 586-608: Add a new unit test mirroring
should_return_invalid_result_if_withdrawal_below_min that exercises the >
max_withdrawal_amount branch: create a PlatformVersion::latest(), read max from
platform_version.system_limits.max_withdrawal_amount, build a transition via
valid_withdrawal_transition(), set transition.inputs to a single input with
amount = max + 1 and transition.output = None, call
transition.validate_structure(platform_version) and assert the returned errors
slice contains the same
crate::consensus::ConsensusError::BasicError(BasicError::WithdrawalBelowMinAmountError(_))
variant (use the existing valid_withdrawal_transition and validate_structure
symbols to locate code).
- Remove redundant `use rand::SeedableRng` inside valid_withdrawal_transition() - Add WithdrawalBalanceMismatchError test (output == input_sum path) - Assert max_address_fee_strategies < max_address_inputs invariant in fee strategy test - Use u32 (4 bytes) for address index encoding in transfer max-recipients test - Use assert_matches! consistently in transfer happy-path test
thepastaclaw
force-pushed
the
test/validation-error-paths-transitions
branch
from
fb98fbb to
c00a7f9
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs (3)
430-448: Add a guard assertion consistent with the adjacentfee_strategy_too_many_stepstest.This test sets 2 fee-strategy steps, but the too-many-steps check runs before the duplicate check in the production code. If
max_address_fee_strategiesis ever set to 1, validation would returnFeeStrategyTooManyStepsErrorinstead ofFeeStrategyDuplicateError, making theassert_matches!fail for the wrong reason.The adjacent
should_return_invalid_result_if_fee_strategy_too_many_stepstest guards against exactly this kind of ordering assumption. Apply the same pattern here:🛡️ Proposed guard
fn should_return_invalid_result_if_fee_strategy_duplicate() { let platform_version = PlatformVersion::latest(); + let max_fee_strategies = platform_version + .dpp + .state_transitions + .max_address_fee_strategies as usize; + assert!( + max_fee_strategies >= 2, + "max_address_fee_strategies ({}) must be >= 2 for this test to reach the duplicate check", + max_fee_strategies + ); let mut transition = valid_withdrawal_transition();🤖 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/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs` around lines 430 - 448, The test should include a guard assertion to ensure the max allowed fee strategy steps is greater than the number used in the test so the duplicate-check path runs; in the should_return_invalid_result_if_fee_strategy_duplicate test, fetch the PlatformVersion constant (max_address_fee_strategies) and assert it is > transition.fee_strategy.len() (or > 2) before calling validate_structure so the test cannot accidentally hit FeeStrategyTooManyStepsError instead of FeeStrategyDuplicateError.
450-465:ReduceOutputvariant ofFeeStrategyIndexOutOfBoundsErroris not covered.The production code has two arms:
DeductFromInputandReduceOutput(lines 108–136). OnlyDeductFromInput(999)is tested here. TheReduceOutput(0)case withoutput: None(output_count = 0, so0 >= 0fires the same error) is an uncovered branch.📋 Suggested additional test
#[test] fn should_return_invalid_result_if_fee_strategy_reduce_output_index_out_of_bounds() { let platform_version = PlatformVersion::latest(); let mut transition = valid_withdrawal_transition(); // output is None (output_count = 0), so any ReduceOutput index is out of bounds transition.fee_strategy = vec![AddressFundsFeeStrategyStep::ReduceOutput(0)]; let result = transition.validate_structure(platform_version); assert_matches!( result.errors.as_slice(), [crate::consensus::ConsensusError::BasicError( BasicError::FeeStrategyIndexOutOfBoundsError(_) )] ); }🤖 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/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs` around lines 450 - 465, Add a test that covers the ReduceOutput branch of AddressFundsFeeStrategyStep by creating a valid_withdrawal_transition with no outputs (output_count = 0), setting transition.fee_strategy = vec![AddressFundsFeeStrategyStep::ReduceOutput(0)], calling transition.validate_structure(PlatformVersion::latest()), and asserting the resulting error matches BasicError::FeeStrategyIndexOutOfBoundsError; name the test something like should_return_invalid_result_if_fee_strategy_reduce_output_index_out_of_bounds and mirror the existing DeductFromInput test structure to ensure the ReduceOutput arm is exercised.
586-608: Missing coverage for the above-max_withdrawal_amountbranch.The production condition at line 230–231 fires
WithdrawalBelowMinAmountErrorfor two distinct cases:withdrawal_amount < MIN_WITHDRAWAL_AMOUNTandwithdrawal_amount > max_withdrawal_amount. This test covers only the below-min path. The above-max path is an untested branch.📋 Suggested additional test
#[test] fn should_return_invalid_result_if_withdrawal_above_max() { let platform_version = PlatformVersion::latest(); let max_withdrawal = platform_version.system_limits.max_withdrawal_amount; let input_amount = max_withdrawal + 1; let mut transition = valid_withdrawal_transition(); transition.inputs = [(PlatformAddress::P2pkh([1u8; 20]), (0, input_amount))].into(); transition.output = None; let result = transition.validate_structure(platform_version); assert_matches!( result.errors.as_slice(), [crate::consensus::ConsensusError::BasicError( BasicError::WithdrawalBelowMinAmountError(_) )] ); }🤖 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/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs` around lines 586 - 608, Add a new unit test that mirrors should_return_invalid_result_if_withdrawal_below_min but sets an input amount above the configured max withdrawal (use PlatformVersion::latest() and platform_version.system_limits.max_withdrawal_amount), e.g. input_amount = max_withdrawal + 1, set transition.inputs and transition.output = None on valid_withdrawal_transition(), call transition.validate_structure(platform_version), and assert the returned error slice contains crate::consensus::ConsensusError::BasicError(BasicError::WithdrawalBelowMinAmountError(_)); this ensures the branch in validate_structure that treats withdrawal_amount > max_withdrawal_amount is covered.
🤖 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/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs`:
- Around line 430-448: The test should include a guard assertion to ensure the
max allowed fee strategy steps is greater than the number used in the test so
the duplicate-check path runs; in the
should_return_invalid_result_if_fee_strategy_duplicate test, fetch the
PlatformVersion constant (max_address_fee_strategies) and assert it is >
transition.fee_strategy.len() (or > 2) before calling validate_structure so the
test cannot accidentally hit FeeStrategyTooManyStepsError instead of
FeeStrategyDuplicateError.
- Around line 450-465: Add a test that covers the ReduceOutput branch of
AddressFundsFeeStrategyStep by creating a valid_withdrawal_transition with no
outputs (output_count = 0), setting transition.fee_strategy =
vec![AddressFundsFeeStrategyStep::ReduceOutput(0)], calling
transition.validate_structure(PlatformVersion::latest()), and asserting the
resulting error matches BasicError::FeeStrategyIndexOutOfBoundsError; name the
test something like
should_return_invalid_result_if_fee_strategy_reduce_output_index_out_of_bounds
and mirror the existing DeductFromInput test structure to ensure the
ReduceOutput arm is exercised.
- Around line 586-608: Add a new unit test that mirrors
should_return_invalid_result_if_withdrawal_below_min but sets an input amount
above the configured max withdrawal (use PlatformVersion::latest() and
platform_version.system_limits.max_withdrawal_amount), e.g. input_amount =
max_withdrawal + 1, set transition.inputs and transition.output = None on
valid_withdrawal_transition(), call
transition.validate_structure(platform_version), and assert the returned error
slice contains
crate::consensus::ConsensusError::BasicError(BasicError::WithdrawalBelowMinAmountError(_));
this ensures the branch in validate_structure that treats withdrawal_amount >
max_withdrawal_amount is covered.
PastaPastaPasta
approved these changes
Feb 22, 2026
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.
Issue Being Fixed
Tracker: thepastaclaw/tracker#14 (parent: #11)
Addresses findings I-03 and I-04 from the rs-dpp test quality audit.
What was done
Added missing validation error path tests for two state transition
validate_structuremethods.I-04:
IdentityCreditTransferToAddressesTransitionV0::validate_structure— 1 test → 5 testsshould_return_invalid_result_if_no_recipients— empty recipient map →TransitionNoOutputsErrorshould_return_invalid_result_if_over_max_recipients— max+1 recipients →TransitionOverMaxOutputsErrorshould_return_invalid_result_if_amount_below_minimum— amount=1 →OutputBelowMinimumErrorshould_return_valid_result_for_valid_transition— happy path with min_output_amountshould_return_invalid_result_if_recipient_sum_overflowsI-03:
AddressCreditWithdrawalTransitionV0::validate_structure— 1 test → 16 testsAdded a
valid_withdrawal_transition()baseline helper, then one-mutation-per-test:should_return_invalid_result_if_no_inputs→TransitionNoInputsErrorshould_return_invalid_result_if_over_max_inputs→TransitionOverMaxInputsErrorshould_return_invalid_result_if_witness_count_mismatch→InputWitnessCountMismatchErrorshould_return_invalid_result_if_output_address_also_input→OutputAddressAlsoInputErrorshould_return_invalid_result_if_fee_strategy_empty→FeeStrategyEmptyErrorshould_return_invalid_result_if_fee_strategy_too_many_steps→FeeStrategyTooManyStepsErrorshould_return_invalid_result_if_fee_strategy_duplicate→FeeStrategyDuplicateErrorshould_return_invalid_result_if_fee_strategy_index_out_of_bounds→FeeStrategyIndexOutOfBoundsErrorshould_return_invalid_result_if_input_below_minimum→InputBelowMinimumErrorshould_return_invalid_result_if_output_below_minimum→OutputBelowMinimumErrorshould_return_invalid_result_if_pooling_not_never→NotImplementedCreditWithdrawalTransitionPoolingErrorshould_return_invalid_result_if_core_fee_not_fibonacci→InvalidCreditWithdrawalTransitionCoreFeeErrorshould_return_invalid_result_if_output_script_invalid→InvalidCreditWithdrawalTransitionOutputScriptErrorshould_return_invalid_result_if_withdrawal_below_min→WithdrawalBelowMinAmountErrorshould_return_valid_result_for_valid_transition— happy pathshould_return_invalid_result_if_input_sum_overflowsAll tests compile and pass.
Summary by CodeRabbit