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
Closed
test(dpp): add validation error path tests for identity_nonce, max_depth, and GroupV0#3123thepastaclaw wants to merge 5 commits intodashpay:v3.1-devfrom
thepastaclaw wants to merge 5 commits intodashpay:v3.1-devfrom
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 (2)
📝 WalkthroughWalkthroughAdds 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
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📝 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 |
![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 (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—1000is an arbitrary sentinel with no expressed relationship toMISSING_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 carriessetting_identity_nonce(should equal the input nonce) andcurrent_identity_nonce(should beNonefor 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.
thepastaclaw
force-pushed
the
test/validation-error-paths-simple
branch
from
e77aa73 to
2ea57b2
Compare
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🤖 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.
4f89e9a to
db23345
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-dpp/src/identity/identity_nonce.rs (1)
215-226:validate_new_identity_nonce_invalid_largeis redundant withvalidate_new_identity_nonce_invalid_above_max.
validate_new_identity_noncehas exactly two branches (>= MAX/< MAX). Both are fully covered byvalid_zeroandinvalid_at_max; neitherinvalid_above_maxnorinvalid_largeadd new coverage. Consider droppinginvalid_largeor replacing it with a semantically distinct case (e.g., maxu64to 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.
db23345 to
4d6476c
Compare
thepastaclaw
force-pushed
the
test/validation-error-paths-simple
branch
from
4d6476c to
6327c5c
Compare
thepastaclaw
force-pushed
the
test/validation-error-paths-simple
branch
from
6327c5c to
b61b132
Compare
Member
|
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
added a commit
that referenced
this pull request
Mar 15, 2026
…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 <thepastaclaw@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 tasks
Codecov Report❌ Patch coverage is 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
🚀 New features to boost your workflow:
|
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 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 testsvalidate_new_identity_nonce_valid_zero— nonce=0, validvalidate_new_identity_nonce_valid_max_minus_one— nonce=23 (boundary), validvalidate_new_identity_nonce_invalid_at_max— nonce=24 (boundary), returnsNonceTooFarInPastvalidate_new_identity_nonce_invalid_above_max— nonce=25, returns errorvalidate_new_identity_nonce_invalid_large— nonce=1000, returns errorI-01:
validate_max_depth_v0— max-depth error path never tested → 1 testshould_return_error_when_max_depth_exceeded— dynamically builds a schema exceedingmax_depthfrom platform version, assertsDataContractMaxDepthExceedErrorI-05:
GroupV0::validate— 1 test → 11 teststest_group_exceeds_max_members— too many memberstest_group_too_few_members_zero— empty grouptest_group_too_few_members_one— single membertest_group_member_has_power_of_zero— member with power 0test_group_member_power_over_limit— power > GROUP_POWER_LIMIT (u16::MAX)test_group_member_power_exceeds_required— power > required_powertest_group_total_power_less_than_required— insufficient total powertest_group_non_unilateral_member_power_less_than_required— non-unilateral members can't meet thresholdtest_group_required_power_zero— documents thatrequired_power=0hitspower > required_powerbefore the invalid-power checktest_group_required_power_over_limit— required_power > GROUP_POWER_LIMIT (65536), reached via 3 members at max powerAll tests compile and pass (
cargo test -p dpp --lib— 32 targeted tests, 0 failures).Summary by CodeRabbit
Validation
What was tested
cargo test -p dpp --libResults
Environment
dashpay/platform(Rust test run)