test(dpp): add validation error path tests for identity_nonce, max_depth, and GroupV0#3123

Closed

thepastaclaw wants to merge 5 commits intodashpay:v3.1-devfrom

thepastaclaw:test/validation-error-paths-simple

Closed

test(dpp): add validation error path tests for identity_nonce, max_depth, and GroupV0#3123
thepastaclaw wants to merge 5 commits intodashpay:v3.1-devfrom
thepastaclaw:test/validation-error-paths-simple

Conversation

Copy link

Collaborator

thepastaclaw commented Feb 20, 2026 *

edited

Loading

Issue Being Fixed

Tracker: thepastaclaw/tracker#14 (parent: #11)

Addresses findings C-07, I-01, and I-05 from the rs-dpp test quality audit.

What was done

Added missing validation error path tests for three validators that had little to no test coverage:

C-07: `validate_new_identity_nonce` -- 0 tests - 5 tests

validate_new_identity_nonce_valid_zero -- nonce=0, valid
validate_new_identity_nonce_valid_max_minus_one -- nonce=23 (boundary), valid
validate_new_identity_nonce_invalid_at_max -- nonce=24 (boundary), returns NonceTooFarInPast
validate_new_identity_nonce_invalid_above_max -- nonce=25, returns error
validate_new_identity_nonce_invalid_large -- nonce=1000, returns error

I-01: `validate_max_depth_v0` -- max-depth error path never tested - 1 test

should_return_error_when_max_depth_exceeded -- dynamically builds a schema exceeding max_depth from platform version, asserts DataContractMaxDepthExceedError

I-05: `GroupV0::validate` -- 1 test - 11 tests

test_group_exceeds_max_members -- too many members
test_group_too_few_members_zero -- empty group
test_group_too_few_members_one -- single member
test_group_member_has_power_of_zero -- member with power 0
test_group_member_power_over_limit -- power > GROUP_POWER_LIMIT (u16::MAX)
test_group_member_power_exceeds_required -- power > required_power
test_group_total_power_less_than_required -- insufficient total power
test_group_non_unilateral_member_power_less_than_required -- non-unilateral members can't meet threshold
test_group_required_power_zero -- documents that required_power=0 hits power > required_power before the invalid-power check
test_group_required_power_over_limit -- required_power > GROUP_POWER_LIMIT (65536), reached via 3 members at max power

All tests compile and pass (cargo test -p dpp --lib -- 32 targeted tests, 0 failures).

Summary by CodeRabbit

Tests
- Added comprehensive unit tests covering document-schema max-depth enforcement, group validation edge cases (member counts, power constraints, and related error scenarios), and identity nonce boundary checks across valid and invalid ranges.

Validation

What was tested
- cargo test -p dpp --lib
Results
- 32 targeted tests executed
- 0 failures
Environment
- Local development environment for dashpay/platform (Rust test run)

thepastaclaw requested a review from QuantumExplorer as a code owner

February 20, 2026 20:10

Copy link

Contributor

coderabbitai bot commented Feb 20, 2026 *

edited

Loading

Warning

Rate limit exceeded

@QuantumExplorer has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 31 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

i Review info

Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ac8fb56f-f5ce-443a-94a5-c9ffbaf01eda

Commits

Reviewing files that changed from the base of the PR and between db23345 and b61b132.

Files selected for processing (2)

packages/rs-dpp/src/data_contract/document_type/schema/validate_max_depth/v0/mod.rs
packages/rs-dpp/src/data_contract/group/v0/mod.rs

Walkthrough

Adds unit tests for three validators--max-depth schema validation, GroupV0::validate, and validate_new_identity_nonce--that exercise error branches and boundary conditions without changing public APIs.

Changes

Cohort / File(s)	Summary
Max Depth Validation Tests `packages/rs-dpp/src/data_contract/document_type/schema/validate_max_depth/v0/mod.rs`	Adds a unit test that builds a nested JSON schema up to the configured max depth, invokes `validate_max_depth_v0`, and asserts the first error is `DataContractMaxDepthExceedError` with the reported `max_depth` equal to the configured maximum.
Group Validation Tests `packages/rs-dpp/src/data_contract/group/v0/mod.rs`	Adds an extensive test suite for `GroupV0::validate` covering exceeding max members, too few members, various power constraints, unilateral power cases, and invalid `required_power`; tests assert specific `ConsensusError::BasicError` variants in results.
Identity Nonce Validation Tests `packages/rs-dpp/src/identity/identity_nonce.rs`	Adds tests for `validate_new_identity_nonce` covering zero, max-1, at max, above max, and very large nonces; expects valid ranges to pass and out-of-range values to produce `NonceTooFarInPast` errors.

Estimated code review effort

3 (Moderate) | ~20 minutes

Poem

I hopped through tests with eager eyes,

Depths, groups, nonces -- all boundary-wise.
I counted members, measured depth with care,
Chased errant nonces here and there,
A tiny thump -- the suite is fair.

Pre-merge checks | 5

Passed checks (5 passed)

Check name	Status	Explanation
Title check	Passed	The title accurately describes the main change: adding validation error path tests for three specific validators (identity_nonce, max_depth, and GroupV0).
Linked Issues check	Passed	The PR successfully addresses all three findings from issue `#14`: C-07 adds 5 identity_nonce tests, I-01 adds max_depth_v0 test, and I-05 adds 10 GroupV0::validate tests covering error branches.
Out of Scope Changes check	Passed	All changes are focused test additions directly addressing the three linked issues; no unrelated modifications or scope creep detected.
Docstring Coverage	Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	Passed	Check skipped - CodeRabbit's high-level summary is enabled.

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

Finishing Touches

Generate unit tests (beta)

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

Coding Plan

Generate coding plan for human review comments

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 help to get the list of available commands and usage tips.}

coderabbitai bot reviewed

Feb 20, 2026

View reviewed changes

Copy link

Contributor

coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Nitpick comments (1)

packages/rs-dpp/src/identity/identity_nonce.rs (1)

189-226: Consider asserting additional error fields and replacing the magic number.

Two small improvements for completeness:

Magic literal in validate_new_identity_nonce_invalid_large -- 1000 is an arbitrary sentinel with no expressed relationship to MISSING_IDENTITY_REVISIONS_MAX_BYTES. A constant expression makes the intent self-documenting:

Suggested change for magic literal

- let nonce = 1000; + let nonce = MISSING_IDENTITY_REVISIONS_MAX_BYTES * 42; // well above the limit

Incomplete error field assertions -- the three invalid tests only assert e.error. The struct also carries setting_identity_nonce (should equal the input nonce) and current_identity_nonce (should be None for a new identity). Adding these assertions would fully pin the error payload and catch regressions in those fields:

Suggested additional assertions (shown for the `at_max` test; pattern applies to all three)

assert_eq!(e.error, MergeIdentityNonceResult::NonceTooFarInPast); + assert_eq!(e.setting_identity_nonce, nonce); + assert!(e.current_identity_nonce.is_none());

Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/identity/identity_nonce.rs` around lines 189 - 226, Replace the magic literal in validate_new_identity_nonce_invalid_large with a value derived from MISSING_IDENTITY_REVISIONS_MAX_BYTES (e.g. MISSING_IDENTITY_REVISIONS_MAX_BYTES * 2) so the relationship to the boundary is explicit, and in all three failing tests (validate_new_identity_nonce_invalid_at_max, validate_new_identity_nonce_invalid_above_max, validate_new_identity_nonce_invalid_large) add assertions on the returned InvalidIdentityNonceError fields: assert e.error == MergeIdentityNonceResult::NonceTooFarInPast, assert e.setting_identity_nonce == nonce, and assert e.current_identity_nonce.is_none() to fully pin the error payload from validate_new_identity_nonce.

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/data_contract/group/v0/mod.rs`: - Around line 360-379: The check for self.required_power == 0 in GroupV0::validate is unreachable because the per-member validation (the power > self.required_power check in the member loop) will always fire first when required_power == 0; either remove the unreachable branch that returns GroupRequiredPowerIsInvalidError or refactor the per-member check so it doesn't preempt the required_power == 0 validation (e.g., validate required_power early in validate before iterating members); update references in tests that assert GroupMemberHasPowerOverLimitError vs GroupRequiredPowerIsInvalidError to match the chosen behavior. --- Nitpick comments: In `@packages/rs-dpp/src/identity/identity_nonce.rs`: - Around line 189-226: Replace the magic literal in validate_new_identity_nonce_invalid_large with a value derived from MISSING_IDENTITY_REVISIONS_MAX_BYTES (e.g. MISSING_IDENTITY_REVISIONS_MAX_BYTES * 2) so the relationship to the boundary is explicit, and in all three failing tests (validate_new_identity_nonce_invalid_at_max, validate_new_identity_nonce_invalid_above_max, validate_new_identity_nonce_invalid_large) add assertions on the returned InvalidIdentityNonceError fields: assert e.error == MergeIdentityNonceResult::NonceTooFarInPast, assert e.setting_identity_nonce == nonce, and assert e.current_identity_nonce.is_none() to fully pin the error payload from validate_new_identity_nonce.

packages/rs-dpp/src/data_contract/group/v0/mod.rs Show resolved Hide resolved

thepastaclaw mentioned this pull request Feb 20, 2026

rs-dpp: add missing validation error path tests (6+ validators at 10-30% branch coverage) thepastaclaw/tracker#14

Closed

thepastaclaw force-pushed the test/validation-error-paths-simple branch from e77aa73 to 2ea57b2 Compare

February 20, 2026 21:39

Copy link

Collaborator Author

thepastaclaw commented Feb 20, 2026

@coderabbitai review

Copy link

Contributor

coderabbitai bot commented Feb 20, 2026

Actions performed

Review triggered.

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

This was referenced Feb 20, 2026

CI: cargo fmt failures on PRs #3123, #3124, #3125, #3129 thepastaclaw/tracker#42

Closed

[GH] CI Failure: dashpay/platform -- Tests workflow failed on test/validation-error-paths-simple branch thepastaclaw/tracker#39

Closed

coderabbitai bot reviewed

Feb 21, 2026

View reviewed changes

Copy link

Contributor

coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed. Duplicate comments: In `@packages/rs-dpp/src/data_contract/group/v0/mod.rs`: - Around line 358-376: This test correctly documents current behavior and requires no code changes: leave the test in mod.rs as-is, keeping the inline comment and the assertion that GroupV0::validate produces a BasicError::GroupMemberHasPowerOverLimitError when required_power == 0; ensure references to the two relevant errors (GroupMemberHasPowerOverLimitError and GroupRequiredPowerIsInvalidError) and the GroupV0::validate function remain unchanged so future fixes to validation order will surface as test failures requiring assertion updates.

This was referenced Feb 21, 2026

[CI-GLOBAL] dashpay/platform -- JS NPM security audit thepastaclaw/tracker#56

Closed

[CI-GLOBAL] dashpay/platform -- assign-milestone permission thepastaclaw/tracker#67

Closed

thepastaclaw force-pushed the test/validation-error-paths-simple branch from 4f89e9a to db23345 Compare

February 21, 2026 18:02

github-actions bot added this to the v3.1.0 milestone Feb 21, 2026

coderabbitai bot reviewed

Feb 21, 2026

View reviewed changes

Copy link

Contributor

coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick comments (1)

packages/rs-dpp/src/identity/identity_nonce.rs (1)

215-226: validate_new_identity_nonce_invalid_large is redundant with validate_new_identity_nonce_invalid_above_max.

validate_new_identity_nonce has exactly two branches (>= MAX / < MAX). Both are fully covered by valid_zero and invalid_at_max; neither invalid_above_max nor invalid_large add new coverage. Consider dropping invalid_large or replacing it with a semantically distinct case (e.g., max u64 to document overflow behaviour isn't possible).

Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/identity/identity_nonce.rs` around lines 215 - 226, The test validate_new_identity_nonce_invalid_large duplicates coverage of validate_new_identity_nonce already exercised by validate_new_identity_nonce_invalid_above_max and valid_zero; remove validate_new_identity_nonce_invalid_large or replace it with a semantically distinct scenario (e.g., use u64::MAX as the nonce to document overflow/edge behavior) so it adds unique coverage; update or delete the test function validate_new_identity_nonce_invalid_large accordingly and ensure references/assertions still target validate_new_identity_nonce and MergeIdentityNonceResult::NonceTooFarInPast if keeping a meaningful new case.

Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed. Duplicate comments: In `@packages/rs-dpp/src/data_contract/group/v0/mod.rs`: - Around line 358-376: The validate logic in GroupV0 currently checks per-member power limits before validating required_power, making the required_power == 0 branch (which should produce GroupRequiredPowerIsInvalidError) unreachable and leaving dead code; update GroupV0::validate to validate required_power (and emit GroupRequiredPowerIsInvalidError when required_power == 0 or otherwise invalid) before iterating members for GroupMemberHasPowerOverLimitError so the intended error path is reachable, and adjust the tests to expect GroupRequiredPowerIsInvalidError for the zero required_power case or keep the existing test as a regression test if you prefer to assert the new behavior. --- Nitpick comments: In `@packages/rs-dpp/src/identity/identity_nonce.rs`: - Around line 215-226: The test validate_new_identity_nonce_invalid_large duplicates coverage of validate_new_identity_nonce already exercised by validate_new_identity_nonce_invalid_above_max and valid_zero; remove validate_new_identity_nonce_invalid_large or replace it with a semantically distinct scenario (e.g., use u64::MAX as the nonce to document overflow/edge behavior) so it adds unique coverage; update or delete the test function validate_new_identity_nonce_invalid_large accordingly and ensure references/assertions still target validate_new_identity_nonce and MergeIdentityNonceResult::NonceTooFarInPast if keeping a meaningful new case.

thepastaclaw mentioned this pull request Feb 23, 2026

[PR-VALIDATION] dashpay/platform#3123 -- add Validation section thepastaclaw/tracker#162

Closed

thepastaclaw marked this pull request as draft February 25, 2026 08:21

thepastaclaw mentioned this pull request Feb 25, 2026

[CI-GLOBAL] dashpay/platform -- JS NPM security audit failing (high/moderate vulns) thepastaclaw/tracker#295

Closed

thepastaclaw force-pushed the test/validation-error-paths-simple branch from db23345 to 4d6476c Compare

February 25, 2026 16:22

thepastaclaw mentioned this pull request Feb 25, 2026

[CI] dashpay/platform#3123 -- CI failure thepastaclaw/tracker#322

Closed

thepastaclaw force-pushed the test/validation-error-paths-simple branch from 4d6476c to 6327c5c Compare

March 12, 2026 17:47

thepastaclaw added 4 commits March 12, 2026 15:17


 test(dpp): add validate_new_identity_nonce tests for all branches (C-07)

ee44a55


 test(dpp): add max-depth-exceeded error path test for validate_max_de...

1fcad6b

...pth_v0 (I-01)


 test(dpp): add GroupV0::validate error path tests for all branches (I...

f5b6a05

...-05)


 style(dpp): apply cargo fmt formatting to GroupV0 tests

b61b132

thepastaclaw force-pushed the test/validation-error-paths-simple branch from 6327c5c to b61b132 Compare

March 12, 2026 20:18

QuantumExplorer marked this pull request as ready for review March 15, 2026 18:52


 Merge branch 'v3.1-dev' into test/validation-error-paths-simple

679d3b5

Copy link

Member

QuantumExplorer commented Mar 15, 2026

Closing to recreate from the main repo so we get coverage reports (fork PRs can't upload to Codecov). The tests are good -- will be preserved in the new PR. Thanks for the contribution!

QuantumExplorer closed this Mar 15, 2026

QuantumExplorer added a commit that referenced this pull request Mar 15, 2026


 test(dpp): add validation error path tests for identity_nonce, max_de...

993abd2

...pth, and GroupV0 Add 16 tests covering boundary conditions and specific error variants: - validate_new_identity_nonce: valid/invalid at boundary values - validate_max_depth: schema exceeding max depth - GroupV0::validate: max members, too few members, power limits, required power validation Originally authored by thepastaclaw in PR #3123, recreated from main repo for coverage reporting. Co-Authored-By: thepastaclaw Co-Authored-By: Claude Opus 4.6

QuantumExplorer mentioned this pull request Mar 15, 2026

test(dpp): add validation error path tests for identity_nonce, max_depth, and GroupV0 #3323

Merged

2 tasks

Copy link

codecov bot commented Mar 15, 2026

Codecov Report

Patch coverage is 92.00000% with 14 lines in your changes missing coverage. Please review.
Project coverage is 72.65%. Comparing base (6bc1c1b) to head (679d3b5).
Report is 1 commits behind head on v3.1-dev.

Files with missing lines	Patch %	Lines
packages/rs-dpp/src/data_contract/group/v0/mod.rs	91.86%	10 Missing
packages/rs-dpp/src/identity/identity_nonce.rs	90.90%	3 Missing
.../document_type/schema/validate_max_depth/v0/mod.rs	94.73%	1 Missing

Additional details and impacted files

@@ Coverage Diff @@ ## v3.1-dev #3123 +/- ## ============================================ + Coverage 72.62% 72.65% +0.02% ============================================ Files 3293 3293 Lines 269077 269252 +175 ============================================ + Hits 195425 195617 +192 + Misses 73652 73635 -17

Components	Coverage D
dpp	`60.30% <92.00%> (+0.13%)`
drive	`78.31% (o)`
drive-abci	`84.17% (o)`
sdk	`31.25% (o)`
dapi-client	`79.06% (o)`
platform-version	`()`
platform-value	`58.46% (o)`
platform-wallet	`60.40% (o)`
drive-proof-verifier	`()`

New features to boost your workflow:

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

Labels

None yet

Conversation

thepastaclaw commented Feb 20, 2026 * edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Being Fixed

What was done

C-07: validate_new_identity_nonce -- 0 tests - 5 tests

I-01: validate_max_depth_v0 -- max-depth error path never tested - 1 test

I-05: GroupV0::validate -- 1 test - 11 tests

Summary by CodeRabbit

Validation

Uh oh!

coderabbitai bot commented Feb 20, 2026 * edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thepastaclaw commented Feb 20, 2026

Uh oh!

coderabbitai bot commented Feb 20, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

QuantumExplorer commented Mar 15, 2026

Uh oh!

codecov bot commented Mar 15, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thepastaclaw commented Feb 20, 2026 *

edited

Loading

C-07: `validate_new_identity_nonce` -- 0 tests - 5 tests

I-01: `validate_max_depth_v0` -- max-depth error path never tested - 1 test

I-05: `GroupV0::validate` -- 1 test - 11 tests

coderabbitai bot commented Feb 20, 2026 *

edited

Loading