Uh oh!
There was an error while loading. Please reload this page.
test: cover drive/document, votes, queries, object_size_info, and lowcov - #3513
Merged
QuantumExplorer merged 2 commits intoApr 23, 2026
Merged
Conversation
Add 181 new unit tests across 30 files targeting the biggest remaining gaps. Per-target breakdown: - rs-drive/drive/document (83% → error-path focus, 11 tests): DataContractNotFound + unknown-document-type + malformed-bytes rejection paths in add_document_v0 / delete_document_for_contract_id / delete_document_for_contract_id _with_named_type_operations / update_document_for_contract_id / update_document_with_serialization_for_contract, plus apply=false estimation-costs branches. - rs-drive/drive/votes (81% → 37 tests): ResourceVoteChoice::to_key for all 3 variants; TreePath for ResourceVote error branches (contract-id mismatch, unknown doc type, unknown index name); TreePathStorageForm::try_from_tree_path exhaustive error matrix across two impls (length, empty/wrong bytes, non-UTF-8 doc type, VOTE_DECISIONS_TREE_KEY NotSupported); resolve_with_contract UnknownVersionMismatch dispatch error; ContestedDocumentResource VotePoll index/document_type accessor errors + owned/borrowed parity. - rs-drive/state_transition_action/identity (82% → 10 tests): identity_create_from_addresses + identity_topup_from_addresses transformers — empty-inputs ValueError, input-sum overflow, output > inputs overflow, success with and without output. - rs-drive-abci/query/token_queries + query/system + query/voting (81-83% → 21 tests): token_pre_programmed_distributions limit=0/over-max; system/path_elements missing-key + empty-keys; system/finalized_epoch_infos equal-indexes-both-included + u16::MAX range guard; voting/contested_resource_identity_votes prove + descending start_at + limit-rejection; contested_resource _vote_state prove + count zero/over-u16; contested_resource_ voters_for_identity prove + validation ordering; contested_ resources prove + short-contract-id identifier-byte-length error; vote_polls_by_end_date_query both limit-rejection branches + prove. - rs-dpp/data_contract/config (76% → 11 tests): V0 + V1 bincode roundtrips including sized_integer_types field; from_value empty-object defaults; get_contract_configuration_properties boolean reads + fallback + non-bool-value error. - rs-dpp/voting/contender_structs (68% → 4 tests): FinalizedResourceVoteChoicesWithVoterInfo Display + bincode roundtrip; FinalizedContenderWithSerializedDocument defaults; TryFrom<ContenderWithSerializedDocument> error paths (missing serialized_document, missing vote_tally) + happy path. - rs-dpp/voting/vote_info_storage (69% → 26 tests): finalize_vote_poll guards (NotStarted/Locked), all three ContestedDocumentVotePollWinnerInfo branches (NoWinner with/ without prior locks, WonByIdentity, Locked), last_locked_votes + last_abstain_votes aggregation, Display for status variants, bincode roundtrip, new/update_to_latest_version, all ContestedDocumentVotePollStoredInfoV0Getters proxies. - rs-drive/util/object_size_info (76% → 50+ tests): drive_key_info variants len/is_empty/to_owned_key_info/ to_key_info/add_path_* routing; key_value_info as_key_ref_request CorruptedCodeExecution branch; path_info push errors (fixed-size + KeySize-into-Vec) + convert_to_key_ info_path; path_key_info TryFrom empty-vec error + 5-variant len/is_empty/cache/convert/Display; path_key_element_info from_*_and_key_element across three branches + KeyUnknownElementSize rejection; contract_info accessors + DocumentTypeInfo::resolve missing-type errors. - rs-drive/drive/balances (73% → 10 tests): all path-helper functions shape-checked (array form matches vec form); add_to_system_credits + remove_from_system_credits roundtrip via setup_drive_with_initial_state_structure; zero-amount no-op. - rs-drive-abci/platform_types/platform (46% → 5 tests): Debug impls, From<&PlatformRef> for PlatformStateRef pointer- equality forwarding, open-with-latest-protocol success, committed_block_height_guard init. A latent production bug in data_contract/config/v0/mod.rs (requires_identity_encryption_bounded_key and requires_identity_decryption_bounded_key both read from same map key — v1 has the correct code) was flagged via a follow-up task and is out of scope for this PR. All tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds extensive unit tests across rs-dpp and rs-drive packages (many modules), covering data-contract config, voting structures, drive queries, document operations, vote path/storage forms, state-transition transformers, and object-size utilities. All changes are test-only; no production APIs or logic were modified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
⏳ Review in progress (commit 4da56df) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/query/voting/contested_resource_voters_for_identity/v0/mod.rs (1)
365-393: Test name doesn't match what's being asserted.
test_query_voters_count_zero_rejectedassertsDataContractNotFound, not a count-rejection error, because contract lookup runs beforecountvalidation (as your own comment notes). The test therefore doesn't actually cover thecount=0 → "out of bounds of [1, ...]"branch — it only pins the current validation order. Consider renaming it (e.g.,test_query_voters_count_zero_hidden_by_contract_lookup_order) to avoid misleading future readers, or — if covering the count guard is the goal — add a fixture with a real contract so thecountcheck actually executes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/voting/contested_resource_voters_for_identity/v0/mod.rs` around lines 365 - 393, The test test_query_voters_count_zero_rejected is misleading because it asserts a DataContractNotFound error (contract lookup runs before count validation); either rename the test to reflect that behavior (e.g., test_query_voters_count_zero_hidden_by_contract_lookup_order) or modify the fixture so count validation actually runs: use setup_platform to provide a real contract/valid contract_id (or set contract_id to a known existing id) so that calling query_contested_resource_voters_for_identity_v0 with GetContestedResourceVotersForIdentityRequestV0 and count: Some(0) triggers the out-of-bounds/count validation branch instead of DataContractNotFound; update the assertion accordingly.packages/rs-drive-abci/src/query/voting/contested_resource_identity_votes/v0/mod.rs (1)
316-365: Fragile assertions viaDebugstring matching; consider matching the error variant instead.Matching
format!("{:?}", err).contains("InvalidLimit") || contains("limit")will silently keep passing even if the error becomes something unrelated that happens to contain the substring"limit"(e.g., a message mentioning "client limit reached"). Since both paths go through the.ok_or(drive::error::Error::Query(QuerySyntaxError::InvalidLimit(...)))?construction, a structuralmatches!on theError/QuerySyntaxError::InvalidLimitvariant would be far more robust and future-proof. Non-blocking; fine as-is for coverage, but worth tightening.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/voting/contested_resource_identity_votes/v0/mod.rs` around lines 316 - 365, The tests test_query_contested_resource_identity_votes_limit_above_max_is_error and test_query_contested_resource_identity_votes_limit_zero_is_error use fragile Debug string matching on the returned error; change them to assert the actual error variant instead by pattern-matching the result of platform.query_contested_resource_identity_votes_v0(...) for the expected drive::error::Error::Query(QuerySyntaxError::InvalidLimit(..)) (or using matches! on that pattern) rather than checking format!("{:?}", err). This keeps the assertion robust to message changes while still verifying the InvalidLimit variant raised by GetContestedResourceIdentityVotesRequestV0 handling.
🤖 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/config/mod.rs`:
- Around line 664-675: The test reads_booleans_from_map uses identical booleans
which masks key mix-ups; update the test to use distinct values (e.g., call
make_contract_map(true, false) and assert cfg.can_be_deleted() is true and
cfg.readonly() is false), and ideally add the opposite case
(make_contract_map(false, true)) to catch swapped-key bugs; locate the test
reads_booleans_from_map and the helper make_contract_map and change the input
boolean pairs and corresponding assertions for
DataContractConfig::get_contract_configuration_properties to validate both
fields separately.
In
`@packages/rs-dpp/src/voting/vote_info_storage/contested_document_vote_poll_stored_info/mod.rs`:
- Around line 302-305: The current matches! invocation on
info.vote_poll_status_ref() with ContestedDocumentVotePollStatus::Started(_) is
evaluated but its result is discarded, so the expectation isn't enforced; change
it to assert!(matches!(info.vote_poll_status_ref(),
ContestedDocumentVotePollStatus::Started(_))) (or otherwise assert the match) so
the test actually fails when the status is not Started; locate the check around
vote_poll_status_ref() in contested_document_vote_poll_stored_info::mod.rs and
wrap the matches! call in an assert.
- Around line 229-236: The test currently calls matches!(...) on
ContestedDocumentVotePollStoredInfo::V0(v0) and discards the bool result, making
the check a no-op; change the statement to assert!(matches!(v0.vote_poll_status,
ContestedDocumentVotePollStatus::Started(_))) so the test actually fails when
the variant is different—update the code in the match arm handling
ContestedDocumentVotePollStoredInfo::V0 to use assert! around the matches!
expression referencing v0.vote_poll_status and
ContestedDocumentVotePollStatus::Started(_) .
In
`@packages/rs-dpp/src/voting/vote_info_storage/contested_document_vote_poll_stored_info/v0/mod.rs`:
- Around line 373-381: The test currently discards the boolean returned by
matches!(err, ProtocolError::CorruptedCodeExecution(_)) so the error variant
isn't asserted; update the assertion that calls info.finalize_vote_poll(...) to
assert that the error matches ProtocolError::CorruptedCodeExecution(_) (use
assert!(matches!(err, ProtocolError::CorruptedCodeExecution(_)))) and, like the
sibling test around lines 358-363, optionally also inspect the inner
message/payload of the CorruptedCodeExecution error to ensure the expected text
when state is Locked; reference the finalize_vote_poll call, the err binding,
and ProtocolError::CorruptedCodeExecution for locating and changing the test.
In
`@packages/rs-drive-abci/src/query/voting/contested_resource_vote_state/v0/mod.rs`:
- Around line 348-400: The two tests
(test_query_contested_resource_vote_state_count_zero_rejected and
test_query_contested_resource_vote_state_count_out_of_bounds_u16) never reach
the count validation because contract_id: vec![0; 8] fails earlier; either (A)
keep the current invalid-contract_id intent but rename the tests to indicate
they pin validation ordering and tighten the second test's assertion to expect
the same InvalidArgument about contract_id (match
QueryError::InvalidArgument(msg) if msg.contains("contract_id must be a valid
identifier")), or (B) change both tests to supply a valid 32-byte contract_id
and ensure a fixture contract exists in the test state so the request passes
contract_id.try_into() and then assert the actual count validation messages from
query_contested_resource_vote_state_v0 (for count = Some(0) assert errors
contains InvalidArgument with "limit out of bounds" or similar; for count >
u16::MAX assert errors contains "out of bounds of" / "limit out of bounds"). Use
the structs GetContestedResourceVoteStateRequestV0 and function
query_contested_resource_vote_state_v0 to locate where to modify the test inputs
and assertions.
In `@packages/rs-drive/src/drive/document/delete/mod.rs`:
- Around line 1422-1448: The test
test_delete_nonexistent_document_without_apply_estimation_costs currently
discards the result of drive.delete_document_for_contract; instead assert the
dry-run fails by checking the Result is an error (e.g.,
assert!(result.is_err())), so the estimation-only branch exercised by
delete_document_for_contract with apply=false is actually protected; keep the
existing call parameters and replace the unused binding with an assertion on
result.is_err() referencing delete_document_for_contract and the test name.
In `@packages/rs-drive/src/util/object_size_info/drive_key_info.rs`:
- Around line 219-231: The tests currently call matches! on the result of
DriveKeyInfo::KeyRef(...).add_fixed_size_path(path) and
DriveKeyInfo::KeySize(...).add_fixed_size_path(path) but ignore the boolean
result; change those standalone matches! calls to assert!(matches!(...)) so the
test will fail when the returned PathKeyInfo variant is incorrect (do the same
for the other similar blocks where add_fixed_size_path is used and matched
against PathFixedSizeKeyRef and PathKeySize).
In `@packages/rs-drive/src/util/object_size_info/path_key_element_info.rs`:
- Around line 128-138: The tests call matches!(...) but ignore its boolean
result; update each standalone matches! usage (e.g., inside fn
from_path_info_vec_with_key_element_produces_ref_element and the other test
blocks covering lines ~169-190, 192-219, 221-231, 246-292) to
assert!(matches!(...)) so the test fails when the variant is wrong; specifically
wrap the matches!(res, PathKeyRefElement(_)) and similar variant checks after
PathKeyElementInfo::from_path_info_and_key_element (and other calls returning
PathKeyElementInfo) with assert! to ensure the boolean is asserted.
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/query/voting/contested_resource_identity_votes/v0/mod.rs`:
- Around line 316-365: The tests
test_query_contested_resource_identity_votes_limit_above_max_is_error and
test_query_contested_resource_identity_votes_limit_zero_is_error use fragile
Debug string matching on the returned error; change them to assert the actual
error variant instead by pattern-matching the result of
platform.query_contested_resource_identity_votes_v0(...) for the expected
drive::error::Error::Query(QuerySyntaxError::InvalidLimit(..)) (or using
matches! on that pattern) rather than checking format!("{:?}", err). This keeps
the assertion robust to message changes while still verifying the InvalidLimit
variant raised by GetContestedResourceIdentityVotesRequestV0 handling.
In
`@packages/rs-drive-abci/src/query/voting/contested_resource_voters_for_identity/v0/mod.rs`:
- Around line 365-393: The test test_query_voters_count_zero_rejected is
misleading because it asserts a DataContractNotFound error (contract lookup runs
before count validation); either rename the test to reflect that behavior (e.g.,
test_query_voters_count_zero_hidden_by_contract_lookup_order) or modify the
fixture so count validation actually runs: use setup_platform to provide a real
contract/valid contract_id (or set contract_id to a known existing id) so that
calling query_contested_resource_voters_for_identity_v0 with
GetContestedResourceVotersForIdentityRequestV0 and count: Some(0) triggers the
out-of-bounds/count validation branch instead of DataContractNotFound; update
the assertion accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f87c70a9-87af-4915-9681-0be88fe66b21
📒 Files selected for processing (30)
packages/rs-dpp/src/data_contract/config/mod.rspackages/rs-dpp/src/voting/contender_structs/mod.rspackages/rs-dpp/src/voting/vote_info_storage/contested_document_vote_poll_stored_info/mod.rspackages/rs-dpp/src/voting/vote_info_storage/contested_document_vote_poll_stored_info/v0/mod.rspackages/rs-dpp/src/voting/vote_info_storage/contested_document_vote_poll_winner_info/mod.rspackages/rs-drive-abci/src/platform_types/platform/mod.rspackages/rs-drive-abci/src/query/system/finalized_epoch_infos/v0/mod.rspackages/rs-drive-abci/src/query/system/path_elements/v0/mod.rspackages/rs-drive-abci/src/query/token_queries/token_pre_programmed_distributions/v0/mod.rspackages/rs-drive-abci/src/query/voting/contested_resource_identity_votes/v0/mod.rspackages/rs-drive-abci/src/query/voting/contested_resource_vote_state/v0/mod.rspackages/rs-drive-abci/src/query/voting/contested_resource_voters_for_identity/v0/mod.rspackages/rs-drive-abci/src/query/voting/contested_resources/v0/mod.rspackages/rs-drive-abci/src/query/voting/vote_polls_by_end_date_query/v0/mod.rspackages/rs-drive/src/drive/balances/mod.rspackages/rs-drive/src/drive/document/delete/mod.rspackages/rs-drive/src/drive/document/insert/mod.rspackages/rs-drive/src/drive/document/update/mod.rspackages/rs-drive/src/drive/votes/mod.rspackages/rs-drive/src/drive/votes/resolved/vote_polls/contested_document_resource_vote_poll/mod.rspackages/rs-drive/src/drive/votes/storage_form/contested_document_resource_storage_form/mod.rspackages/rs-drive/src/drive/votes/storage_form/vote_storage_form/mod.rspackages/rs-drive/src/state_transition_action/identity/identity_create_from_addresses/v0/transformer.rspackages/rs-drive/src/state_transition_action/identity/identity_topup_from_addresses/v0/transformer.rspackages/rs-drive/src/util/object_size_info/contract_info.rspackages/rs-drive/src/util/object_size_info/drive_key_info.rspackages/rs-drive/src/util/object_size_info/key_value_info.rspackages/rs-drive/src/util/object_size_info/path_info.rspackages/rs-drive/src/util/object_size_info/path_key_element_info.rspackages/rs-drive/src/util/object_size_info/path_key_info.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+1422
to
+1448
| #[test] | ||
| fn test_delete_nonexistent_document_without_apply_estimation_costs() { | ||
| // Delete a nonexistent document with apply=false to exercise the | ||
| // estimation-costs branch (different control flow than the apply=true | ||
| // branch already covered). | ||
| let (drive, contract) = setup_dashpay("delete-nonexist-estimate", true); | ||
| let platform_version = PlatformVersion::latest(); | ||
| let document_id = Identifier::random(); | ||
| // With apply=false the code takes the estimated_costs_only_with_layer_info | ||
| // = Some(HashMap::new()) branch. The document does not exist, so we | ||
| // expect an error, but a different one than the stateful path. | ||
| let result = drive.delete_document_for_contract( | ||
| document_id, | ||
| &contract, | ||
| "profile", | ||
| BlockInfo::default(), | ||
| false, | ||
| None, | ||
| platform_version, | ||
| Some(&EPOCH_CHANGE_FEE_VERSION_TEST), | ||
| ); | ||
| // Either succeeds (estimation path can be more permissive) or errors; | ||
| // whichever happens, it exercises the previously-uncovered branch. | ||
| let _ = result; |
Contributor
There was a problem hiding this comment.
Assert the expected dry-run failure instead of discarding the result.
Line 1448 makes this test pass whether delete_document_for_contract succeeds or fails, despite the comment saying the nonexistent document should error. Add an assertion so the test protects the branch it covers.
Proposed test assertion
let result = drive.delete_document_for_contract(
document_id,
&contract,
"profile",
@@
platform_version,
Some(&EPOCH_CHANGE_FEE_VERSION_TEST),
);
- // Either succeeds (estimation path can be more permissive) or errors;- // whichever happens, it exercises the previously-uncovered branch.- let _ = result;+ assert!(+ result.is_err(),+ "expected dry-run delete of nonexistent document to fail, got {result:?}"+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn test_delete_nonexistent_document_without_apply_estimation_costs(){ | |
| // Delete a nonexistent document with apply=false to exercise the | |
| // estimation-costs branch (different control flow than the apply=true | |
| // branch already covered). | |
| let(drive, contract) = setup_dashpay("delete-nonexist-estimate",true); | |
| let platform_version = PlatformVersion::latest(); | |
| let document_id = Identifier::random(); | |
| // With apply=false the code takes the estimated_costs_only_with_layer_info | |
| // = Some(HashMap::new()) branch. The document does not exist, so we | |
| // expect an error, but a different one than the stateful path. | |
| let result = drive.delete_document_for_contract( | |
| document_id, | |
| &contract, | |
| "profile", | |
| BlockInfo::default(), | |
| false, | |
| None, | |
| platform_version, | |
| Some(&EPOCH_CHANGE_FEE_VERSION_TEST), | |
| ); | |
| // Either succeeds (estimation path can be more permissive) or errors; | |
| // whichever happens, it exercises the previously-uncovered branch. | |
| let _ = result; | |
| #[test] | |
| fn test_delete_nonexistent_document_without_apply_estimation_costs(){ | |
| // Delete a nonexistent document with apply=false to exercise the | |
| // estimation-costs branch (different control flow than the apply=true | |
| // branch already covered). | |
| let(drive, contract) = setup_dashpay("delete-nonexist-estimate",true); | |
| let platform_version = PlatformVersion::latest(); | |
| let document_id = Identifier::random(); | |
| // With apply=false the code takes the estimated_costs_only_with_layer_info | |
| // = Some(HashMap::new()) branch. The document does not exist, so we | |
| // expect an error, but a different one than the stateful path. | |
| let result = drive.delete_document_for_contract( | |
| document_id, | |
| &contract, | |
| "profile", | |
| BlockInfo::default(), | |
| false, | |
| None, | |
| platform_version, | |
| Some(&EPOCH_CHANGE_FEE_VERSION_TEST), | |
| ); | |
| assert!( | |
| result.is_err(), | |
| "expected dry-run delete of nonexistent document to fail, got {result:?}" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-drive/src/drive/document/delete/mod.rs` around lines 1422 - 1448,
The test test_delete_nonexistent_document_without_apply_estimation_costs
currently discards the result of drive.delete_document_for_contract; instead
assert the dry-run fails by checking the Result is an error (e.g.,
assert!(result.is_err())), so the estimation-only branch exercised by
delete_document_for_contract with apply=false is actually protected; keep the
existing call parameters and replace the unused binding with an assertion on
result.is_err() referencing delete_document_for_contract and the test name.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## v3.1-dev #3513 +/- ##
============================================
+ Coverage 86.77% 87.04% +0.26%
============================================
Files 2474 2474 Lines 281241 283604 +2363 ============================================
+ Hits 244048 246863 +2815 + Misses 37193 36741 -452
🚀 New features to boost your workflow:
|
Tighten 8 test assertions per CodeRabbit feedback:
- data_contract/config/mod.rs reads_booleans_from_map: iterate over
(true,false)/(false,true) pairs so a key mix-up (reading readonly
from can_be_deleted's key, or vice versa) would fail the test.
- voting/vote_info_storage/.../mod.rs (2 sites) and v0/mod.rs:
wrap bare `matches!(...)` in `assert!(matches!(...))`. The bare
form returns a discarded bool so the tests were no-ops — they
would pass with any variant.
- drive/document/delete test_delete_nonexistent_document_without_
apply_estimation_costs: the estimation branch actually succeeds
for nonexistent documents (uses fake cost estimates instead of
real grove ops). Assert that success + non-zero fee estimate
rather than ignoring the result.
- util/object_size_info/drive_key_info.rs + path_key_element_info
.rs: same `matches!` → `assert!(matches!(..))` fix across ~13
call sites that were silently passing.
- query/voting/contested_resource_vote_state v0: the two `count_*`
tests both used a short (8-byte) contract_id which fails
validation before the count branch. Renamed them to reflect
what they actually test (validation ORDERING — contract_id check
runs before count), and tightened the second test's assertion
from `!errors.is_empty()` to the same specific `InvalidArgument
("contract_id must be a valid identifier")` match.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 freeto 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 or feature implemented
Seventh PR in the push toward 90% global coverage. After #3503–#3512 we were at 86.77%. This PR targets ~4,000 missed lines across 4 focus areas and brings the project to 87.04%.
What was done?
181 new unit tests across 30 files.
Highlighted coverage wins
drive/document — error-path focus:
DataContractNotFound+ unknown-document-type + malformed-bytes rejection paths inadd_document_v0/delete_document_for_contract_id_*/update_document_for_contract_id_*/update_document_with_serialization_for_contract_v0, plusapply=falseestimation-costs branches.drive/votes —
ResourceVoteChoice::to_keyall 3 variants;TreePath for ResourceVoteerror branches (contract-id mismatch, unknown doc type, unknown index name);TreePathStorageForm::try_from_tree_pathexhaustive error matrix across two impls;resolve_with_contractUnknownVersionMismatchdispatch error;ContestedDocumentResourceVotePollindex/document_type accessor errors + owned/borrowed parity.state_transition_action/identity —
identity_create_from_addresses+identity_topup_from_addressestransformers: empty-inputsValueError, input-sum overflow,output > inputsoverflow, success with and without output.query modules —
prove: truepaths (previously untested in 5 voting modules),InvalidLimitpropagation for token_pre_programmed_distributions,u16::MAXrange guard in finalized_epoch_infos, limit-rejection in contested_resource_identity_votes and vote_polls_by_end_date_query. Fixed two mis-namedcount_*tests incontested_resource_vote_stateto reflect that they pin validation ordering (contract_id check runs before count), not the count branch itself.voting/vote_info_storage —
finalize_vote_pollguards (NotStarted/Locked), all threeContestedDocumentVotePollWinnerInfobranches,last_locked_votes+last_abstain_votesaggregation, bincode roundtrip, allContestedDocumentVotePollStoredInfoV0Gettersproxies.util/object_size_info (50+ tests across 6 files) —
drive_key_info/key_value_info/path_info/path_key_info/path_key_element_info/contract_infolen/is_empty/convert branches,pusherror paths (fixed-size, KeySize-into-Vec),KeyUnknownElementSizerejection,DocumentTypeInfo::resolvemissing-type errors.drive/balances — path-helper consistency (array form matches vec form);
add_to_system_credits+remove_from_system_creditsroundtrip under a real Drive; zero-amount no-op.platform_types/platform — Debug impls,
From<&PlatformRef> for PlatformStateRefpointer-equality forwarding, open-with-latest-protocol success,committed_block_height_guardinit.CodeRabbit review addressed
8 review comments handled in the follow-up commit, all test-quality tightenings:
data_contract/configreads_booleans_from_map: use distinct(true, false)/(false, true)pairs so a key mix-up would fail.voting/vote_info_storage(3 sites) +util/object_size_info(~13 sites): wrap barematches!(...)inassert!(matches!(...))— the bare form returns a discarded bool, making the tests no-ops.drive/document/deletetest_delete_nonexistent_document_without_apply_estimation_costs: the estimation branch actually succeeds (fake cost estimates, not real grove ops). Assert success + non-zero fee estimate instead of dropping the result.query/voting/contested_resource_vote_statecount_*tests: renamed to reflect they pin validation ordering, and tightened the second assertion to match the sameInvalidArgument("contract_id …")as the first.Follow-up observations
Two things surfaced during coverage work that deserve visibility but are out of scope for this PR:
V0 config parser quirk — consensus-frozen, NOT a bug
get_contract_configuration_properties_v0at packages/rs-dpp/src/data_contract/config/v0/mod.rs:177-185 reads the decryption bounded-key field fromREQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY(V1 at v1/mod.rs:140 reads from the correct key).Originally flagged in the commit message as a potential bug. That characterization was wrong — this is consensus-frozen V0 parser behavior. Changing it would risk a fork on any node replaying historical V0 contracts. #3514 pins the current behavior in place with lock-in tests and an in-code
CONSENSUS-FROZEN BUGcomment so it doesn't get "fixed" later.InvalidLimiterror surface in query modules (minor design observation)Multiple query modules propagate
InvalidLimitvia a top-levelErr(Error::Drive(Query(InvalidLimit)))instead of wrapping it in aQueryValidationResult::new_with_error(...). This is inconsistent with sibling validators in the same files, and means gRPC clients see a transport-level error forlimit=0/limit>maxrather than a structured validation response. Not incorrect — just inconsistent. Worth a small unifying PR but not required here.How Has This Been Tested?
cargo check --tests -p dpp -p drive -p drive-abci— cleancargo fmt -p dpp -p drive -p drive-abci— cleandrive::document,drive::votes,state_transition_action::identity,query::token_queries,query::system,query::voting,data_contract::config,voting::contender_structs,voting::vote_info_storage,util::object_size_info,drive::balances,platform_types::platform.86.77% → 87.04%(+0.27%), patch 98%+.Breaking Changes
None. Tests only.
Checklist
For repository code-owners and collaborators only