test: cover low-coverage modules in dpp and drive#3506
Merged
QuantumExplorer merged 2 commits intoApr 21, 2026
Conversation
Add 161 new unit tests across 8 modules identified as highest leverage — sorted by lowest coverage %, not just highest missed line count, since PR #3505 showed that accessor tests on high-cov files add near-zero Codecov gain. This round targets error paths, overflow/underflow branches, corrupted-state arms, and edge cases that integration tests do not exercise. Coverage targets (prev cov → covered here): - rs-dpp/document/document_factory (54.64% → covered): 22 tests on construction errors (u32::MAX / 0 version), create_document errors (InvalidDocumentTypeError + entropy-generator failure), create_extended_document, create_extended_from_document_buffer roundtrip + malformed bytes, and all create_state_transition error branches (empty outer, outer-with-empty-inner, mismatched owners, InvalidInitialRevisionError, RevisionAbsentError on Create/Replace/Delete, and mixed Create+Replace merging). - rs-drive/drive/shielded (64% → covered): 17 tests covering paths.rs pool_type branches + unknown-pool error, update/read total balance with i64::MAX boundary + CorruptedDriveState, has_anchor / has_nullifier / notes_count / insert_note / insert_nullifiers happy + empty paths, prune_anchors cutoff boundary, record_anchor_if_changed zero-anchor short-circuit + corrupted-element error, nullifiers/types serialization errors (bincode garbage, wrong-length parse_key). - rs-drive/drive/prefunded_specialized_balances (64% → covered): 28 tests including v1 StatefulDirectQuery vs StatelessDirectQuery branches, checked_add overflow (CriticalCorruptedState), MAX_CREDITS guard (CriticalBalanceOverflow), missing-id errors with carried (avail, req), CorruptedElementType on negative SumItem, i64::MAX sentinel in v1, estimation override, and empty_prefunded's three branches (missing-without-error-flag, missing-with-error-flag, existing-returns-prev-amount). - rs-dpp/state_transition/mod.rs (66% → covered): 34 NEW tests on top of the 25 from PR #3504, covering all previously-untested variants (DataContractCreate/Update, IdentityCreate/TopUp/Update, Unshield, ShieldedTransfer/Withdrawal) across name(), state_transition_type(), is_identity_signed(), signature / owner_id / user_fee_increase accessors + setters (incl. shielded no-op arms), active_version_range() contract V1 + shielded ranges, inputs() None wildcard, optional_asset_lock_proof() _=> None, required_asset_lock_balance_for_processing_start's CorruptedCodeExecution for 8 non-asset-lock variants, security_level_requirement / purpose_requirement None-arms, and deserialize_from_bytes_in_version's StateTransitionIsNotActive error (ShieldedTransfer against protocol < 12). - rs-drive/verify/state_transition (76% → covered): 13 tests on verify_state_transition_was_executed_with_proof_v0 for DataContractUpdate empty-proof, MasternodeVote UnknownContract, ShieldedWithdrawal/Transfer/Unshield empty-proof, Batch::V1 Token(Burn) with missing contract provider, plus TryTransitionIntoPathQuery keeps_history branches (Burn/Freeze/Unfreeze/SetPriceForDirectPurchase) and the expected_token_configuration invalid-position error. - rs-dpp/shielded/builder (72% → covered): 24 tests on build_output_only_bundle, serialize_authorized_bundle field preservation, From<&OrchardAddress> delegation, build_spend_bundle AnchorMismatch, deterministic empty-spends, shielded transfer overflow (checked_add u64::MAX), fee boundary matrix (below min, above 1000x, exact min, exact 1000x, None default), and withdrawal_amount > i64::MAX branch in shielded_withdrawal / unshield. - rs-dpp/document/v0 (77% → covered): 10 tests covering Display's optional-field conditionals (created_at, updated_at, transferred_at, block/core-block heights, creator_id, empty properties), DateTime::from_timestamp_millis None fallback for u64::MAX, DocumentV0Setters::bump_revision (None no-op + Revision::MAX saturation guard), Default impl, PartialEq discrimination on creator_id. - rs-dpp/document/extended_document (72% → covered): 13 tests on from_untrusted/trusted_platform_value missing-$type + unknown- type errors, set_untrusted non-identifier/non-binary branch, hash distinctness, serialize_specific_version_to_bytes v0 success + UnknownVersionMismatch, from_bytes empty/unknown- version/truncated paths, to_json and to_json_with_identifiers_ using_bytes inclusion of $type + $dataContract, and to_map_value $tokenPaymentInfo absence. All 161 tests pass. A latent production bug in deduct_from_prefunded_specialized_balance/mod.rs (dispatcher only knows v0 but platform v3+ sets 1) was flagged via a follow-up task — it's currently dead code and out of scope for this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 0 seconds. ⌛ 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 (8)
📝 WalkthroughWalkthroughThis PR adds comprehensive test coverage across Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
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 |
Collaborator
Review GateCommit:
|
![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: 8
🧹 Nitpick comments (6)
packages/rs-drive/src/drive/shielded/nullifiers/types.rs (1)
168-260: Nit: move#[cfg(test)] mod teststo the bottom of the file.The test module is inserted between
CompactedNullifierChange(ends line 166) andNullifierChangePerBlock(line 262), splitting the production types. Conventionallymod testssits at the end of the file so all public/production items stay contiguous.♻️ Suggested reordering
Move the
NullifierChangePerBlockdeclaration (lines 262–268) above the#[cfg(test)] mod tests { … }block so the test module is the final item in the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/shielded/nullifiers/types.rs` around lines 168 - 260, The test module is splitting production types—move the entire #[cfg(test)] mod tests block to the end of the file so production items remain contiguous; specifically, relocate the declaration of NullifierChangePerBlock so it appears before the tests (ensure CompactedNullifierChange and NullifierChangePerBlock are adjacent), then place #[cfg(test)] mod tests { ... } as the final item in the file.packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs (1)
146-179: Rename test to reflect exclusive-cutoff semantics.The test name
prune_cutoff_excludes_anchors_at_cutoff_heightreads as "excludes [from preservation]" i.e. prunes, but the body asserts the opposite (anchor atcutoffis preserved becauseRangeTois exclusive). Considercutoff_is_exclusive_anchor_at_cutoff_preservedor similar to avoid future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs` around lines 146 - 179, Rename the test function prune_cutoff_excludes_anchors_at_cutoff_height to a name that reflects the exclusive-cutoff behavior (e.g., cutoff_is_exclusive_anchor_at_cutoff_preserved) so the name matches the assertion that an anchor at the cutoff (prune_shielded_pool_anchors_v0 with cutoff 20) is preserved; update the test declaration and any references to the function name, keeping the test body using seed_anchor, has_shielded_anchor, prune_shielded_pool_anchors_v0, and PlatformVersion::latest unchanged.packages/rs-drive/src/drive/shielded/has_anchor/v0/mod.rs (1)
54-94: Consider dropping the unrelated transaction on test teardown.Minor nit only: the
Transactionstarted at line 60 is never committed or rolled back. That is harmless here (it drops at end of scope), but if future changes add a post-test assertion outside the transaction it could silently observe stale state. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/shielded/has_anchor/v0/mod.rs` around lines 54 - 94, The test has_anchor_returns_true_after_direct_insert creates a Transaction via drive.grove.start_transaction() but never closes it; explicitly close the transaction before test exit to avoid leaking an open transaction. After the assertions, call the appropriate transaction finalizer on the transaction variable (e.g., rollback or commit using the grove/transaction API your codebase provides) so the started transaction is deterministically ended.packages/rs-drive/src/drive/shielded/insert_nullifiers/v0/mod.rs (1)
58-127: LGTM — tests cover empty, single (with apply + round-trip viahas_nullifier), and multi-insert op-count paths.Minor note:
insert_many_nullifiers_produces_matching_op_countonly asserts the returned ops length and never applies the batch or checks membership. Since the apply+read path is already covered by the single-insert test and byhas_nullifier/v0's tests, this is fine as-is; consider applying the batch and asserting all five are discoverable if you want stronger coverage here later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/shielded/insert_nullifiers/v0/mod.rs` around lines 58 - 127, The test insert_many_nullifiers_produces_matching_op_count only asserts ops.len() and doesn't apply or verify persistence; to strengthen the test, after calling insert_nullifiers_v0 in insert_many_nullifiers_produces_matching_op_count capture the returned ops, convert them via LowLevelDriveOperation::grovedb_operations_batch_consume, apply the batch with drive.grove_apply_batch_with_add_costs (matching the single-nullifier test) and then assert each nullifier is present using drive.has_nullifier to ensure the inserts were persisted and discoverable; use the same transaction and platform_version variables already in the test.packages/rs-dpp/src/state_transition/mod.rs (1)
2120-2166: Nit: redundant rebinding ofcontract_v1_range.
contract_v1_rangeis declared identically at Line 2124 and again at Line 2129 before its second use. The second binding is a no-op; drop it (or reuse the first binding for both assertions).Proposed tweak
let contract_v1_range = 9..=LATEST_VERSION; assert_eq!( sample_data_contract_create_st().active_version_range(), - contract_v1_range + contract_v1_range.clone() ); - let contract_v1_range = 9..=LATEST_VERSION; assert_eq!( sample_data_contract_update_st().active_version_range(), contract_v1_range );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/mod.rs` around lines 2120 - 2166, In test_active_version_range_contract_and_shielded_branches, remove the redundant re-declaration of contract_v1_range and reuse the initial binding (contract_v1_range = 9..=LATEST_VERSION) for both asserts that call sample_data_contract_create_st().active_version_range() and sample_data_contract_update_st().active_version_range(); keep the single contract_v1_range binding and leave the rest of the assertions unchanged.packages/rs-dpp/src/document/document_factory/mod.rs (1)
155-675: Test suite looks solid — good error-path and dispatch coverage.The new
DocumentFactorytests cover construction error paths, entropy-generator failure propagation,create_document/create_extended_documenthappy and error paths, buffer roundtrip, and the full matrix ofcreate_state_transitionerror arms (NoDocumentsSupplied,MismatchOwnerIds,InvalidInitialRevision,RevisionAbsentfor Create/Replace/Delete) plus nonce-increment and mixed-action success. Shared helpers (setup_factory,build_doc, deterministic/failing entropy generators) keep the cases compact and consistent.One trivial observation:
new_variant_is_v0at Lines 234-241 only performs an exhaustive-variant match with no explicit assertion, so it serves only as a compile-time guard that the enum still has a single V0 arm — consider making that intent explicit (e.g.,assert!(matches!(factory, DocumentFactory::V0(_)))) or dropping the test sincenew_with_entropy_generator_valid_version_succeedsalready asserts the variant. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/document/document_factory/mod.rs` around lines 155 - 675, The test new_variant_is_v0 only pattern-matches the DocumentFactory enum without asserting — make the intent explicit by replacing the match-only check with an assertion like assert!(matches!(factory, DocumentFactory::V0(_))) (or remove the test); locate the test function new_variant_is_v0 and update its body to assert the factory variant is V0 (referencing DocumentFactory::V0) so the test fails if the variant changes.
🤖 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/document/v0/mod.rs`:
- Around line 312-322: The test display_invalid_timestamp_uses_default_formatter
incorrectly uses doc.created_at = Some(u64::MAX) which becomes -1 when cast to
i64 and thus DateTime::from_timestamp_millis returns Some; update the test to
supply a created_at value outside chrono's supported millisecond range (e.g. set
doc.created_at to (i64::MAX as u64) + 1 or another u64 > i64::MAX) so
DateTime::from_timestamp_millis returns None and the Display impl's
unwrap_or_default() branch is exercised; refer to the test function
display_invalid_timestamp_uses_default_formatter, the doc.created_at field, and
the DateTime::from_timestamp_millis usage in the Display impl when making the
change.
In `@packages/rs-dpp/src/shielded/builder/shielded_transfer.rs`:
- Around line 265-305: The test currently accepts either an "overflow" message
or "exceeds total", which masks whether the checked-add overflow branch
(transfer_amount.checked_add(effective_fee)) was hit; update the assertion in
test_shielded_transfer_fee_plus_amount_overflow_errors to require the
overflow-specific error only (i.e., assert that the returned error string/from
build_shielded_transfer_transition contains the overflow indicator) so the test
explicitly verifies the checked_add overflow path rather than the "exceeds
total" branch.
In `@packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs`:
- Around line 316-338: The test currently calls unwrap_err() on the result of
build_shielded_withdrawal_transition which fails if the call actually returned
Ok; change the assertion to allow Ok results for exact-boundary fee cases by
checking result.is_ok() first and only converting to a string and asserting the
error message does not contain "exceeds 1000x" when result.is_err(); update the
same pattern for the second occurrence around the
build_shielded_withdrawal_transition call at the later block (lines ~357-377)
and reference the fee_at_boundary variable and
build_shielded_withdrawal_transition to locate the code.
In `@packages/rs-dpp/src/shielded/builder/unshield.rs`:
- Around line 299-318: The test currently calls build_unshield_transition(...)
and immediately does result.unwrap_err().to_string(), which will panic if the
function returns Ok; change it to handle both outcomes: match on result from
build_unshield_transition (the call with spends, output_address, 100,
&change_address, &fvk, &ask, Anchor::empty_tree(), &TestProver, [0u8; 36],
Some(boundary), platform_version) — if Ok(_) then treat the test as passed for
the accepted-path; if Err(e) then convert e.to_string() and assert that the
error does not contain the upper-bound message ("exceeds 1000x") as before.
Apply the same pattern to the other identical block (the second occurrence
around the 407-427 region).
In
`@packages/rs-drive/src/drive/prefunded_specialized_balances/add_prefunded_specialized_balance_operations/v0/mod.rs`:
- Around line 105-134: The test calls
add_prefunded_specialized_balance_operations_v0 and then uses seed_balance to
create the entry instead of applying the ops produced, so swap the seed_balance
step: apply the returned ops (the ops variable from
add_prefunded_specialized_balance_operations_v0) to the Drive using the
appropriate apply method for drive operations (replace the seed_balance call)
and then call fetch_prefunded_specialized_balance to assert the persisted value;
do the same fix for the other similar test around lines 245-267 so the test
verifies the actual ops emitted by
add_prefunded_specialized_balance_operations_v0.
In
`@packages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance_operations/v0/mod.rs`:
- Around line 96-116: The test
empty_missing_without_error_flag_returns_zero_and_no_ops currently ignores the
returned operations (_ops) which allows read/cost ops to mask the intended "no
delete op" behavior; update the test to capture the returned ops (rename _ops to
ops) from empty_prefunded_specialized_balance_operations_v0 and explicitly
assert that no delete operation targeting the Identifier id is present in ops
(e.g., search ops for a delete of id and assert not found). Apply the analogous
change to the other test mentioned (the one around lines 164-184) to explicitly
assert presence/absence of the delete op for id instead of only checking ops
non-emptiness.
In
`@packages/rs-drive/src/verify/state_transition/state_transition_execution_path_queries/token_transition.rs`:
- Around line 443-464: Add an assertion that the TokenHistory contract id
appears as an exact path segment (not just across segment boundaries) for every
history-enabled query (e.g., the Burn/Freeze/Unfreeze/SetPrice transition
checks) after successful SingleDocumentDriveQuery::construct_path_query;
implement a small helper that takes &path_query.path and
token_history_contract.id().into_buffer() and verifies that one of the path
segments equals the contract id bytes (rather than using flattened windows on
full_path), then invoke that helper wherever you currently verify successful
query construction for history-enabled token operations.
In
`@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- Around line 2624-2668: The test for ShieldedWithdrawal named "no nullifiers"
should assert the specific InvalidTransition proof error instead of accepting
any Proof/GroveDB failure: update the assert! to match
Err(crate::error::Error::Proof(ProofError::InvalidTransition)) (referencing the
ProofError variant from the proof types used by
verify_state_transition_was_executed_with_proof and the first_nullifier guard in
ShieldedWithdrawalTransitionV0); locate the assertion near the call to
Drive::verify_state_transition_was_executed_with_proof and replace the broad
matches! with a pattern that specifically expects the InvalidTransition Proof
error (or, if you prefer the test to cover malformed proofs, rename the test to
reflect that intent).
---
Nitpick comments:
In `@packages/rs-dpp/src/document/document_factory/mod.rs`:
- Around line 155-675: The test new_variant_is_v0 only pattern-matches the
DocumentFactory enum without asserting — make the intent explicit by replacing
the match-only check with an assertion like assert!(matches!(factory,
DocumentFactory::V0(_))) (or remove the test); locate the test function
new_variant_is_v0 and update its body to assert the factory variant is V0
(referencing DocumentFactory::V0) so the test fails if the variant changes.
In `@packages/rs-dpp/src/state_transition/mod.rs`:
- Around line 2120-2166: In
test_active_version_range_contract_and_shielded_branches, remove the redundant
re-declaration of contract_v1_range and reuse the initial binding
(contract_v1_range = 9..=LATEST_VERSION) for both asserts that call
sample_data_contract_create_st().active_version_range() and
sample_data_contract_update_st().active_version_range(); keep the single
contract_v1_range binding and leave the rest of the assertions unchanged.
In `@packages/rs-drive/src/drive/shielded/has_anchor/v0/mod.rs`:
- Around line 54-94: The test has_anchor_returns_true_after_direct_insert
creates a Transaction via drive.grove.start_transaction() but never closes it;
explicitly close the transaction before test exit to avoid leaking an open
transaction. After the assertions, call the appropriate transaction finalizer on
the transaction variable (e.g., rollback or commit using the grove/transaction
API your codebase provides) so the started transaction is deterministically
ended.
In `@packages/rs-drive/src/drive/shielded/insert_nullifiers/v0/mod.rs`:
- Around line 58-127: The test insert_many_nullifiers_produces_matching_op_count
only asserts ops.len() and doesn't apply or verify persistence; to strengthen
the test, after calling insert_nullifiers_v0 in
insert_many_nullifiers_produces_matching_op_count capture the returned ops,
convert them via LowLevelDriveOperation::grovedb_operations_batch_consume, apply
the batch with drive.grove_apply_batch_with_add_costs (matching the
single-nullifier test) and then assert each nullifier is present using
drive.has_nullifier to ensure the inserts were persisted and discoverable; use
the same transaction and platform_version variables already in the test.
In `@packages/rs-drive/src/drive/shielded/nullifiers/types.rs`:
- Around line 168-260: The test module is splitting production types—move the
entire #[cfg(test)] mod tests block to the end of the file so production items
remain contiguous; specifically, relocate the declaration of
NullifierChangePerBlock so it appears before the tests (ensure
CompactedNullifierChange and NullifierChangePerBlock are adjacent), then place
#[cfg(test)] mod tests { ... } as the final item in the file.
In `@packages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rs`:
- Around line 146-179: Rename the test function
prune_cutoff_excludes_anchors_at_cutoff_height to a name that reflects the
exclusive-cutoff behavior (e.g., cutoff_is_exclusive_anchor_at_cutoff_preserved)
so the name matches the assertion that an anchor at the cutoff
(prune_shielded_pool_anchors_v0 with cutoff 20) is preserved; update the test
declaration and any references to the function name, keeping the test body using
seed_anchor, has_shielded_anchor, prune_shielded_pool_anchors_v0, and
PlatformVersion::latest unchanged.
🪄 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: 5ed24db9-feb8-4c8b-9cbf-5238781da8e0
📒 Files selected for processing (30)
packages/rs-dpp/src/document/document_factory/mod.rspackages/rs-dpp/src/document/extended_document/v0/mod.rspackages/rs-dpp/src/document/v0/mod.rspackages/rs-dpp/src/shielded/builder/mod.rspackages/rs-dpp/src/shielded/builder/shield.rspackages/rs-dpp/src/shielded/builder/shield_from_asset_lock.rspackages/rs-dpp/src/shielded/builder/shielded_transfer.rspackages/rs-dpp/src/shielded/builder/shielded_withdrawal.rspackages/rs-dpp/src/shielded/builder/unshield.rspackages/rs-dpp/src/state_transition/mod.rspackages/rs-drive/src/drive/prefunded_specialized_balances/add_prefunded_specialized_balance_operations/v0/mod.rspackages/rs-drive/src/drive/prefunded_specialized_balances/add_prefunded_specialized_balance_operations/v1/mod.rspackages/rs-drive/src/drive/prefunded_specialized_balances/deduct_from_prefunded_specialized_balance_operations/v0/mod.rspackages/rs-drive/src/drive/prefunded_specialized_balances/deduct_from_prefunded_specialized_balance_operations/v1/mod.rspackages/rs-drive/src/drive/prefunded_specialized_balances/empty_prefunded_specialized_balance_operations/v0/mod.rspackages/rs-drive/src/drive/prefunded_specialized_balances/fetch/single_balance/v0/mod.rspackages/rs-drive/src/drive/prefunded_specialized_balances/prove/single_balance/v0/mod.rspackages/rs-drive/src/drive/shielded/has_anchor/v0/mod.rspackages/rs-drive/src/drive/shielded/has_nullifier/v0/mod.rspackages/rs-drive/src/drive/shielded/insert_note/v0/mod.rspackages/rs-drive/src/drive/shielded/insert_nullifiers/v0/mod.rspackages/rs-drive/src/drive/shielded/notes_count/v0/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/types.rspackages/rs-drive/src/drive/shielded/paths.rspackages/rs-drive/src/drive/shielded/prune_anchors/v0/mod.rspackages/rs-drive/src/drive/shielded/read_total_balance/v0/mod.rspackages/rs-drive/src/drive/shielded/record_anchor_if_changed/v0/mod.rspackages/rs-drive/src/drive/shielded/update_total_balance/v0/mod.rspackages/rs-drive/src/verify/state_transition/state_transition_execution_path_queries/token_transition.rspackages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
Comment on lines
+299
to
+318
| let result = build_unshield_transition( | ||
| spends, | ||
| output_address, | ||
| 100, | ||
| &change_address, | ||
| &fvk, | ||
| &ask, | ||
| Anchor::empty_tree(), | ||
| &TestProver, | ||
| [0u8; 36], | ||
| Some(boundary), | ||
| platform_version, | ||
| ); | ||
| let err = result.unwrap_err().to_string(); | ||
| // Must not trip the upper-bound branch. | ||
| assert!( | ||
| !err.contains("exceeds 1000x"), | ||
| "boundary fee should be accepted: {}", | ||
| err | ||
| ); |
Contributor
There was a problem hiding this comment.
Don’t fail accepted-path tests on successful builds.
These cases are checking that validation is bypassed/reaches downstream logic. A future Ok result should satisfy that intent instead of panicking via unwrap_err().
Proposed test adjustment
- let err = result.unwrap_err().to_string();
- // Must not trip the upper-bound branch.
- assert!(
- !err.contains("exceeds 1000x"),
- "boundary fee should be accepted: {}",
- err
- );
+ if let Err(err) = result {
+ let err = err.to_string();
+ // Must not trip the upper-bound branch.
+ assert!(
+ !err.contains("exceeds 1000x"),
+ "boundary fee should be accepted: {}",
+ err
+ );
+ }
@@
- let err_msg = result.unwrap_err().to_string();
- assert!(
- err_msg.contains("failed to add spend")
- || err_msg.contains("anchor")
- || err_msg.contains("AnchorMismatch"),
- "expected downstream add_spend error, got: {}",
- err_msg
- );
+ if let Err(err) = result {
+ let err_msg = err.to_string();
+ assert!(
+ err_msg.contains("failed to add spend")
+ || err_msg.contains("anchor")
+ || err_msg.contains("AnchorMismatch"),
+ "expected downstream add_spend error, got: {}",
+ err_msg
+ );
+ }Also applies to: 407-427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-dpp/src/shielded/builder/unshield.rs` around lines 299 - 318, The
test currently calls build_unshield_transition(...) and immediately does
result.unwrap_err().to_string(), which will panic if the function returns Ok;
change it to handle both outcomes: match on result from
build_unshield_transition (the call with spends, output_address, 100,
&change_address, &fvk, &ask, Anchor::empty_tree(), &TestProver, [0u8; 36],
Some(boundary), platform_version) — if Ok(_) then treat the test as passed for
the accepted-path; if Err(e) then convert e.to_string() and assert that the
error does not contain the upper-bound message ("exceeds 1000x") as before.
Apply the same pattern to the other identical block (the second occurrence
around the 407-427 region).
Comment on lines
+2624
to
+2668
| // --- ShieldedWithdrawal: no nullifiers → InvalidTransition error. | ||
| #[test] | ||
| fn verify_shielded_withdrawal_no_nullifiers_returns_invalid_transition() { | ||
| let platform_version = PlatformVersion::latest(); | ||
| use dpp::identity::core_script::CoreScript; | ||
| use dpp::state_transition::shielded_withdrawal_transition::v0::ShieldedWithdrawalTransitionV0; | ||
| use dpp::state_transition::shielded_withdrawal_transition::ShieldedWithdrawalTransition; | ||
| use dpp::withdrawal::Pooling; | ||
|
|
||
| let st = StateTransition::ShieldedWithdrawal(ShieldedWithdrawalTransition::V0( | ||
| ShieldedWithdrawalTransitionV0 { | ||
| actions: vec![], | ||
| unshielding_amount: 0, | ||
| anchor: [0u8; 32], | ||
| proof: vec![], | ||
| binding_signature: [0u8; 64], | ||
| core_fee_per_byte: 1, | ||
| pooling: Pooling::Never, | ||
| output_script: CoreScript::from_bytes(vec![]), | ||
| }, | ||
| )); | ||
|
|
||
| let known_contracts_provider_fn: &ContractLookupFn = &|_id| Ok(None); | ||
|
|
||
| // The proof content doesn't matter: we expect the grove-db call to | ||
| // either fail or return with no spent nullifiers, but the path runs | ||
| // through several verify_shielded_nullifiers calls which typically | ||
| // error out first. The test only asserts the result is an Error. | ||
| let result = Drive::verify_state_transition_was_executed_with_proof( | ||
| &st, | ||
| &BlockInfo::default(), | ||
| &[], | ||
| known_contracts_provider_fn, | ||
| platform_version, | ||
| ); | ||
|
|
||
| assert!( | ||
| matches!( | ||
| result, | ||
| Err(crate::error::Error::Proof(_)) | Err(crate::error::Error::GroveDB(_)) | ||
| ), | ||
| "expected error for shielded withdrawal with no nullifiers, got: {:?}", | ||
| result | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Match the test name by asserting InvalidTransition.
The test is named and commented as the “no nullifiers” invalid-transition case, but Lines 2660-2667 accept any proof/GroveDB failure. If the intent is to cover the first_nullifier guard, assert ProofError::InvalidTransition; otherwise rename it to an empty-proof/malformed-proof test.
Suggested assertion if the invalid-transition branch is intended
- assert!(
- matches!(
- result,
- Err(crate::error::Error::Proof(_)) | Err(crate::error::Error::GroveDB(_))
- ),
- "expected error for shielded withdrawal with no nullifiers, got: {:?}",
- result
- );
+ assert!(
+ matches!(
+ result,
+ Err(crate::error::Error::Proof(ProofError::InvalidTransition(msg)))
+ if msg.contains("no nullifiers")
+ ),
+ "expected InvalidTransition for shielded withdrawal with no nullifiers, got: {:?}",
+ result
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`
around lines 2624 - 2668, The test for ShieldedWithdrawal named "no nullifiers"
should assert the specific InvalidTransition proof error instead of accepting
any Proof/GroveDB failure: update the assert! to match
Err(crate::error::Error::Proof(ProofError::InvalidTransition)) (referencing the
ProofError variant from the proof types used by
verify_state_transition_was_executed_with_proof and the first_nullifier guard in
ShieldedWithdrawalTransitionV0); locate the assertion near the call to
Drive::verify_state_transition_was_executed_with_proof and replace the broad
matches! with a pattern that specifically expects the InvalidTransition Proof
error (or, if you prefer the test to cover malformed proofs, rename the test to
reflect that intent).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3506 +/- ##
============================================
+ Coverage 85.30% 85.76% +0.45%
============================================
Files 2476 2476
Lines 270705 276009 +5304
============================================
+ Hits 230938 236728 +5790
+ Misses 39767 39281 -486
🚀 New features to boost your workflow:
|
Tighten 8 test assertions per CodeRabbit feedback: - document/v0: use i64::MAX (outside chrono's range) instead of u64::MAX (which casts to -1i64, inside range) so the DateTime::from_timestamp_millis None branch + unwrap_or_default is actually exercised. - shielded/builder/shielded_transfer: assert the specific "fee + transfer_amount overflows u64" message instead of the broader "overflow or exceeds total" or-pattern, so the test actually verifies checked_add's overflow branch. - shielded/builder/shielded_withdrawal + unshield: boundary-fee tests now accept Ok as valid (exact-boundary values SHOULD pass validation) — only assert that any error message doesn't mention "exceeds 1000x" / "below minimum required fee". - prefunded/add: apply the ops the function actually returned via apply_batch_low_level_drive_operations and fetch, rather than re-seeding through the dispatcher — so the insert branch's GroveDB write is what we verify. - prefunded/empty: assert GroveDB delete op presence/absence directly via a local has_delete_op helper rather than checking ops.is_empty() (read/cost ops can satisfy non-empty alone). - verify/state_transition/token_transition: assert the TokenHistory contract id appears as an exact path segment for every history-enabled transition (Burn/Freeze/Unfreeze/SetPrice) via new assert_path_targets_token_history helper. Replaces the old flatten+windows check which could match across segment boundaries. - verify/state_transition/verify_..._with_proof: rename verify_shielded_withdrawal_no_nullifiers_returns_invalid_transition to verify_shielded_withdrawal_empty_proof_error since the empty proof fails before the first_nullifier guard is reachable. All 185 touched tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
deleted the
test/coverage-lowcov-shielded-factories-verify
branch
6 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Fourth PR in the push toward 90% global coverage (now 85.44% after #3503, #3504, #3505). PR #3505 only added +0.14% despite 168 tests, because many accessor tests overlap with integration test coverage.
Lesson applied: this round sorts targets by lowest coverage % (more untested error branches per line) rather than by missed-line count, and focuses the tests on error paths, overflow/underflow, corrupted-state arms, and edge cases.
What was done?
161 new unit tests across 8 modules and 30 files.
rs-dpp/document/document_factoryrs-drive/drive/shieldedrs-drive/drive/prefunded_specialized_balancesrs-dpp/state_transition/mod.rsrs-dpp/shielded/builderrs-dpp/document/extended_documentrs-dpp/document/v0rs-drive/verify/state_transitionHighlighted new coverage
DocumentError::*variant increate_state_transition(NoDocumentsSuppliedError,MismatchOwnerIdsError,RevisionAbsentErroron Create/Replace/Delete,InvalidInitialRevisionError), factory construction with version 0/u32::MAX, failing entropy generators, create_extended_from_document_buffer roundtrip + malformed bytes.paths.rspool_type 0/1/2 + unknown →InvalidInput;update_total_balance_v0i64::MAX boundary + u64::MAXCorruptedDriveState;prune_anchors_v0cutoff-exclusive boundary;record_anchor_if_changed_v0zero-anchor short-circuit + corrupted-element error;nullifiers/types.rsbincode-garbage and wrong-lengthparse_keyerrors.checked_addoverflow →CriticalCorruptedState,MAX_CREDITSguard →CriticalBalanceOverflow, missing-id with carried(avail, req),CorruptedElementTypeon negative SumItem, v1i64::MAXsentinel,error_if_does_not_existtrue/false branches inempty_prefunded_specialized_balance_operations_v0.StateTransitionvariants (DataContractCreate/Update,IdentityCreate/TopUp/Update,Unshield,ShieldedTransfer/Withdrawal) acrossname(),is_identity_signed(),signature(),owner_id(), setters (incl. shielded no-op arms),required_asset_lock_balance_for_processing_startCorruptedCodeExecutionfor 8 non-asset-lock variants, anddeserialize_from_bytes_in_versionStateTransitionIsNotActiveError(ShieldedTransfer against protocol < 12).ProofError::UnknownContract,TryTransitionIntoPathQuerykeeps_historybranches (Burn/Freeze/Unfreeze/SetPriceForDirectPurchase).checked_addoverflow viau64::MAXtransfer,withdrawal_amount > i64::MAXbranch in withdrawal/unshield,AnchorMismatcherror,From<&OrchardAddress>delegation, deterministic empty-spends error.Displayimpl optional-field branches (everycreated_at/updated_at/transferred_at/block-height/core-block-height/creator_id/empty-properties conditional),DateTime::from_timestamp_millisNone fallback foru64::MAX,DocumentV0Setters::bump_revisionNone no-op +Revision::MAXsaturation guard (overflow regression guard).from_untrusted/trusted_platform_valuemissing-$typeand unknown-type errors,serialize_specific_version_to_bytesUnknownVersionMismatch,from_bytesempty / unknown-version / truncated paths.Production bug flagged
packages/rs-drive/src/drive/prefunded_specialized_balances/deduct_from_prefunded_specialized_balance/mod.rs top-level dispatcher only has a
0 =>branch, but platform drive versions v3, v4, v5 all setdeduct_from_prefunded_specialized_balance: 1. Grepping confirms there are no callers — the production state-transition code calls*_operationsinstead (which properly supports v0+v1). This is a dead-path bug; flagged for a follow-up PR to either add the v1 branch or remove the dispatcher entirely. Out of scope for this PR.How Has This Been Tested?
cargo test -p dpp --lib 'document::v0'— 88 passcargo test -p dpp --lib 'document::extended_document'— 46 passcargo test -p dpp --lib 'document::document_factory'— 23 passcargo test -p dpp --all-features --lib 'shielded::builder'— 34 passcargo test -p dpp --lib 'state_transition::tests'— 63 passcargo test -p drive --lib 'shielded'— 136 passcargo test -p drive --lib 'prefunded_specialized_balances'— 28 passcargo test -p drive --lib 'verify::state_transition'— 35 passcargo check --tests -p dpp -p drive— cleancargo fmt -p dpp -p drive— cleanBreaking Changes
None. Tests only.
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes