test: cover document_type, drive votes/tokens/identity/shielded, drive-abci validation#3525
Merged
QuantumExplorer merged 3 commits intoApr 23, 2026
Conversation
…e-abci validation
Add 194 new unit tests across 31 files, all targeting error paths
and edge cases per the coverage-PR pattern established.
Per-target breakdown:
- rs-dpp/data_contract/document_type (5 submodules, 45 tests):
* class_methods/should_use_creator_id (50% → covered, 8 tests):
every branch of v0 short-circuit, v1 transferable/trade_mode
AND-chain, unknown-version error.
* class_methods/try_from_schema v0 (12 tests) + v1 (12 tests):
MissingPositions, DuplicateIndexName, UndefinedIndexProperty,
InvalidIndexedPropertyConstraint (string + byte_array),
InvalidTokenPosition, RedundantDocumentPaidForByTokenWithContractId,
TokenPaymentByBurningOnlyAllowedOnInternalToken error variants;
full_validation=false skip paths; schema.to_map() failure;
Transferable::try_from(u8) error; insert_values_nested for
nested object schemas; happy paths populating TokenCosts.
* methods/mod.rs (62% → covered, 9 tests): requires_revision /
initial_revision across 4 boolean branches, top_level_indices
first-property-of-each, top_level_indices_of_contested_unique
filter, sanitize_document_properties hex→bytes conversion and
unknown-fields pass-through.
* v1/mod.rs (38% → covered, 4 tests): From<DocumentTypeV0>
all-fields + TokenCostsV0 default, properties/indices
preservation, all 6 TokenCost setters, trait dispatch
(DocumentTypeBasicMethods + DocumentTypeV0Methods on V1).
- rs-dpp/data_contract/document_type/property/mod.rs (88% →
covered, ~74 tests): random_value/random_sub_filled_value/
random_filled_value for all type variants; read_optionally_from
error paths for every scalar with truncated buffers; encode
type-mismatch error arms; try_from_value_map extra branches
(string without sizes, enum-driven integer, non-identifier
content-media-type); find_integer_type_for_min_and_max_values
negative-range (I8/I16/I32/I64); sanitize_value_mut base64
fallback / size-constraint rejection / fixed-size Bytes20/32/36;
value_from_string I64/U128/I128 overflow + negative-u8 + boolean
empty + byte-array exact bytes.
- rs-drive/drive/votes/insert (67% → covered, 4 tests):
ContestedIndexNotFound + DataContractError in the operations
path for register + insert_stored_info.
- rs-drive/drive/tokens/status (70% → covered, 9 tests):
fetch-missing None, paused↔active round-trip, stateless branch,
non-Item CorruptedElementType, undecodable-Item rejection,
mixed-present-and-absent batch, empty-id-list short-circuit,
with_costs FeeResult, GroveDB InvalidQuery for limit=0 in prove.
- rs-drive/drive/address_funds/prove (75% → covered, 10 tests):
prove_balance_and_nonce round-trip + operations + absence proof
+ transactions-not-supported; prove_address_funds_branch_query
depth-below-min / depth-above-max InvalidInput + unknown-
checkpoint + no-ops-on-validation-error; prove_address_funds
_trunk_query operations-populate-ops.
- rs-drive/drive/identity/contract_info (82% → covered, 10 tests):
fetch_identity_contract_nonce no-contract-identity None + no-
identity None + after-merge round-trip + with_fees + stateless;
prove_identity_contract_nonce absent + presence-differs-from-
absence; merge_identity_contract_nonce estimation-mode success
+ stateless-layer-info + ops population.
- rs-drive/drive/shielded/nullifiers (81% → covered, 15 tests):
store empty-noop + round-trip + compaction-on-threshold +
transaction-commit semantics; fetch empty-pool + start-height
skip + limit-honored + CorruptedSerialization; fetch_compacted
limit=0 + empty + past-range + inside-range + undecodable;
compact empty + same-timestamp-append + cross-block-order;
cleanup_expired future-untouched (strict boundary) + past-
removed + undecodable-expiration.
- rs-drive-abci/execution/validation/state_transition/common
(86% → covered, 18 tests across 6 files): validate_identity
_exists absent/present + RetrieveIdentity op recording;
validate_non_masternode_identity_exists missing/present-with-
master-key; validate_identity_public_key_ids_dont_exist no-dup
+ duplicate-BasicError + empty-list; validate_identity_public
_key_ids_exist all-missing StateError + all-present + partial-
overlap reporting only missing id; asset_lock/proof/verify_is
_not_spent NotPresent + FullyConsumed + PartiallyConsumed<req
+ replay-attack-tag match + happy-path; asset_lock/transaction
/fetch_*_output_sync Instant out-of-range + Chain tx-not-found.
- rs-drive-abci/execution/validation/state_transition/check_tx
_verification (82% → covered, 1 test): DataContractCreate
with signer whose identity is absent from state →
SignatureError::IdentityNotFoundError.
- rs-drive-abci/query/document_query/v0 (86% → covered, 8 tests):
validation-ordering pins across prove=true path for
DataContractNotFound / InvalidDocumentId / InvalidWhereClause /
InvalidStartAtClause / InvalidOrderBy / AbsentDocumentType;
limit-just-over-bound rejection; documents-returned-without-
proof happy path; where-clause-on-non-indexed-field rejection.
All 194 tests pass. No production bugs surfaced.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Review GateCommit:
|
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 32 minutes and 16 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 (11)
📝 WalkthroughWalkthroughThis pull request adds comprehensive unit test coverage across multiple data-contract, state-transition validation, and drive-query modules. Tests validate control-flow behaviors for document-type class methods, property operations, identity/asset-lock validations, token operations, and nullifier management without modifying any public APIs. 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (13)
packages/rs-drive-abci/src/query/document_query/v0/mod.rs (1)
1850-1854: Nit: use already-importedWhereClause/WhereOperator.
WhereClauseandWhereOperatorare already imported at line 194, so the fully-qualifieddrive::query::prefixes here are redundant and inconsistent with the other tests in this module.♻️ Proposed tidy-up
- let bogus_clause = drive::query::WhereClause { + let bogus_clause = WhereClause { field: "thisFieldIsNotIndexed".to_string(), - operator: drive::query::WhereOperator::Equal, + operator: WhereOperator::Equal, value: Value::Text("value".to_string()), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/document_query/v0/mod.rs` around lines 1850 - 1854, Replace the redundant fully-qualified types in the bogus_clause construction with the already-imported symbols: use WhereClause instead of drive::query::WhereClause and WhereOperator instead of drive::query::WhereOperator so the test matches the other cases that rely on the module-level imports (update the bogus_clause variable construction accordingly).packages/rs-drive-abci/src/execution/validation/state_transition/common/asset_lock/transaction/fetch_asset_lock_transaction_output_sync/v0/mod.rs (1)
112-116: RedundantHashimport.
use super::*;already re-exportsdpp::dashcore::hashes::Hashfrom the parent module (line 8), making the explicituse dpp::dashcore::hashes::Hash;at line 116 redundant and likely to trigger anunused_importswarning under clippy.♻️ Proposed cleanup
use super::*; use crate::rpc::core::MockCoreRPCLike; use dpp::consensus::basic::BasicError; use dpp::consensus::ConsensusError; - use dpp::dashcore::hashes::Hash; use dpp::dashcore::transaction::special_transaction::asset_lock::AssetLockPayload;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/common/asset_lock/transaction/fetch_asset_lock_transaction_output_sync/v0/mod.rs` around lines 112 - 116, Remove the redundant explicit import of Hash: the prelude import use super::* already brings dpp::dashcore::hashes::Hash into scope, so delete the line use dpp::dashcore::hashes::Hash; (the other imports like MockCoreRPCLike, BasicError, ConsensusError should remain); after removing it, run cargo check/cargo clippy to ensure no unused_imports warnings remain.packages/rs-drive/src/drive/tokens/status/prove_token_statuses/mod.rs (2)
134-145: Strengthen the empty-list assertion.The test's stated intent is to pin the GroveDB
InvalidQuery"proved path queries can not be for limit 0" rejection, but it only binds the error to_and discards it. If a future change madeprove_token_statuses([])returnOk(vec![])or a different error kind, this test would still pass as long as any error is returned — and if it starts returningOk,expect_erris the only guard. Consider matching the variant (or at least the error message) so the test actually pins the documented behavior.♻️ Suggested tightening
- let err = drive - .prove_token_statuses(&[], None, platform_version) - .expect_err("empty list should bubble up a GroveDB InvalidQuery"); - // The exact variant is GroveDB; we just verify the error propagated and - // did not silently return a proof of nothing. - let _ = err; + let err = drive + .prove_token_statuses(&[], None, platform_version) + .expect_err("empty list should bubble up a GroveDB InvalidQuery"); + assert!( + matches!(err, Error::GroveDB(_)), + "expected GroveDB error, got {:?}", + err + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/tokens/status/prove_token_statuses/mod.rs` around lines 134 - 145, The test prove_empty_token_list_errors_from_grovedb currently discards the returned error; change it to assert the specific GroveDB "InvalidQuery" rejection instead of just any error: after calling drive.prove_token_statuses(&[], None, platform_version).expect_err(...), match or assert on the returned err from prove_token_statuses to verify it is the GroveDB InvalidQuery variant (or that err.to_string() contains the "proved path queries can not be for limit 0" message), so the test pins the documented behavior in prove_token_statuses and fails if a different error or Ok is returned.
149-164: Consider also asserting the FeeResult is non-trivial.The test name advertises that a
FeeResultis returned, but_feesis discarded. A cheapassert!(fees.processing_fee > 0 || fees.storage_fee > 0)(mirroring the sibling test infetch_token_status/mod.rs) would make the fee-path assertion actually load-bearing. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/tokens/status/prove_token_statuses/mod.rs` around lines 149 - 164, The test prove_with_costs_returns_fee_result currently ignores the returned FeeResult; change the binding from `_fees` to `fees` in the call to prove_token_statuses_with_costs and add an assertion that the fee result is non-trivial (e.g., assert!(fees.processing_fee > 0 || fees.storage_fee > 0)) so the fee-path is actually validated; locate this in the prove_with_costs_returns_fee_result test that calls Drive::prove_token_statuses_with_costs and reference the FeeResult fields processing_fee and storage_fee.packages/rs-drive/src/drive/tokens/status/fetch_token_status/v0/mod.rs (1)
202-230: Tighten the deserialization-error assertion.
fetch_rejects_undecodable_token_status_itemonly checks that some error occurred (let _ = err;). Since the comment above says the intent is to exercise theTokenStatus::deserialize_from_bytespropagation path, consider matching on the expected error variant (likelyError::Protocol(..)/ serialization error) so a regression that returns a different error — e.g.CorruptedElementTypeorGroveDB— is actually caught.♻️ Suggested tightening
- let err = drive - .fetch_token_status_v0(token_id, None, platform_version) - .expect_err("fetch should fail on garbage item"); - // Allow any deserialization error variant; the important behavior is - // that fetch does not silently succeed. - let _ = err; + let err = drive + .fetch_token_status_v0(token_id, None, platform_version) + .expect_err("fetch should fail on garbage item"); + assert!( + !matches!(err, Error::Drive(DriveError::CorruptedElementType(_))), + "expected a deserialization error, got CorruptedElementType: {:?}", + err + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/tokens/status/fetch_token_status/v0/mod.rs` around lines 202 - 230, The test fetch_rejects_undecodable_token_status_item currently only asserts that an error occurred; update it to assert that the error comes from the TokenStatus deserialization path by matching the error returned from drive.fetch_token_status_v0 against the expected serialization/protocol error variant (e.g. Error::Protocol or the specific protocol/serialization error produced by TokenStatus::deserialize_from_bytes), and fail the test if the error is a different variant such as CorruptedElementType or a GroveDB error; keep references to fetch_token_status_v0, TokenStatus::deserialize_from_bytes, Error::Protocol (or the concrete protocol error enum) when adding the pattern match/assertion.packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_non_masternode_identity_exists/v0/mod.rs (2)
74-77: Tighten the operation assertion to match the sibling test.The sibling
validate_identity_existstest explicitly asserts aValidationOperation::RetrieveIdentity(_)variant is recorded. Here,!operations_slice().is_empty()is weaker — any spurious op added in the future (or added before the fetch) would silently pass. Consider matching the pattern explicitly so the test actually validates that the key-fetch was accounted for.♻️ Suggested tightening
- assert!(!exists, "non-masternode identity should not be found"); - assert!( - !execution_context.operations_slice().is_empty(), - "should record the key-fetch operation" - ); + assert!(!exists, "non-masternode identity should not be found"); + let has_retrieve_op = execution_context + .operations_slice() + .iter() + .any(|op| matches!(op, ValidationOperation::RetrieveIdentity(_))); + assert!( + has_retrieve_op, + "should record a RetrieveIdentity validation operation" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_non_masternode_identity_exists/v0/mod.rs` around lines 74 - 77, The test currently only checks that execution_context.operations_slice() is non-empty; tighten it to assert that the recorded operation is specifically a ValidationOperation::RetrieveIdentity(...) like the sibling validate_identity_exists test does—inspect execution_context.operations_slice(), find the operation (e.g., the first entry) and assert it matches the RetrieveIdentity variant (using pattern matching or an equals/assert_matches call) so the test verifies the key-fetch was recorded, not just any operation.
80-125: Consider adding a test for an identity without a master authentication key.The current tests cover missing identities and identities with master keys. However, the function's purpose is to validate "non-masternode" identities (which lack master authentication keys). A test that creates an identity without a master authentication key and asserts the function returns
falsewould verify the core semantic the validator is designed to enforce.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_non_masternode_identity_exists/v0/mod.rs` around lines 80 - 125, Add a test that verifies validate_non_masternode_identity_exists_v0 returns false for an identity that does NOT have a master authentication key: create a new test (e.g., should_return_false_when_identity_without_master_key_present) in the same test module, build a TestPlatformBuilder and genesis state like the existing test, create or construct an Identity that lacks a master authentication key (either via an available helper like Identity::random_identity_without_master_key or by removing the master key from a generated identity), add it to the drive with drive.add_new_identity(...), create a StateTransitionExecutionContext, call validate_non_masternode_identity_exists_v0(&platform.drive, &identity_id, &mut execution_context, None, platform_version) and assert the result is false (expect no error).packages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rs (1)
679-681: Remove the dummy mutable borrow.
platformis only read after construction, so the finallet _ = &mut platformcan be dropped along withmut. This keeps the test cleaner and avoids masking future borrow/lint issues.Proposed cleanup
- let mut platform = TestPlatformBuilder::new() + let platform = TestPlatformBuilder::new() .build_with_mock_rpc() .set_genesis_state(); ... - // Silence unused: platform is mutable only to match existing setup function style. - let _ = &mut platform;Also applies to: 746-747
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rs` around lines 679 - 681, The test unnecessarily declares platform as mutable and performs a dummy mutable borrow; remove the mut from the let binding where you construct the platform (the chain using TestPlatformBuilder::new().build_with_mock_rpc().set_genesis_state()) and delete the trailing dummy statement like let _ = &mut platform; (also apply the same removal at the analogous lines around the second occurrence). This keeps platform immutable and removes the unneeded mutable borrow.packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_ids_exist_in_state/v0/mod.rs (1)
205-219: Optional: asserterrors.len() == 1before indexing.Test 3 indexes
result.errors[0]without first assertingerrors.len(), unlike the all-missing test (Line 90). If the validator ever produces additional errors, the match arm would still pass but silently skip them. Consider addingassert_eq!(result.errors.len(), 1);for consistency withshould_fail_when_identity_has_no_keys_at_requested_ids.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_ids_exist_in_state/v0/mod.rs` around lines 205 - 219, Add an explicit length check before indexing result.errors: assert_eq!(result.errors.len(), 1); so the test verifies there is exactly one error before matching on result.errors[0]; place this assertion immediately before the match that expects ConsensusError::StateError(StateError::MissingIdentityPublicKeyIdsError(_)) to ensure additional errors aren’t silently ignored when inspecting the missing ids (variables to locate: result, result.errors, missing_id, existing_id, and the match arm for MissingIdentityPublicKeyIdsError).packages/rs-dpp/src/data_contract/document_type/property/mod.rs (3)
6761-6788: Assert element sanitization, not just array length.Both tests would still pass if nested sanitization were a no-op. Use values that visibly change after
item_type.sanitize_value_mut(item)and assert the transformed elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/property/mod.rs` around lines 6761 - 6788, The tests test_sanitize_value_mut_array_elements and test_sanitize_value_mut_variable_type_array_elements currently only assert array length, which misses whether item_type.sanitize_value_mut actually transforms elements; update both tests (referencing DocumentPropertyType::Array, DocumentPropertyType::VariableTypeArray, ArrayItemType and its sanitize_value_mut) to use element Values that visibly change when sanitized (e.g. a string with surrounding whitespace that becomes trimmed or a numeric string that becomes an integer, or another item-type-specific transformation) and assert the transformed element values rather than just the array length so the nested sanitization behavior is validated.
5744-5750: UseRangeInclusive::containsfor range assertions.Replace manual
>= && <=comparisons with idiomatic range syntax to keep the tests clippy-clean:Suggested cleanup
- s.len() >= 5 && s.len() <= 10, + (5..=10).contains(&s.len()),- assert!(b.len() >= 1 && b.len() <= 10); + assert!((1..=10).contains(&b.len()));- assert!(sz >= 3 && sz <= 6); + assert!((3..=6).contains(&sz));Affects lines 5746, 5824, 6112.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/property/mod.rs` around lines 5744 - 5750, Replace manual range checks on string length with RangeInclusive::contains: where the test currently does if let Value::Text(s) = prop.random_value(&mut rng) { assert!(s.len() >= 5 && s.len() <= 10, ...); ... }, change the assertion to use (5..=10).contains(&s.len()). Apply the same replacement for the other occurrences referenced (around usages of prop.random_value, Value::Text and s.len()) so all three assertions use (min..=max).contains(&s.len()) and keep the other checks (e.g., s.chars().all(...)) unchanged.
6586-6600: Add coverage for a negative single-value integer enum.This positive-only case won’t catch the
OneElement(-1)path, which currently flows through unsigned type selection. Add a regression test or explicitly document that behavior if intentional.🧪 Suggested test case
fn test_try_from_value_map_integer_with_enum_single_value() { // A single-element enum picks the unsigned type for that max let type_val = Value::Text("integer".to_string()); let enum_val = Value::Array(vec![Value::I64(300)]); @@ // 300 => U16 assert_eq!(result, DocumentPropertyType::U16); } + + #[test] + fn test_try_from_value_map_integer_with_enum_single_negative_value() { + let type_val = Value::Text("integer".to_string()); + let enum_val = Value::Array(vec![Value::I64(-1)]); + let mut map = BTreeMap::new(); + map.insert("type".to_string(), &type_val); + map.insert("enum".to_string(), &enum_val); + let options = DocumentPropertyTypeParsingOptions { + sized_integer_types: true, + }; + + let result = DocumentPropertyType::try_from_value_map(&map, &options).unwrap(); + + assert_eq!(result, DocumentPropertyType::I8); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/property/mod.rs` around lines 6586 - 6600, The current test test_try_from_value_map_integer_with_enum_single_value only checks a positive single-element enum and misses the negative OneElement path in DocumentPropertyType::try_from_value_map; add a new test (e.g., test_try_from_value_map_integer_with_enum_single_negative_value) that supplies enum = [-1] and asserts the expected signed DocumentPropertyType (or, if unsigned selection is intentional, assert that behavior and add documentation), and update DocumentPropertyType::try_from_value_map to detect negative single-element integer enums and select an appropriate signed variant (or explicitly document/guard the unsigned selection) so the behavior is deterministic and covered by tests.packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/merge_identity_contract_nonce/v0/mod.rs (1)
655-668: Assert the exact success variant and nonce.These tests are meant to cover
MergeIdentityNonceSuccess, buterror_message().is_none()is weaker than matching the enum payload. Matching the variant also confirms the requested nonce is the value being returned.🧪 Proposed tightening
+ use crate::drive::identity::contract_info::identity_contract_nonce::merge_identity_contract_nonce::MergeIdentityNonceResult::MergeIdentityNonceSuccess; + let result = drive .merge_identity_contract_nonce_v0( [1u8; 32], @@ ) .expect("estimation should succeed"); - // Error-free success result. - assert!(result.error_message().is_none()); + assert!(matches!(result, MergeIdentityNonceSuccess(3))); @@ + use crate::drive::identity::contract_info::identity_contract_nonce::merge_identity_contract_nonce::MergeIdentityNonceResult::MergeIdentityNonceSuccess; + let (result, ops) = drive .merge_identity_contract_nonce_operations_v0( [2u8; 32], @@ ) .expect("estimation ops"); - assert!(result.error_message().is_none()); + assert!(matches!(result, MergeIdentityNonceSuccess(5)));Also applies to: 688-699
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/merge_identity_contract_nonce/v0/mod.rs` around lines 655 - 668, The test currently only checks result.error_message().is_none(), but should assert the exact success variant and returned nonce to ensure MergeIdentityNonceSuccess is returned with the requested nonce; update the assertion after calling merge_identity_contract_nonce_v0 to pattern-match the result (e.g., match on the success enum variant or use an assert_eq! on result into a MergeIdentityNonceSuccess payload) and verify the contained nonce equals 3 (and similarly tighten the other occurrence around lines 688-699), referencing merge_identity_contract_nonce_v0, MergeIdentityNonceSuccess, and the result variable to locate and change the assertions.
🤖 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/document_type/property/mod.rs`:
- Around line 6298-6330: The test currently uses inner_bytes = vec![0] which
consumes an absence marker and doesn't exercise the finished-buffer branch;
change inner_bytes to an empty Vec (inner_bytes = vec![]) so that when
prop.read_optionally_from(&mut reader, true) is called the optional field read
returns finished = true and the subsequent required field triggers the "required
field after finished buffer in object" error; update the data length encoding
and reader setup around inner_bytes accordingly in the test function
test_read_optionally_from_object_required_field_after_finished_buffer and keep
the call to prop.read_optionally_from unchanged.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/common/asset_lock/proof/verify_is_not_spent/v0/mod.rs`:
- Line 135: Replace uses of PlatformVersion::latest() in this v0 verification
module with an explicit, deterministic v0 constructor (e.g.,
PlatformVersion::first() or the concrete v0 constructor used elsewhere) so the
tests are pinned to the v0 protocol; update each occurrence of
PlatformVersion::latest() in this file (the verify_is_not_spent v0
implementation) — including the other instances noted — to
PlatformVersion::first() (or the repository's explicit v0 constructor) so tests
no longer drift as the protocol evolves.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_ids_dont_exist_in_state/v0/mod.rs`:
- Around line 185-209: Add a short comment in the test function
should_pass_when_empty_key_list explaining the guaranteed no-op contract: when
identity_public_keys_with_witness is empty, constructing SpecificKeys(vec![])
produces an empty query that the drive fetch API treats as a trivial no-op
(returns empty result set), so the test validates this behavior and no
additional short-circuiting is required; reference the
validate_identity_public_key_ids_dont_exist_in_state_v0 call and the
SpecificKeys(vec![]) behavior, and optionally cite
test_key_id_vec_try_from_path_key_optional_empty() as the existing assertion of
this contract.
In
`@packages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rs`:
- Around line 153-154: The comment above
prove_address_funds_branch_query_operations_v0 is stale: it says the function
"populates drive_operations even when the underlying call errors, provided
validation passed" but the test actually asserts validation short-circuits and
leaves ops empty; update the doc comment for
prove_address_funds_branch_query_operations_v0 to state that when validation
fails/short-circuits the function does not populate ops (ops remains empty),
referencing the ops variable and the validation short-circuit behavior so the
comment matches the test's intent.
In
`@packages/rs-drive/src/drive/address_funds/prove/prove_address_funds_trunk_query/v0/mod.rs`:
- Around line 106-109: Replace the current is_ok() parity check with a
comparison of the actual dispatcher outputs: call
drive.prove_address_funds_trunk_query(platform_version) and
drive.prove_address_funds_trunk_query_v0(platform_version), assert both return
Ok, unwrap both results and compare the proof bytes/content for equality (use
the proof/bytes field or serialized form from the returned type) so the
dispatcher and v0 path produce identical proofs; if you also want to cover error
parity, add a branch that asserts both are Err and optionally compare error
kinds/messages.
- Around line 81-96: The test currently accepts any Err from
prove_address_funds_trunk_query_operations_v0 which hides regressions; change
the Err arm so it only permits the known "not supported / checkpoints
uninitialized" failure and still verifies drive_operations was populated: when
result is Err(e) assert!(!drive_operations.is_empty(), "drive_operations should
be observable even when trunk query is unsupported"); then also assert that e
indicates the expected unsupported condition (e.g. check e's error kind or
e.to_string() contains "trunk" or "checkpoint" or matches the specific error
variant used by prove_address_funds_trunk_query_operations_v0), otherwise panic
so the test fails on unexpected errors.
In
`@packages/rs-drive/src/drive/address_funds/prove/prove_balance_and_nonce/v0/mod.rs`:
- Around line 122-125: The test currently only checks that the proof bytes from
prove_balance_and_nonce_v0(&ADDR_MISSING, None, platform_version) are non-empty;
instead, pass that proof into drive.verify_address_info (using the same
platform_version and the address or proof as required by verify_address_info)
and assert the verification returns an Ok(Some(...)) or Ok(None) as appropriate
— specifically assert that verify_address_info reports the address as missing
(i.e., returns None for the address info) to ensure the proof is a valid absence
proof for ADDR_MISSING.
In
`@packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/fetch_identity_contract_nonce/v0/mod.rs`:
- Around line 121-122: The test module lacks the server feature guard causing a
feature mismatch when calling setup_drive(); wrap the module declaration "mod
tests" in the same conditional as the adjacent merge tests by adding
#[cfg(feature = "server")] above the module so the tests that call setup_drive()
are only compiled when the "server" feature (and transitively "full") is
enabled; update the tests module containing references to setup_drive() in
identity_contract_nonce/fetch_identity_contract_nonce/v0 to use that feature
guard to match merge_identity_contract_nonce.
In
`@packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/prove_identity_contract_nonce/v0/mod.rs`:
- Around line 55-63: The test currently only checks proof bytes but doesn't
verify semantics; after calling
Drive::prove_identity_contract_nonce_v0(identity.id().to_buffer(), ...), call
Drive::verify_identity_contract_nonce(...) to assert the verified nonce matches
expectation (assert None for the absence case), and in the later section call
verify after the merge to assert Some(1); update both the absence-block (lines
with prove_identity_contract_nonce_v0 and assert!(!proof.is_empty())) and the
before/after-merge blocks to replace or augment raw-byte assertions with
Drive::verify_identity_contract_nonce checks using the same
identity.id().to_buffer() and platform_version.drive parameters.
In
`@packages/rs-drive/src/drive/shielded/nullifiers/cleanup_expired_nullifier_compactions/v0/mod.rs`:
- Around line 244-276: The test currently verifies the compacted entry is
removed but not that the expiration entry was deleted; after the existing
assertions, call cleanup_expired_nullifier_compactions_v0 again with the same
current timestamp and platform_version and assert it returns 0 to prove the
expiration entry was removed (use the existing drive variable and the
cleanup_expired_nullifier_compactions_v0 function); keep the existing calls to
compact_nullifiers_with_current_block_v0 and fetch_compacted_nullifier_changes
as context.
- Around line 279-304: The test cleanup_rejects_undecodable_expiration_payload
currently asserts a broad Error::Protocol(_) which can match unrelated protocol
errors; update the assertion to match the exact corrupted-serialization protocol
variant produced by NullifierExpirationRanges::decode (e.g., assert that err
matches Error::Protocol(ProtocolError::CorruptedSerialization(_)) or the
project’s exact enum variant name) so the test only passes for the intended
CorruptedSerialization error coming from
cleanup_expired_nullifier_compactions_v0.
In
`@packages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/v0/mod.rs`:
- Around line 206-238: The test currently only asserts both compacted rows exist
after calling compact_nullifiers_with_current_block_v0 twice; to verify the
second compaction appended to the shared expiration payload (instead of
overwriting it), after the second compact call invoke the Drive cleanup routine
that removes expired compacted nullifier changes for the expiration key tied to
block time 1_000 (e.g., cleanup_compacted_nullifier_changes or
cleanup_expired_compacted_nullifier_changes) and then call
fetch_compacted_nullifier_changes to assert those ranges were both removed (or
assert the cleanup returned a count of 2), proving the payload contained both
ranges and was cleaned up together.
In `@packages/rs-drive/src/drive/shielded/nullifiers/store_nullifiers/v0/mod.rs`:
- Around line 219-235: The test compaction_triggers_on_nullifier_threshold
hard-codes 2048 which can drift from the protocol value; change it to derive the
threshold from the platform version used in the test (you already call
PlatformVersion::latest()), e.g. read the platform-versioned
max_nullifiers_before_compaction value from the PlatformVersion instance and use
that to build the nullifiers vector and expectations; update references in the
test (compaction_triggers_on_nullifier_threshold and the call to
store_nullifiers_for_block_v0) so the count equals platform_version's
max_nullifiers_before_compaction and any assertions about recent/compacted tree
sizes use that derived value instead of 2048.
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/property/mod.rs`:
- Around line 6761-6788: The tests test_sanitize_value_mut_array_elements and
test_sanitize_value_mut_variable_type_array_elements currently only assert array
length, which misses whether item_type.sanitize_value_mut actually transforms
elements; update both tests (referencing DocumentPropertyType::Array,
DocumentPropertyType::VariableTypeArray, ArrayItemType and its
sanitize_value_mut) to use element Values that visibly change when sanitized
(e.g. a string with surrounding whitespace that becomes trimmed or a numeric
string that becomes an integer, or another item-type-specific transformation)
and assert the transformed element values rather than just the array length so
the nested sanitization behavior is validated.
- Around line 5744-5750: Replace manual range checks on string length with
RangeInclusive::contains: where the test currently does if let Value::Text(s) =
prop.random_value(&mut rng) { assert!(s.len() >= 5 && s.len() <= 10, ...); ...
}, change the assertion to use (5..=10).contains(&s.len()). Apply the same
replacement for the other occurrences referenced (around usages of
prop.random_value, Value::Text and s.len()) so all three assertions use
(min..=max).contains(&s.len()) and keep the other checks (e.g.,
s.chars().all(...)) unchanged.
- Around line 6586-6600: The current test
test_try_from_value_map_integer_with_enum_single_value only checks a positive
single-element enum and misses the negative OneElement path in
DocumentPropertyType::try_from_value_map; add a new test (e.g.,
test_try_from_value_map_integer_with_enum_single_negative_value) that supplies
enum = [-1] and asserts the expected signed DocumentPropertyType (or, if
unsigned selection is intentional, assert that behavior and add documentation),
and update DocumentPropertyType::try_from_value_map to detect negative
single-element integer enums and select an appropriate signed variant (or
explicitly document/guard the unsigned selection) so the behavior is
deterministic and covered by tests.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rs`:
- Around line 679-681: The test unnecessarily declares platform as mutable and
performs a dummy mutable borrow; remove the mut from the let binding where you
construct the platform (the chain using
TestPlatformBuilder::new().build_with_mock_rpc().set_genesis_state()) and delete
the trailing dummy statement like let _ = &mut platform; (also apply the same
removal at the analogous lines around the second occurrence). This keeps
platform immutable and removes the unneeded mutable borrow.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/common/asset_lock/transaction/fetch_asset_lock_transaction_output_sync/v0/mod.rs`:
- Around line 112-116: Remove the redundant explicit import of Hash: the prelude
import use super::* already brings dpp::dashcore::hashes::Hash into scope, so
delete the line use dpp::dashcore::hashes::Hash; (the other imports like
MockCoreRPCLike, BasicError, ConsensusError should remain); after removing it,
run cargo check/cargo clippy to ensure no unused_imports warnings remain.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_ids_exist_in_state/v0/mod.rs`:
- Around line 205-219: Add an explicit length check before indexing
result.errors: assert_eq!(result.errors.len(), 1); so the test verifies there is
exactly one error before matching on result.errors[0]; place this assertion
immediately before the match that expects
ConsensusError::StateError(StateError::MissingIdentityPublicKeyIdsError(_)) to
ensure additional errors aren’t silently ignored when inspecting the missing ids
(variables to locate: result, result.errors, missing_id, existing_id, and the
match arm for MissingIdentityPublicKeyIdsError).
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_non_masternode_identity_exists/v0/mod.rs`:
- Around line 74-77: The test currently only checks that
execution_context.operations_slice() is non-empty; tighten it to assert that the
recorded operation is specifically a ValidationOperation::RetrieveIdentity(...)
like the sibling validate_identity_exists test does—inspect
execution_context.operations_slice(), find the operation (e.g., the first entry)
and assert it matches the RetrieveIdentity variant (using pattern matching or an
equals/assert_matches call) so the test verifies the key-fetch was recorded, not
just any operation.
- Around line 80-125: Add a test that verifies
validate_non_masternode_identity_exists_v0 returns false for an identity that
does NOT have a master authentication key: create a new test (e.g.,
should_return_false_when_identity_without_master_key_present) in the same test
module, build a TestPlatformBuilder and genesis state like the existing test,
create or construct an Identity that lacks a master authentication key (either
via an available helper like Identity::random_identity_without_master_key or by
removing the master key from a generated identity), add it to the drive with
drive.add_new_identity(...), create a StateTransitionExecutionContext, call
validate_non_masternode_identity_exists_v0(&platform.drive, &identity_id, &mut
execution_context, None, platform_version) and assert the result is false
(expect no error).
In `@packages/rs-drive-abci/src/query/document_query/v0/mod.rs`:
- Around line 1850-1854: Replace the redundant fully-qualified types in the
bogus_clause construction with the already-imported symbols: use WhereClause
instead of drive::query::WhereClause and WhereOperator instead of
drive::query::WhereOperator so the test matches the other cases that rely on the
module-level imports (update the bogus_clause variable construction
accordingly).
In
`@packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/merge_identity_contract_nonce/v0/mod.rs`:
- Around line 655-668: The test currently only checks
result.error_message().is_none(), but should assert the exact success variant
and returned nonce to ensure MergeIdentityNonceSuccess is returned with the
requested nonce; update the assertion after calling
merge_identity_contract_nonce_v0 to pattern-match the result (e.g., match on the
success enum variant or use an assert_eq! on result into a
MergeIdentityNonceSuccess payload) and verify the contained nonce equals 3 (and
similarly tighten the other occurrence around lines 688-699), referencing
merge_identity_contract_nonce_v0, MergeIdentityNonceSuccess, and the result
variable to locate and change the assertions.
In `@packages/rs-drive/src/drive/tokens/status/fetch_token_status/v0/mod.rs`:
- Around line 202-230: The test fetch_rejects_undecodable_token_status_item
currently only asserts that an error occurred; update it to assert that the
error comes from the TokenStatus deserialization path by matching the error
returned from drive.fetch_token_status_v0 against the expected
serialization/protocol error variant (e.g. Error::Protocol or the specific
protocol/serialization error produced by TokenStatus::deserialize_from_bytes),
and fail the test if the error is a different variant such as
CorruptedElementType or a GroveDB error; keep references to
fetch_token_status_v0, TokenStatus::deserialize_from_bytes, Error::Protocol (or
the concrete protocol error enum) when adding the pattern match/assertion.
In `@packages/rs-drive/src/drive/tokens/status/prove_token_statuses/mod.rs`:
- Around line 134-145: The test prove_empty_token_list_errors_from_grovedb
currently discards the returned error; change it to assert the specific GroveDB
"InvalidQuery" rejection instead of just any error: after calling
drive.prove_token_statuses(&[], None, platform_version).expect_err(...), match
or assert on the returned err from prove_token_statuses to verify it is the
GroveDB InvalidQuery variant (or that err.to_string() contains the "proved path
queries can not be for limit 0" message), so the test pins the documented
behavior in prove_token_statuses and fails if a different error or Ok is
returned.
- Around line 149-164: The test prove_with_costs_returns_fee_result currently
ignores the returned FeeResult; change the binding from `_fees` to `fees` in the
call to prove_token_statuses_with_costs and add an assertion that the fee result
is non-trivial (e.g., assert!(fees.processing_fee > 0 || fees.storage_fee > 0))
so the fee-path is actually validated; locate this in the
prove_with_costs_returns_fee_result test that calls
Drive::prove_token_statuses_with_costs and reference the FeeResult fields
processing_fee and storage_fee.
🪄 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: 0e910108-e36b-40fd-9ddf-07773d009266
📒 Files selected for processing (31)
packages/rs-dpp/src/data_contract/document_type/class_methods/should_use_creator_id/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/mod.rspackages/rs-dpp/src/data_contract/document_type/property/mod.rspackages/rs-dpp/src/data_contract/document_type/v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/asset_lock/proof/verify_is_not_spent/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/asset_lock/transaction/fetch_asset_lock_transaction_output_sync/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_exists/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_ids_dont_exist_in_state/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_ids_exist_in_state/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/common/validate_non_masternode_identity_exists/v0/mod.rspackages/rs-drive-abci/src/query/document_query/v0/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_branch_query/v0/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_address_funds_trunk_query/v0/mod.rspackages/rs-drive/src/drive/address_funds/prove/prove_balance_and_nonce/v0/mod.rspackages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/fetch_identity_contract_nonce/v0/mod.rspackages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/merge_identity_contract_nonce/v0/mod.rspackages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/prove_identity_contract_nonce/v0/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/cleanup_expired_nullifier_compactions/v0/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/v0/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/fetch_compacted_nullifiers/v0/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/fetch_nullifiers/v0/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/store_nullifiers/v0/mod.rspackages/rs-drive/src/drive/tokens/status/fetch_token_status/mod.rspackages/rs-drive/src/drive/tokens/status/fetch_token_status/v0/mod.rspackages/rs-drive/src/drive/tokens/status/fetch_token_statuses/v0/mod.rspackages/rs-drive/src/drive/tokens/status/prove_token_statuses/mod.rspackages/rs-drive/src/drive/votes/insert/contested_resource/individual_vote/register_contested_resource_identity_vote/v0/mod.rspackages/rs-drive/src/drive/votes/insert/contested_resource/insert_stored_info_for_contested_resource_vote_poll/v0/mod.rs
|
|
||
| #[test] | ||
| fn should_return_valid_empty_result_when_not_present() { | ||
| let platform_version = PlatformVersion::latest(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the repository's existing pattern for explicit protocol-version test fixtures.
# Expected: Prefer an explicit version constructor/helper used by other version-specific tests
# instead of PlatformVersion::latest() in this v0 test module.
rg -n -C2 --type=rust 'PlatformVersion::(latest|get|first|new|default)\('
# Also locate version-specific test modules that already avoid latest().
rg -n -C3 --type=rust 'mod tests|verify_.*_v0|PlatformVersion::latest\(\)'Repository: dashpay/platform
Length of output: 50374
🏁 Script executed:
# First, let's look at the file structure and the specific lines mentioned
wc -l packages/rs-drive-abci/src/execution/validation/state_transition/common/asset_lock/proof/verify_is_not_spent/v0/mod.rsRepository: dashpay/platform
Length of output: 182
🏁 Script executed:
# Now let's examine the specific lines mentioned in the review (135, 171, 220, 268, 326)
sed -n '130,140p; 166,176p; 215,225p; 263,273p; 321,331p' packages/rs-drive-abci/src/execution/validation/state_transition/common/asset_lock/proof/verify_is_not_spent/v0/mod.rsRepository: dashpay/platform
Length of output: 1729
🏁 Script executed:
# Let's search for similar v0 test modules and how they handle version initialization
find packages/rs-drive-abci -path '*v0/mod.rs' -type f | head -10 | xargs -I {} sh -c 'echo "=== {} ===" && rg "PlatformVersion::" {} | head -5'Repository: dashpay/platform
Length of output: 1927
🏁 Script executed:
# Check what the rs-platform-version crate actually provides
rg -A 5 "pub fn (get|latest|first)" packages/rs-platform-version/src/ --type=rust | head -40Repository: dashpay/platform
Length of output: 3729
Pin these v0 tests to an explicit protocol version instead of latest().
These tests live in a v0 implementation but rely on PlatformVersion::latest(), which drifts as the protocol evolves. Similar v0 modules in the codebase (e.g., init_chain/v0) use PlatformVersion::first() for deterministic, version-specific testing. Replace all instances of PlatformVersion::latest() with an explicit version constructor.
Also applies to: 171-171, 220-220, 268-268, 326-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/common/asset_lock/proof/verify_is_not_spent/v0/mod.rs`
at line 135, Replace uses of PlatformVersion::latest() in this v0 verification
module with an explicit, deterministic v0 constructor (e.g.,
PlatformVersion::first() or the concrete v0 constructor used elsewhere) so the
tests are pinned to the v0 protocol; update each occurrence of
PlatformVersion::latest() in this file (the verify_is_not_spent v0
implementation) — including the other instances noted — to
PlatformVersion::first() (or the repository's explicit v0 constructor) so tests
no longer drift as the protocol evolves.
Comment on lines
+55
to
+63
| let proof = drive | ||
| .prove_identity_contract_nonce_v0( | ||
| identity.id().to_buffer(), | ||
| [0u8; 32], | ||
| None, | ||
| &platform_version.drive, | ||
| ) | ||
| .expect("prove absent"); | ||
| assert!(!proof.is_empty()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find existing proof-verification helpers to use in these tests.
rg -n -C3 'rs_drive_proof_verifier|drive_proof_verifier|verify_.*proof|prove_identity_contract_nonce|identity_contract_nonce.*proof' --type=rustRepository: dashpay/platform
Length of output: 50373
🏁 Script executed:
# Read the file to understand the context of lines 55-63 and 88-120
wc -l packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/prove_identity_contract_nonce/v0/mod.rsRepository: dashpay/platform
Length of output: 178
🏁 Script executed:
# Read the target file with context around the specified lines
head -130 packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/prove_identity_contract_nonce/v0/mod.rs | tail -80Repository: dashpay/platform
Length of output: 2551
🏁 Script executed:
# Search for proof verification helpers related to identity contract nonce in rs-drive
rg -n 'verify.*proof|verify_identity.*nonce|identity_contract.*nonce' packages/rs-drive --type=rust | head -50Repository: dashpay/platform
Length of output: 7204
🏁 Script executed:
# Look for proof verification implementations in rs-drive-proof-verifier
fd -type f -name '*.rs' packages/rs-drive-proof-verifier 2>/dev/null | head -20Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Look for identity-related verification in rs-drive/src/verify
find packages/rs-drive/src/verify -type f -name '*.rs' | head -20Repository: dashpay/platform
Length of output: 1583
🏁 Script executed:
# Search for GroveDb verification methods that might apply to identity nonce proofs
rg -n 'verify_query|verify_subset_query|GroveDb::verify' packages/rs-drive --type=rust | grep -i 'identity\|nonce' | head -20Repository: dashpay/platform
Length of output: 3090
🏁 Script executed:
# Look for any existing proof verification for identity_contract_nonce specifically
rg -n 'identity_contract_nonce' packages/rs-drive --type=rust -A 5 | head -80Repository: dashpay/platform
Length of output: 10585
🏁 Script executed:
# Check if rs-drive-proof-verifier has identity-specific verification
find packages -name 'rs-drive-proof-verifier' -type dRepository: dashpay/platform
Length of output: 92
🏁 Script executed:
# Look for identity nonce verification functions in rs-drive-proof-verifier
find packages/rs-drive-proof-verifier -type f -name '*.rs' | xargs grep -l 'identity.*nonce\|contract.*nonce' 2>/dev/nullRepository: dashpay/platform
Length of output: 151
🏁 Script executed:
# Check what verification modules are available in rs-drive-proof-verifier
ls -la packages/rs-drive-proof-verifier/src/ | head -30Repository: dashpay/platform
Length of output: 719
🏁 Script executed:
# Search for IdentityContractNonce in rs-drive-proof-verifier
rg -n 'IdentityContractNonce' packages/rs-drive-proof-verifier --type=rustRepository: dashpay/platform
Length of output: 742
🏁 Script executed:
# Look at the full file to understand if there's a way to verify the proof with semantics
head -60 packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/prove_identity_contract_nonce/v0/mod.rsRepository: dashpay/platform
Length of output: 2060
🏁 Script executed:
# Look for verify_identity_nonce to understand verification pattern
cat packages/rs-drive/src/verify/identity/verify_identity_nonce/v0/mod.rsRepository: dashpay/platform
Length of output: 6916
🏁 Script executed:
# Check if there's a verify_identity_contract_nonce in the codebase
rg -n 'verify_identity_contract_nonce\|verify.*contract.*nonce' packages/rs-drive --type=rust | head -20Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Look at the full test file to see if there are any verification attempts
tail -70 packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/prove_identity_contract_nonce/v0/mod.rsRepository: dashpay/platform
Length of output: 2219
🏁 Script executed:
# Check if there's a verify_identity_contract_nonce function anywhere
find packages/rs-drive/src -type f -name '*.rs' | xargs grep -l 'verify_identity_contract_nonce' 2>/dev/nullRepository: dashpay/platform
Length of output: 259
🏁 Script executed:
# Look at the pattern used in verify_identity_nonce to see what contract_nonce would need
grep -A 30 'fn verify_identity_nonce' packages/rs-drive/src/verify/identity/verify_identity_nonce/v0/mod.rs | head -40Repository: dashpay/platform
Length of output: 1477
🏁 Script executed:
# Check how the prove_identity_contract_nonce is defined and what path it uses
grep -B 5 -A 20 'identity_contract_nonce_query\|contract_nonce_query' packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/prove_identity_contract_nonce/v0/mod.rsRepository: dashpay/platform
Length of output: 932
🏁 Script executed:
# Look at the verify_identity_contract_nonce implementation
cat packages/rs-drive/src/verify/identity/verify_identity_contract_nonce/v0/mod.rsRepository: dashpay/platform
Length of output: 7460
🏁 Script executed:
# Check the public interface in mod.rs
cat packages/rs-drive/src/verify/identity/verify_identity_contract_nonce/mod.rsRepository: dashpay/platform
Length of output: 3008
🏁 Script executed:
# Look at how identity verify module is exposed
grep -A 10 'verify_identity_contract_nonce' packages/rs-drive/src/verify/identity/mod.rsRepository: dashpay/platform
Length of output: 366
🏁 Script executed:
# Show the full test file content to see both tests side by side
cat packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/prove_identity_contract_nonce/v0/mod.rsRepository: dashpay/platform
Length of output: 3982
Verify proof semantics by using Drive::verify_identity_contract_nonce() for assertion.
The tests currently only check proof bytes (!proof.is_empty() and proof_before != proof_after), which do not guarantee the proof verifies the claimed state. Use the existing Drive::verify_identity_contract_nonce() function to semantically verify absence/presence:
- Line 55-63: After proving, call
Drive::verify_identity_contract_nonce()and assert the nonce isNone(absence case). - Line 88-120: After proving before merge, verify and assert
None; after merge and proving, verify and assertSome(1).
The verification function already exists in the codebase (packages/rs-drive/src/verify/identity/verify_identity_contract_nonce/v0/mod.rs) and follows the same pattern as identity nonce verification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/prove_identity_contract_nonce/v0/mod.rs`
around lines 55 - 63, The test currently only checks proof bytes but doesn't
verify semantics; after calling
Drive::prove_identity_contract_nonce_v0(identity.id().to_buffer(), ...), call
Drive::verify_identity_contract_nonce(...) to assert the verified nonce matches
expectation (assert None for the absence case), and in the later section call
verify after the merge to assert Some(1); update both the absence-block (lines
with prove_identity_contract_nonce_v0 and assert!(!proof.is_empty())) and the
before/after-merge blocks to replace or augment raw-byte assertions with
Drive::verify_identity_contract_nonce checks using the same
identity.id().to_buffer() and platform_version.drive parameters.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3525 +/- ##
============================================
+ Coverage 87.55% 87.84% +0.28%
============================================
Files 2474 2474
Lines 288832 292800 +3968
============================================
+ Hits 252888 257196 +4308
+ Misses 35944 35604 -340
🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
deleted the
test/coverage-document-type-drive-identity-shielded
branch
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
Improve code coverage for
rs-dppdocument_type internals,rs-drivevoting/tokens/identity/shielded modules, andrs-drive-abcistate-transition validation — targeting error paths and edge cases that were previously unexercised.What was done
Added 194 new unit tests across 31 files. All tests are
#[cfg(test)]additions — no production code was modified.rs-dpp— Document type (45 tests)data_contract/document_type/class_methods/should_use_creator_id— v0 short-circuit, v1 AND-chain, unknown-version error (8 tests)data_contract/document_type/class_methods/try_from_schema/v0— MissingPositions, DuplicateIndexName, UndefinedIndexProperty, InvalidIndexedPropertyConstraint (12 tests)data_contract/document_type/class_methods/try_from_schema/v1— InvalidTokenPosition, RedundantDocumentPaidForByTokenWithContractId, TokenPaymentByBurningOnlyAllowedOnInternalToken (12 tests)data_contract/document_type/methods—requires_revision,top_level_indices,sanitize_document_properties(9 tests)data_contract/document_type/v1—From<DocumentTypeV0>, TokenCost setters (4 tests)rs-dpp— Document type property (74 tests)data_contract/document_type/property/mod.rs—random_value,random_sub_filled_value,random_filled_value,read_optionally_fromerror paths, encode type-mismatches,try_from_value_map,sanitize_value_mutrs-drive— Votes, tokens, address_funds, identity, shielded (48 tests)drive/votes/insert/contested_resource/**/v0— ContestedIndexNotFound + DataContractError surfaces (4 tests)drive/tokens/status/*/v0— fetch-missing, paused↔active, stateless, CorruptedElementType (9 tests)drive/address_funds/prove/*/v0—prove_balance_and_nonce,branch_query,trunk_query(10 tests)drive/identity/contract_info/identity_contract_nonce/*/v0— insert/fetch/update/merkle (10 tests)drive/shielded/nullifiers/*/v0— store, fetch, compact, cleanup (15 tests)rs-drive-abci— Validation + query (27 tests)execution/validation/state_transition/common/**/v0— common validation helpers (18 tests)execution/validation/state_transition/check_tx_verification/v0(1 test)query/document_query/v0— document-query edge cases (8 tests)How Has This Been Tested?
All new tests pass locally.
Breaking Changes
None — tests only.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit