test: cover query handlers, platform_events, state_transition verify, document v0#3528
Merged
Merged
Conversation
Adds 56 tests targeting uncovered paths in rs-dpp/src/document/: - platform_value_conversion.rs: round-trip tests for to_map_value, into_map_value, to_object, into_value, and from_platform_value (including non-map error path). Previously had zero tests. - serialize.rs: missing-required-field errors for serialize_v0/v1/v2 using the withdrawal contract (requires $createdAt/$updatedAt) and family/person contract (required user properties). Direct from_bytes_v0/v1/v2 too-small-buffer errors, v1/v2 truncated-post-id errors, V0-then-V1 fallback path, V0-contract feature_version gating. - cbor_conversion.rs: from_map missing $id/$ownerId, creator_id parsing, empty/truncated CBOR, user-property preservation. - json_conversion.rs: minimal document emits only id/owner keys, null creator/id handling, empty-object default-document, base58 identifier emission. - is_equal_ignoring_timestamps: nonexistent-field ignore list, empty-vec ignore list, None/Some revision comparison, self-equality. - get_raw_for_document_type: None-valued system fields return None, owner_id override only affects $ownerId path, unknown document_type_name surfaces DocumentTypeNotFound. - accessors/mod.rs: Document enum getter/setter delegation, bump_revision saturation, get() by path. Previously had zero tests. - document/mod.rs: increment_revision overflow, From<DocumentV0>, Display version prefix, get_raw_for_document_type dispatch. Documents a V0 consensus-frozen quirk: serialize_v2 / from_bytes_v2 intentionally skip the creator_id byte for non-transferable / TradeMode::None document types (contactRequest in dashpay), so a creator_id set on such a document is lost on round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add 60 tests across nine gRPC query handlers in rs-drive-abci/src/query/ that target real user-facing error paths — malformed identifiers, edge-case list sizes, mixed valid/invalid identifier lists, contract-info branch boundaries, start_at info variants, address parsing, chunk alignment, and mixed proof/non-proof response paths. - address_funds/addresses_infos: 7 tests (P2SH roundtrip, empty bytes, first-invalid short-circuit, mixed known/unknown proofs) - data_contract_based_queries/data_contract: 6 tests (found+serialize, found+proof, absence proof, various invalid id shapes) - shielded/encrypted_notes: 9 tests (chunk alignment, boundary at chunk size, count=max, prove path) - system/current_quorums_info: 5 tests populating validator sets via TestQuorumInfo (descending sort, banned validator, proposer propagation, multi-set) - token_queries/identities_token_infos: 7 tests (mixed frozen/unfrozen, list with one invalid id, empty list, max+proof) - token_queries/token_direct_purchase_prices: 5 tests (empty+proof, variable-price tier integrity, mixed known/unknown proof) - token_queries/token_perpetual_distribution_last_claim: 6 tests (u16::MAX boundary for token position, nonexistent position, identity-id-with- contract-info ordering) - token_queries/token_pre_programmed_distributions: 9 tests (start_at recipient included true/false/none, prove+start_at, far-future start) - voting/contested_resource_identity_votes: 6 tests (invalid/valid start_at poll identifier, offset+descending, offset at u16::MAX boundary)
Add 72 tests across 22 submodules of execution/platform_events/ covering error branches, short-circuits, and pure-constructor helpers: - masternode identity helpers (owner/voter/operator key constructors, identity factories, identifier derivation) - block_start/clear_drive_block_cache: idempotency and lock recovery - block_end/update_state_cache: quorum rotation + genesis clearing - block_end/update_checkpoints: disabled/misconfigured short-circuits - block_processing_end_events: shielded-pool pruning/anchor wrappers - core_based_updates/update_core_info: same-height short-circuit - core_instant_send_lock: invalid-signature-format returns Ok(false) - epoch/get_genesis_time: genesis-height echo vs drive lookup - epoch/gather_epoch_info: genesis-path, no-previous-block default - fee_pool_outwards_distribution/fetch_reward_shares_list: empty cases - initialization/initial_core_height: fork-inactive, height-not-locked, genesis-time-in-future, happy path, equality boundary - voting/clean_up_after_vote_polls_end: empty-input no-op branches - voting/remove_votes_for_removed_masternodes: no-diff no-op - voting/run_dao_platform_events: empty-platform success path Tests only; no production code modified.
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 14 minutes and 28 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 (9)
📝 WalkthroughWalkthroughThis PR adds extensive unit test coverage across the rs-dpp, rs-drive-abci, and rs-drive packages, covering document operations, platform event handlers, query implementations, and state transition verification. All changes are confined to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 11
🧹 Nitpick comments (10)
packages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rs (1)
92-203: LGTM — focused coverage of the three early-return paths.The three tests cleanly cover the
should_checkpoint → Noneshort-circuit via distinct triggers (disable_checkpoints,should_checkpoint = Nonemethod version, andfrequency_seconds = 0), and the sharedmake_contexthelper keeps the setup DRY.Optional: the doc comments for the first and third tests claim the filesystem is not touched, but the assertions only check the boolean return. If you want to make that invariant enforceable, consider asserting
!platform.config.db_path.join("checkpoints").exists()after the call. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rs` around lines 92 - 203, Add an assertion to ensure no checkpoint directory was created in the tests that claim the filesystem is untouched: after calling update_checkpoints_v0 in v0_returns_false_when_checkpoints_disabled_in_test_config and v0_returns_false_when_frequency_is_zero, assert that platform.config.db_path.join("checkpoints").exists() is false (or !exists()) to enforce the invariant; keep using the existing TestPlatformBuilder/platform instance and make_context helper so the check uses the same platform used to call update_checkpoints_v0.packages/rs-drive-abci/src/execution/platform_events/core_instant_send_lock/verify_recent_signature_locally/v0/mod.rs (1)
162-162: Nit: redundant import.
InstantLockis already in scope throughuse super::*;(re-exported from line 5). The explicituse dpp::dashcore::InstantLock;here is redundant and can be dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/core_instant_send_lock/verify_recent_signature_locally/v0/mod.rs` at line 162, The explicit import use dpp::dashcore::InstantLock; is redundant because InstantLock is already brought into scope via use super::*;—remove the redundant use dpp::dashcore::InstantLock; line from the module (around the top of mod.rs) so only the re-exported symbol from super is used; no other changes required.packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_operator_identity/v0/mod.rs (1)
44-137: LGTM — covers all four combinations ofoperator_payout_address/platform_node_idand the pro_tx_hash uniqueness invariant. Good cardinality matrix.Optional nit:
make_masternodeis duplicated across several sibling test modules in this directory (create_owner_identity, create_voter_identity, get_operator_identifier). Consider extracting to a shared#[cfg(test)]test-helpers module to reduce drift risk ifMasternodeListItem/DMNStatefields evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_operator_identity/v0/mod.rs` around lines 44 - 137, Tests duplicate the make_masternode helper across multiple modules; extract it into a single shared test helper module (e.g., a new #[cfg(test)] mod test_helpers) and have the test modules (create_operator_identity, create_owner_identity, create_voter_identity, get_operator_identifier) import and call that helper to avoid drift. Move the make_masternode function and any dependent types/fixtures it needs (MasternodeListItem, DMNState construction values) into the shared test_helpers module, update tests to call test_helpers::make_masternode (or use pub(crate) use) and remove the local copies so all tests reference the single canonical implementation.packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_voter_identity/v0/mod.rs (1)
43-150: LGTM — the three tests (id equalscreate_voter_identifier, MN-item flavor agrees with direct constructor, distinct voting keys yield distinct ids) cover the meaningful invariants. Worth noting this intentionally does not pin the pro_tx-vs-voting-key order ofcreate_voter_identifier's arguments — a swap would still passv0_distinct_voting_keys_yield_distinct_identities. Consider an optional assertion with an independent expected-hash, but not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_voter_identity/v0/mod.rs` around lines 43 - 150, Tests validate voter identity invariants but don't assert the exact expected identifier bytes, which leaves the argument order unpinned; add an optional assertion computing a known expected Identifier (via Identifier::create_voter_identifier with the chosen pro_tx and voting_key) and compare it to the identity.id() inside the existing test (e.g., in v0_voter_identity_id_matches_voter_identifier or add a new small test), referencing Platform::create_voter_identity_v0 and Identifier::create_voter_identifier to locate the code; keep it optional/non-blocking and only add a single deterministic equality check with a hardcoded expected hash if you can derive it independently.packages/rs-drive-abci/src/execution/platform_events/block_processing_end_events/record_shielded_pool_anchor/v0/mod.rs (1)
34-94: Assert the no-op state, not only successful execution.These tests document no-op/idempotent behavior, but they only assert
Ok(()). An implementation that always records an anchor successfully would still pass. Consider also asserting that the most-recent anchor / anchor mapping is absent or unchanged before and after these calls. Based on learnings, anchors are stored asanchor_bytes → block_heightandSHIELDED_MOST_RECENT_ANCHOR_KEYis used for O(1) latest reads; do not use ascending/descending range queries for this check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/block_processing_end_events/record_shielded_pool_anchor/v0/mod.rs` around lines 34 - 94, Tests only assert success but not that no anchor was written; update the tests that call record_shielded_pool_anchor_if_changed_v0 (v0_ok_on_fresh_state, v0_idempotent_across_heights_when_tree_unchanged, v0_accepts_height_zero, v0_accepts_max_height) to also assert the shielded anchor state before and after the call: read the SHIELDED_MOST_RECENT_ANCHOR_KEY and the anchor_bytes → block_height mapping and verify they are absent on a genesis fresh state and remain unchanged after each call (use the same transaction/read methods the test harness exposes to fetch the mapping and the O(1 SHIELDED_MOST_RECENT_ANCHOR_KEY lookup rather than range queries). Ensure assertions reference the exact key constant SHIELDED_MOST_RECENT_ANCHOR_KEY and the mapping lookup used by the Drive implementation.packages/rs-drive-abci/src/query/address_funds/addresses_infos/v0/mod.rs (2)
396-418: Assert metadata for absence and mixed proof responses.These tests cover proof-mode edge cases, but they only verify the
Proofvariant. Addingmetadata.is_some()keeps the proof response contract covered for absence and mixed known/unknown queries too.Proposed test assertions
assert!(matches!( response.result, Some(get_addresses_infos_response_v0::Result::Proof(_)) )); + assert!(response.metadata.is_some());- assert!(matches!( - result.data.unwrap().result, + let response = result.data.unwrap(); + assert!(matches!( + response.result, Some(get_addresses_infos_response_v0::Result::Proof(_)) )); + assert!(response.metadata.is_some());Also applies to: 493-514
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/address_funds/addresses_infos/v0/mod.rs` around lines 396 - 418, The proof-mode tests only assert the response.result is the Proof variant but don't assert the associated metadata; update the tests (e.g., test_addresses_infos_proof_with_unknown_address and the similar mixed-known/unknown proof test) to, after unwrapping response from result.data, also assert that response.metadata.is_some() whenever response.result matches get_addresses_infos_response_v0::Result::Proof(_), ensuring the proof contract is validated for absence and mixed queries.
421-439: Rename or strengthen the short-circuit test.Line 427 puts the malformed address last, so this currently proves “mixed list is rejected,” not that validation stops at the first malformed entry. Either rename the test/comment to match the assertion, or add a distinguishable later malformed address and assert the first error is the one returned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/address_funds/addresses_infos/v0/mod.rs` around lines 421 - 439, The test test_addresses_infos_first_invalid_short_circuits_list currently places the malformed address last so it only proves "mixed list is rejected"; either rename it to reflect that or modify it so validation truly short-circuits: put the malformed address as the first entry in the request (e.g. swap the order so the vec starts with vec![0xFF,0xFF] then ADDR1.to_bytes()), or add a second, distinguishable malformed address later and assert that result.errors[0] (matching QueryError::InvalidArgument(msg)) corresponds to the first malformed entry by checking the message text; update the assertion accordingly to reference result.errors and QueryError::InvalidArgument.packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_core_info/v0/mod.rs (1)
129-163: Consider clarifying or removing redundant test.This test duplicates the setup and assertion from the first test but runs it twice in a loop. The added value is unclear:
- If testing idempotency: The short-circuit path is stateless, so calling it twice doesn't verify anything new beyond the first test.
- If testing the dependency on
is_init_chain=false: The test doesn't verify behavior whenis_init_chain=true, so the name is misleading.♻️ Suggested refactor options
Option 1: Remove this test entirely if it doesn't add coverage beyond the first test.
Option 2: Rename and simplify to focus on idempotency (though the value is debatable):
#[test] - fn v0_short_circuit_is_tied_to_is_init_chain_false() { - // Re-confirm that both conditions together are required by running - // the short-circuit case twice; idempotency guards against flakiness. + fn v0_short_circuit_is_idempotent() { + // Confirms the early-return path is idempotent across multiple calls. let platform_version = PlatformVersion::latest(); ... for _ in 0..2 { platform .update_core_info_v0( None, &mut block_platform_state, same_core_height, - false, + /*is_init_chain=*/ false, &block_info, &transaction, platform_version, ) .expect("idempotent short-circuit"); }Option 3: If you want to verify the behavior when
is_init_chain=true, add a separate test with proper RPC mocking (if feasible) and document the expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_core_info/v0/mod.rs` around lines 129 - 163, The test function v0_short_circuit_is_tied_to_is_init_chain_false is redundant/misnamed: either delete it or make intent explicit—rename to test_v0_short_circuit_idempotent and simplify to a single invocation if you only want idempotency, or replace it with a new test that sets is_init_chain=true and asserts the non-short-circuit behavior (mock RPC as needed) to validate the name; update or remove references to update_core_info_v0 and TestPlatformBuilder accordingly so the suite only contains the meaningful test.packages/rs-drive-abci/src/execution/platform_events/initialization/initial_core_height/v0/mod.rs (1)
151-153: Consider constraining RPC mock arguments for test specificity.The current
returning(|_| ...)matchers accept any argument values. While the implementation hardcodesget_fork_info("mn_rr"), adding Mockall argument predicates would strengthen tests forget_block_time_from_height(), which accepts variable heights (line 66). This ensures the implementation queries the correct block height. If preferred, apply.with(eq(height))constraints for the block time expectations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/initialization/initial_core_height/v0/mod.rs` around lines 151 - 153, The mock expectations are too permissive: replace the blanket returning(|_| ...) on MockCoreRPCLike with argument-constrained expectations so the test asserts the exactly-queried values; for example, change the expect for get_fork_info to use .with(eq("mn_rr")) (or the exact chain/id used) and add .with(eq(height)) on the expect for get_block_time_from_height (or the specific height variable used in the test) so the mock only matches when the implementation calls those RPCs with the expected arguments (adjust return values accordingly).packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs (1)
2782-4535: Large amount of boilerplate duplication across empty-proof error tests.Roughly a dozen new tests (e.g.,
verify_address_funds_transfer_empty_proof_returns_error,verify_address_credit_withdrawal_empty_proof_returns_error,verify_shield_empty_proof_returns_error,verify_identity_credit_withdrawal_v1_empty_proof_returns_error,verify_identity_create_from_addresses_empty_proof_returns_error,verify_identity_top_up_from_addresses_empty_proof_returns_error, …) follow the exact same shape: construct a transition, plug in a|_id| Ok(None)provider, invoke with&[], then assertmatches!(result, Err(Error::Proof(_)) | Err(Error::GroveDB(_))). Consider extracting a small helper (or a table-driven test with aVec<StateTransition>) to remove the repeated provider/assertion scaffolding — it would make it much easier to add/maintain future variants and keep the intent of each test visible. Same observation applies to the parallel family of_unknown_contract_returns_errortoken/document tests (lines ~3364-3916), which could share a single "assert UnknownContract for this batched transition" helper.Non-blocking — tests are correct as written.
🤖 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 2782 - 4535, Many tests duplicate the same scaffolding (creating a known_contracts_provider_fn returning Ok(None), calling Drive::verify_state_transition_was_executed_with_proof with an empty or garbage proof, and asserting Err(Error::Proof(_)) | Err(Error::GroveDB(_)) or UnknownContract); extract small reusable helpers to reduce boilerplate: add a helper like assert_empty_proof_errors(st: &StateTransition, platform_version: PlatformVersion) that builds the provider |_id| Ok(None), calls Drive::verify_state_transition_was_executed_with_proof(&st, &BlockInfo::default(), &[], known_contracts_provider_fn, platform_version) and performs the matches! assertion, and a second helper like assert_unknown_contract_for_batched_transition(st: &StateTransition, platform_version: PlatformVersion) that calls the same provider, invokes verify_state_transition_was_executed_with_proof, and asserts Error::Proof(ProofError::UnknownContract(_)); then replace repeated blocks (e.g., verify_address_funds_transfer_empty_proof_returns_error, verify_shield_empty_proof_returns_error, verify_identity_credit_withdrawal_v1_empty_proof_returns_error, and the batch token/document *_unknown_contract_returns_error tests) with calls to these helpers to keep each test focused on constructing the specific StateTransition (symbols: Drive::verify_state_transition_was_executed_with_proof, StateTransition::AddressFundsTransfer, StateTransition::Shield, StateTransition::IdentityCreditWithdrawal, BatchTransition::V0/V1, etc.).
🤖 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/serialize.rs`:
- Around line 2561-2595: The test
from_bytes_v0_falls_back_to_v1_on_decoding_error currently allows a fallback
decode to fail (and accepts any non-UnknownVersionMismatch error), so change it
to require the fallback to succeed: call DocumentV0::from_bytes with v1_bytes
and assert that the result is Ok, then directly assert_eq!(recovered, doc_v0)
(or unwrap and compare); remove the branch that permits Err and the
ProtocolError::UnknownVersionMismatch check so the test fails if v1 fallback
does not successfully recover the original doc_v0.
- Around line 2527-2551: The test
serialize_specific_version_rejects_v2_for_v0_contract can pass vacuously because
it conditionally skips assertions with if matches!, so make the V0 precondition
explicit and always run the rejection assertion: replace the if matches! guard
with an assert!(matches!(contract, DataContract::V0(_)), "expected V0 contract
from dashpay_contract_and_type(...)") to fail fast if the fixture changes, then
call doc_v0.serialize_specific_version(document_type, &contract, 2) and assert
it returns an Err that matches ProtocolError::NotSupported (using expect_err or
matching the Err) so the test always verifies the rejection even if fixtures
change; reference the test name, dashpay_contract_and_type,
PlatformVersion::latest, serialize_specific_version, and
ProtocolError::NotSupported when locating and updating the code.
In
`@packages/rs-drive-abci/src/execution/platform_events/block_processing_end_events/prune_shielded_pool_anchors/v0/mod.rs`:
- Around line 51-67: The tests for prune_shielded_pool_anchors_v0 use hardcoded
heights (1 and 1_048_576) which can miss exercising branches; change them to
derive heights from the versioned event constants instead: obtain the
shielded_anchor_pruning_interval and retention/retention_window from
PlatformVersion::latest().drive.event_constants (or the appropriate event
constants accessor) and compute a guaranteed non-boundary height (e.g., interval
+ 1 or any value not divisible by interval) for the noop test and a boundary
height that also exceeds retention (e.g., a multiple of interval greater than
retention) for the pruning/delegation test so the intended branches are actually
reached when constants change, keeping the calls to
prune_shielded_pool_anchors_v0 and transaction setup unchanged.
In
`@packages/rs-drive-abci/src/execution/platform_events/block_start/clear_drive_block_cache/v0/mod.rs`:
- Around line 52-68: The test should assert the actual blocked/unblocked state
instead of only re-calling clear_drive_block_cache_v0; modify
v0_leaves_cache_usable_after_call to (1) explicitly block the global cache via
the blocking helper (e.g., call platform.drive.cache.block_global_cache() or
whatever API blocks it), assert that a non-blocking read attempt
(protocol_versions_counter.try_read()) fails (is None), (2) call
platform.clear_drive_block_cache_v0(), and then assert that
protocol_versions_counter.try_read() returns Some (unblocked), and optionally
call platform.drive.cache.unblock_global_cache() / cleanup; reference
clear_drive_block_cache_v0, protocol_versions_counter, and
unblock_global_cache()/block_global_cache() to locate the changes.
In
`@packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_core_info/v0/mod.rs`:
- Around line 119-127: The doc comment above the test is misleading: it claims
to exercise the is_init_chain=true path but the test body passes is_init_chain =
false (see the test function where is_init_chain is set around the block
starting at line ~156). Either update the docstring to describe that this test
verifies the short-circuit/idempotent same-height early-return behavior when
is_init_chain=false, or change the test to actually set is_init_chain = true and
add the necessary RPC mocking (or document why RPC is not mocked) so the comment
and the test implementation match; reference the test function and the
is_init_chain assignment to make the fix.
In
`@packages/rs-drive-abci/src/execution/platform_events/epoch/gather_epoch_info/v0/mod.rs`:
- Around line 119-129: The test currently only checks the error message text
from platform.platform.gather_epoch_info_v0 but should assert the propagated
error variant first; update the test that calls gather_epoch_info_v0 to
expect_err and then pattern-match or downcast the returned error to the
DriveIncoherence variant (e.g., match on the error enum or use a helper like
is_drive_incoherence/downcast_ref) to ensure the function propagates
DriveIncoherence, and only after that assert the error message contains
"genesis" — reference the gather_epoch_info_v0 call and the DriveIncoherence
variant when making the change.
In
`@packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/fetch_reward_shares_list_for_masternode/v0/mod.rs`:
- Around line 109-130: The test
v0_different_owners_all_return_empty_on_fresh_state currently cannot prove owner
bytes affect the query because the genesis state is empty; either relax the doc
comment or modify the test to insert a reward-share record and then assert
fetch_reward_shares_list_for_masternode_v0(&matching_owner, ...) returns that
record while calls with non-matching owners return empty; to implement the
stronger test, use TestPlatformBuilder (or platform.drive.grove transaction) to
create/insert a reward-share for a specific owner before calling
fetch_reward_shares_list_for_masternode_v0 and then verify only the matching
owner returns the inserted document.
In
`@packages/rs-drive-abci/src/execution/platform_events/initialization/initial_core_height/v0/mod.rs`:
- Line 317: Add a unit test that asserts a requested core height before the MN
RR fork is rejected: in the existing test module for initial_core_height (v0),
create a test that sets active_fork(10), chain_lock_at(50), and requested =
Some(9) and then calls the function under test (the initializer used in this
module, e.g., initialize_core_height or the test helper used elsewhere in this
file) and asserts it returns an error (or panics) to enforce the documented
invariant that "requested core height is before mn_rr fork" must fail; reference
the existing helpers active_fork and chain_lock_at and the requested parameter
to locate where to add the test.
In `@packages/rs-drive-abci/src/query/shielded/encrypted_notes/v0/mod.rs`:
- Around line 167-173: The test hardcodes an unaligned start_index (5); instead
compute it from the configured chunk size so the test remains valid if
max_encrypted_notes_per_query changes: call max_encrypted_notes_per_query() (or
the constant/method that provides the chunk size used by
GetShieldedEncryptedNotesRequestV0), assert the chunk size > 1, then set
start_index = chunk_size - 1 (which is guaranteed to be unaligned), and use that
in the GetShieldedEncryptedNotesRequestV0 construction instead of the literal 5.
In
`@packages/rs-drive-abci/src/query/voting/contested_resource_identity_votes/v0/mod.rs`:
- Around line 514-521: The test currently asserts any Votes(_) payload; tighten
it to assert the inner votes collection is empty: after asserting
result.errors.is_empty(), pattern-match result.data to
Some(GetContestedResourceIdentityVotesResponseV0 { result:
Some(get_contested_resource_identity_votes_response_v0::Result::Votes(votes_msg)),
metadata: Some(_) }) and then assert that votes_msg.votes (or the actual field
name on the Votes message) is empty (e.g., assert!(votes_msg.votes.is_empty())).
This ensures GetContestedResourceIdentityVotesResponseV0 and
get_contested_resource_identity_votes_response_v0::Result::Votes carry an empty
result set.
- Around line 441-456: The test currently only asserts absence of an "offset out
of bounds" error after calling query_contested_resource_identity_votes_v0;
tighten it to assert full acceptance by requiring result.errors to be empty and
that a valid response exists (e.g., result.data or result.response is Some/Ok
depending on the result shape) instead of just checking for one error string.
Locate the call to platform.query_contested_resource_identity_votes_v0 and the
variable result in the test and replace the has_offset_err/assert block with
assertions that result.errors.is_empty() and that the returned payload field on
result (data/response) is present and valid.
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rs`:
- Around line 92-203: Add an assertion to ensure no checkpoint directory was
created in the tests that claim the filesystem is untouched: after calling
update_checkpoints_v0 in
v0_returns_false_when_checkpoints_disabled_in_test_config and
v0_returns_false_when_frequency_is_zero, assert that
platform.config.db_path.join("checkpoints").exists() is false (or !exists()) to
enforce the invariant; keep using the existing TestPlatformBuilder/platform
instance and make_context helper so the check uses the same platform used to
call update_checkpoints_v0.
In
`@packages/rs-drive-abci/src/execution/platform_events/block_processing_end_events/record_shielded_pool_anchor/v0/mod.rs`:
- Around line 34-94: Tests only assert success but not that no anchor was
written; update the tests that call record_shielded_pool_anchor_if_changed_v0
(v0_ok_on_fresh_state, v0_idempotent_across_heights_when_tree_unchanged,
v0_accepts_height_zero, v0_accepts_max_height) to also assert the shielded
anchor state before and after the call: read the SHIELDED_MOST_RECENT_ANCHOR_KEY
and the anchor_bytes → block_height mapping and verify they are absent on a
genesis fresh state and remain unchanged after each call (use the same
transaction/read methods the test harness exposes to fetch the mapping and the
O(1 SHIELDED_MOST_RECENT_ANCHOR_KEY lookup rather than range queries). Ensure
assertions reference the exact key constant SHIELDED_MOST_RECENT_ANCHOR_KEY and
the mapping lookup used by the Drive implementation.
In
`@packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_core_info/v0/mod.rs`:
- Around line 129-163: The test function
v0_short_circuit_is_tied_to_is_init_chain_false is redundant/misnamed: either
delete it or make intent explicit—rename to test_v0_short_circuit_idempotent and
simplify to a single invocation if you only want idempotency, or replace it with
a new test that sets is_init_chain=true and asserts the non-short-circuit
behavior (mock RPC as needed) to validate the name; update or remove references
to update_core_info_v0 and TestPlatformBuilder accordingly so the suite only
contains the meaningful test.
In
`@packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_operator_identity/v0/mod.rs`:
- Around line 44-137: Tests duplicate the make_masternode helper across multiple
modules; extract it into a single shared test helper module (e.g., a new
#[cfg(test)] mod test_helpers) and have the test modules
(create_operator_identity, create_owner_identity, create_voter_identity,
get_operator_identifier) import and call that helper to avoid drift. Move the
make_masternode function and any dependent types/fixtures it needs
(MasternodeListItem, DMNState construction values) into the shared test_helpers
module, update tests to call test_helpers::make_masternode (or use pub(crate)
use) and remove the local copies so all tests reference the single canonical
implementation.
In
`@packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_voter_identity/v0/mod.rs`:
- Around line 43-150: Tests validate voter identity invariants but don't assert
the exact expected identifier bytes, which leaves the argument order unpinned;
add an optional assertion computing a known expected Identifier (via
Identifier::create_voter_identifier with the chosen pro_tx and voting_key) and
compare it to the identity.id() inside the existing test (e.g., in
v0_voter_identity_id_matches_voter_identifier or add a new small test),
referencing Platform::create_voter_identity_v0 and
Identifier::create_voter_identifier to locate the code; keep it
optional/non-blocking and only add a single deterministic equality check with a
hardcoded expected hash if you can derive it independently.
In
`@packages/rs-drive-abci/src/execution/platform_events/core_instant_send_lock/verify_recent_signature_locally/v0/mod.rs`:
- Line 162: The explicit import use dpp::dashcore::InstantLock; is redundant
because InstantLock is already brought into scope via use super::*;—remove the
redundant use dpp::dashcore::InstantLock; line from the module (around the top
of mod.rs) so only the re-exported symbol from super is used; no other changes
required.
In
`@packages/rs-drive-abci/src/execution/platform_events/initialization/initial_core_height/v0/mod.rs`:
- Around line 151-153: The mock expectations are too permissive: replace the
blanket returning(|_| ...) on MockCoreRPCLike with argument-constrained
expectations so the test asserts the exactly-queried values; for example, change
the expect for get_fork_info to use .with(eq("mn_rr")) (or the exact chain/id
used) and add .with(eq(height)) on the expect for get_block_time_from_height (or
the specific height variable used in the test) so the mock only matches when the
implementation calls those RPCs with the expected arguments (adjust return
values accordingly).
In `@packages/rs-drive-abci/src/query/address_funds/addresses_infos/v0/mod.rs`:
- Around line 396-418: The proof-mode tests only assert the response.result is
the Proof variant but don't assert the associated metadata; update the tests
(e.g., test_addresses_infos_proof_with_unknown_address and the similar
mixed-known/unknown proof test) to, after unwrapping response from result.data,
also assert that response.metadata.is_some() whenever response.result matches
get_addresses_infos_response_v0::Result::Proof(_), ensuring the proof contract
is validated for absence and mixed queries.
- Around line 421-439: The test
test_addresses_infos_first_invalid_short_circuits_list currently places the
malformed address last so it only proves "mixed list is rejected"; either rename
it to reflect that or modify it so validation truly short-circuits: put the
malformed address as the first entry in the request (e.g. swap the order so the
vec starts with vec![0xFF,0xFF] then ADDR1.to_bytes()), or add a second,
distinguishable malformed address later and assert that result.errors[0]
(matching QueryError::InvalidArgument(msg)) corresponds to the first malformed
entry by checking the message text; update the assertion accordingly to
reference result.errors and QueryError::InvalidArgument.
In
`@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- Around line 2782-4535: Many tests duplicate the same scaffolding (creating a
known_contracts_provider_fn returning Ok(None), calling
Drive::verify_state_transition_was_executed_with_proof with an empty or garbage
proof, and asserting Err(Error::Proof(_)) | Err(Error::GroveDB(_)) or
UnknownContract); extract small reusable helpers to reduce boilerplate: add a
helper like assert_empty_proof_errors(st: &StateTransition, platform_version:
PlatformVersion) that builds the provider |_id| Ok(None), calls
Drive::verify_state_transition_was_executed_with_proof(&st,
&BlockInfo::default(), &[], known_contracts_provider_fn, platform_version) and
performs the matches! assertion, and a second helper like
assert_unknown_contract_for_batched_transition(st: &StateTransition,
platform_version: PlatformVersion) that calls the same provider, invokes
verify_state_transition_was_executed_with_proof, and asserts
Error::Proof(ProofError::UnknownContract(_)); then replace repeated blocks
(e.g., verify_address_funds_transfer_empty_proof_returns_error,
verify_shield_empty_proof_returns_error,
verify_identity_credit_withdrawal_v1_empty_proof_returns_error, and the batch
token/document *_unknown_contract_returns_error tests) with calls to these
helpers to keep each test focused on constructing the specific StateTransition
(symbols: Drive::verify_state_transition_was_executed_with_proof,
StateTransition::AddressFundsTransfer, StateTransition::Shield,
StateTransition::IdentityCreditWithdrawal, BatchTransition::V0/V1, etc.).
🪄 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: 07c9daf0-a554-44dc-8bcd-2e5a492cdff3
📒 Files selected for processing (40)
packages/rs-dpp/src/document/accessors/mod.rspackages/rs-dpp/src/document/document_methods/get_raw_for_document_type/v0/mod.rspackages/rs-dpp/src/document/document_methods/is_equal_ignoring_timestamps/v0/mod.rspackages/rs-dpp/src/document/mod.rspackages/rs-dpp/src/document/v0/cbor_conversion.rspackages/rs-dpp/src/document/v0/json_conversion.rspackages/rs-dpp/src/document/v0/platform_value_conversion.rspackages/rs-dpp/src/document/v0/serialize.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_processing_end_events/prune_shielded_pool_anchors/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_processing_end_events/record_shielded_pool_anchor/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_start/clear_drive_block_cache/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_core_info/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_operator_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_owner_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_voter_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/get_operator_identifier/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/get_operator_identity_keys/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/get_owner_identity_owner_key/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/get_owner_identity_withdrawal_key/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/get_voter_identity_key/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_instant_send_lock/verify_recent_signature_locally/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/epoch/gather_epoch_info/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/epoch/get_genesis_time/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/fetch_reward_shares_list_for_masternode/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/initialization/initial_core_height/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/voting/clean_up_after_vote_polls_end/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/voting/remove_votes_for_removed_masternodes/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/voting/run_dao_platform_events/v0/mod.rspackages/rs-drive-abci/src/query/address_funds/addresses_infos/v0/mod.rspackages/rs-drive-abci/src/query/data_contract_based_queries/data_contract/v0/mod.rspackages/rs-drive-abci/src/query/shielded/encrypted_notes/v0/mod.rspackages/rs-drive-abci/src/query/system/current_quorums_info/v0/mod.rspackages/rs-drive-abci/src/query/token_queries/identities_token_infos/v0/mod.rspackages/rs-drive-abci/src/query/token_queries/token_direct_purchase_prices/v0/mod.rspackages/rs-drive-abci/src/query/token_queries/token_perpetual_distribution_last_claim/v0/mod.rspackages/rs-drive-abci/src/query/token_queries/token_pre_programmed_distributions/v0/mod.rspackages/rs-drive-abci/src/query/voting/contested_resource_identity_votes/v0/mod.rspackages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
| ), | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate initialization-height/fork error variants and existing assertions.
rg -n -C3 'Initialization.*(Fork|Height)|before mn_rr|mn_rr fork|initial.*fork' --glob '*.rs'Repository: dashpay/platform
Length of output: 21302
🏁 Script executed:
cat -n packages/rs-drive-abci/src/execution/platform_events/initialization/initial_core_height/v0/mod.rs | head -150Repository: dashpay/platform
Length of output: 6081
🏁 Script executed:
# Check the full line count to understand file size
wc -l packages/rs-drive-abci/src/execution/platform_events/initialization/initial_core_height/v0/mod.rsRepository: dashpay/platform
Length of output: 161
🏁 Script executed:
# Search for all test functions in the file
rg -n '#\[test\]' packages/rs-drive-abci/src/execution/platform_events/initialization/initial_core_height/v0/mod.rsRepository: dashpay/platform
Length of output: 155
🏁 Script executed:
# Search for error variants with "before" or "fork" in error definitions
rg -n 'InitializationHeight|InitializationFork' packages/rs-drive-abci/src/error/execution.rsRepository: dashpay/platform
Length of output: 144
🏁 Script executed:
# Read the test section to see all tests
sed -n '93,317p' packages/rs-drive-abci/src/execution/platform_events/initialization/initial_core_height/v0/mod.rsRepository: dashpay/platform
Length of output: 7858
Add a test for requested < mn_rr_fork_height to expose the documented invariant gap.
The function documentation (lines 24–26) states that "requested core height is before mn_rr fork" must fail, but the implementation accepts any requested height so long as it is chain-locked. No test currently covers this boundary. Add a test with active_fork(10), chain_lock_at(50), and requested = Some(9) to verify that the documented constraint is enforced. This test will likely fail, revealing that either the docs or the implementation must change—and determining which is out of scope for this PR's test additions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-drive-abci/src/execution/platform_events/initialization/initial_core_height/v0/mod.rs`
at line 317, Add a unit test that asserts a requested core height before the MN
RR fork is rejected: in the existing test module for initial_core_height (v0),
create a test that sets active_fork(10), chain_lock_at(50), and requested =
Some(9) and then calls the function under test (the initializer used in this
module, e.g., initialize_core_height or the test helper used elsewhere in this
file) and asserts it returns an error (or panics) to enforce the documented
invariant that "requested core height is before mn_rr fork" must fail; reference
the existing helpers active_fork and chain_lock_at and the requested parameter
to locate where to add the test.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3528 +/- ##
============================================
+ Coverage 88.00% 88.18% +0.18%
============================================
Files 2474 2474
Lines 294440 298763 +4323
============================================
+ Hits 259132 263477 +4345
+ Misses 35308 35286 -22
🚀 New features to boost your workflow:
|
- serialize.rs V0 contract test: assert V0 precondition explicitly so the NotSupported branch is always exercised (no vacuous pass). - serialize.rs v0->v1 fallback: require fallback to recover the document rather than accept any non-version-mismatch error. - prune_shielded_pool_anchors tests: derive pruning-boundary heights from event_constants so tests fail if the targeted branch becomes unreachable under version drift. - clear_drive_block_cache test: directly verify unblock by blocking the global cache first, asserting GlobalCacheIsBlocked is returned, then asserting Ok after clear_drive_block_cache_v0. - update_core_info docstring: rewrite to describe actual behavior (idempotent short-circuit with is_init_chain=false). - gather_epoch_info error assertion: match ExecutionError::DriveIncoherence variant instead of substring-matching the display text. - fetch_reward_shares_list_for_masternode: honest doc comment describing the empty-list branch across distinct owners. - initial_core_height: pin current behavior for requested < fork_height (documents the doc-vs-impl invariant gap). - encrypted_notes test: derive unaligned start_index from chunk size so test is robust to version constant changes. - contested_resource_identity_votes: require empty errors + populated Votes response on acceptance-boundary tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for 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 the highest-value remaining user-facing targets:
What was done
Added 222 new unit tests across ~55 files. Tests only; no production code modified.
rs-drive-abci/src/query — 60 tests
system/current_quorums_info(was 41%): 5 tests on validator set mapping, core-height ordering, is_banned propagationdata_contract_based_queries/data_contract(was 59%): 6 tests on missing, malformed id, large contract, proof vs non-proofshielded/encrypted_notes(was 75%): 9 tests on chunk alignment, empty pool, start/limit boundariesvoting/contested_resource_identity_votes(was 80%): 6 tests on missing identity, empty votes, mixed valid/invalidtoken_queries/identities_token_infos(82%): 7 tests on frozen tokens, missing positions, contract lookuptoken_queries/token_perpetual_distribution_last_claim(82%): 6 tests on nonexistent token, u16 boundariestoken_queries/token_direct_purchase_prices(82%): 5 teststoken_queries/token_pre_programmed_distributions(85%): 9 tests on u16::MAX limits, zero limitsaddress_funds/addresses_infos(83%): 7 tests on P2SH/P2PKH, mixed types, invalid bytesrs-drive-abci/src/execution/platform_events — 72 tests
Across 22 submodules:
block_end/{update_checkpoints, update_state_cache}— disabled config, version=None, quorum rotationblock_processing_end_events/{prune, record}_shielded_pool_anchors— intervals, retention, idempotencyblock_start/clear_drive_block_cache— fresh/idempotent/lock recoverycore_based_updates/update_core_info— same-height short-circuitcore_based_updates/update_masternode_identities/*— 26 tests on operator/owner/voter identity creation, key derivation, identifier determinismcore_instant_send_lock/verify_recent_signature_locally— invalid BLS sig → Ok(false) (not Err)epoch/{gather_epoch_info, get_genesis_time}— genesis, cache, DriveIncoherence propagationfee_pool_outwards_distribution/fetch_reward_shares_list_for_masternode— empty/multi/no-txinitialization/initial_core_height— fork None/inactive/above-chainlock, happy pathvoting/{clean_up_after_vote_polls_end, remove_votes_for_removed_masternodes, run_dao_platform_events}— no-op / idempotent / empty-diffrs-drive/src/verify/state_transition — 34 tests
Cover uncovered state transition variants in
verify_state_transition_was_executed_with_proof:InvalidTransition("shield from asset lock has no outpoint"), empty-proof via ChainAssetLockProofkeeps_history=truepath for DataContractCreate/Updaters-dpp/src/document/v0 — 56 tests
serialize.rs(14): v0/v1/v2 MissingRequiredKey paths, too-small-buffer, truncated-post-id, v0→v1 fallback, serialize_specific_version boundariescbor_conversion.rs(7): from_map without $id/$ownerId, from_cbor empty/truncated, round-tripjson_conversion.rs(7): to_json_with_identifiers_using_bytes, Identifier emission, null creator_id, property types, empty objectplatform_value_conversion.rs(9): all 5 methods, round-trips, non-map erroraccessors/mod.rs(8): all getter/setter delegations, bump_revision overflow, default get(path)mod.rs(4): increment_revision overflow → ProtocolError::Overflow, From, Displayis_equal_ignoring_timestamps(4),get_raw_for_document_type(3)V0 frozen quirk documented:
serialize_v2intentionally skips thecreator_idbyte when document type is non-transferable ANDtrade_mode()isNone(e.g. dashpaycontactRequest). Locked in withfrozen: V0 consensus behaviorcomment.How Has This Been Tested?
Breaking Changes
None — tests only.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit