test(drive-abci): improve platform_events test coverage#3270
Merged
QuantumExplorer merged 2 commits intoMar 15, 2026
Conversation
Add 38 new unit tests covering protocol_upgrade, block_end, and tokens modules to improve test coverage in the platform_events directory: - protocol_upgrade/check_for_desired_protocol_upgrade (v0 + v1): threshold boundary tests, no-vote/sufficient-vote/insufficient-vote cases, multiple-versions-passing error, block cache contribution, edge cases (zero hpmns, single hpmn, large hpmn count) - protocol_upgrade/upgrade_protocol_version (v0 + dispatcher): epoch change cache blocking, fee version insertion on empty map, non-epoch-change same version success, protocol version mismatch error, genesis epoch bypass, upgrade vote integration - protocol_upgrade/perform_events_on_first_block_of_protocol_change: same-version no-op, unknown version error dispatch - block_end/update_drive_cache (v0): block cache merge without epoch change, global cache clear+unblock on epoch change, non-clearing behavior without epoch change, contracts cache merge - block_end/validator_set_update + should_checkpoint: version dispatch error handling - tokens/validate_token_aggregated_balance (v0 + dispatcher): verification enabled/disabled paths, version dispatch error Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds comprehensive unit test coverage for multiple platform event handlers across block-end and protocol-upgrade modules, along with token validation. No production code or public API signatures are modified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/rs-drive-abci/src/execution/platform_events/tokens/validate_token_aggregated_balance/v0/mod.rs (1)
45-82: Add one imbalance case to exercise the actual error path.Both tests currently validate
is_ok()on a fresh genesis state. Consider adding a targeted imbalance fixture (or mockedcalculate_total_tokens_balance) to assertExecutionError::CorruptedTokensNotBalancedwhen verification is enabled, and success when disabled. This would harden branch confidence beyond “happy-path only” assertions.🤖 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/tokens/validate_token_aggregated_balance/v0/mod.rs` around lines 45 - 82, Add a new test that injects a deliberate token-balance mismatch and asserts the error path: create a platform (use TestPlatformBuilder), start a transaction, then either mutate the genesis state to create an aggregated balance imbalance or mock calculate_total_tokens_balance to return an inconsistent sum; call validate_token_aggregated_balance_v0(&transaction, platform_version) and assert it returns Err matching ExecutionError::CorruptedTokensNotBalanced when execution.verify_token_sum_trees is true, then repeat with a config where execution.verify_token_sum_trees = false and assert Ok(); reference the existing test names (test_validate_token_aggregated_balance_with_verification_enabled/disabled), the function validate_token_aggregated_balance_v0, and the error type ExecutionError::CorruptedTokensNotBalanced when implementing the assertions and setup.packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/mod.rs (1)
82-115: Minor: Test name and comment don't match actual behavior.The test is named
test_perform_events_when_version_method_is_noneand comments mention "When the perform_events_on_first_block_of_protocol_change method is None", but it actually usesPlatformVersion::latest()which hasSome(0). The test validates the no-op case whereprevious_protocol_version == current, which is valid, but the naming is misleading.Consider renaming to better reflect what's being tested:
✏️ Suggested rename
#[test] - fn test_perform_events_when_version_method_is_none() { - // When the perform_events_on_first_block_of_protocol_change method is None, - // the dispatcher should return Ok(()) without doing anything. - // We can test this indirectly: the latest platform version has Some(0), - // but we can verify the dispatch for a same-version transition (no-op case). + fn test_perform_events_same_version_transition_is_noop() { + // When previous_protocol_version equals current version, no transition + // events should fire, and the function should return Ok(()).🤖 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/protocol_upgrade/perform_events_on_first_block_of_protocol_change/mod.rs` around lines 82 - 115, Rename the test function test_perform_events_when_version_method_is_none to reflect that it validates the no-op transition when previous_protocol_version == current (e.g., test_perform_events_noop_same_version) and update the inline comments to state that PlatformVersion::latest() has Some(0) and that the test exercises the same-version no-op path; ensure you keep the call to platform.perform_events_on_first_block_of_protocol_change and its assertions unchanged.packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/upgrade_protocol_version/v0/mod.rs (1)
383-386: Make vote-threshold setup data-driven instead of using a magic number.Using
100as a fixed vote count can become brittle if validator-list size or threshold logic changes. Derive votes from the same state inputs used by the function under test.Proposed refactor
- // Insert enough votes for the current version - { - let mut counter = platform.drive.cache.protocol_versions_counter.write(); - counter.global_cache.insert(voted_version, 100); - } + let last_committed_state = platform.state.load(); + let votes = last_committed_state.hpmn_active_list_len() as u32; + { + let mut counter = platform.drive.cache.protocol_versions_counter.write(); + counter.global_cache.insert(voted_version, votes.max(1)); + } @@ - let last_committed_state = platform.state.load(); let mut block_platform_state = last_committed_state.as_ref().clone();Also applies to: 388-399
🤖 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/protocol_upgrade/upgrade_protocol_version/v0/mod.rs` around lines 383 - 386, The test setup currently inserts a magic vote count of 100 into platform.drive.cache.protocol_versions_counter for voted_version; change this to compute the required vote count dynamically from the same state used by the code under test (e.g., derive from the validator set size or threshold function) and insert that computed value instead; update the setup where protocol_versions_counter.global_cache is populated (referencing voted_version and platform.drive.cache.protocol_versions_counter) so it uses the computed threshold (e.g., ceil(validator_count * required_fraction) or by calling the same threshold helper used in the implementation) rather than the hardcoded 100; apply the same change for the other occurrences in the block covering lines ~388-399.packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/check_for_desired_protocol_upgrade/v1/mod.rs (1)
232-253: Consider dynamically calculating the threshold like the other tests.This test hardcodes the assumption of a 75% threshold (comment on line 241-242 and the magic number 7501). Unlike
test_v1_upgrade_with_votes_exactly_at_thresholdandtest_v1_no_upgrade_with_one_below_threshold, which dynamically retrieveprotocol_version_upgrade_percentage_needed, this test would silently become incorrect if the percentage configuration changes.♻️ Suggested refactor to use dynamic threshold calculation
#[test] fn test_v1_upgrade_with_large_number_of_hpmns() { let platform_version = PlatformVersion::latest(); let platform = TestPlatformBuilder::new() .build_with_mock_rpc() .set_initial_state_structure(); let next_version = platform_version.protocol_version + 1; - // With 10000 active hpmns and 75% threshold: - // required = 1 + (10000 * 75 / 100) = 7501 + let pct = platform_version + .drive_abci + .methods + .protocol_upgrade + .protocol_version_upgrade_percentage_needed; + let required = 1 + (10000u64 * pct / 100); + { let mut counter = platform.drive.cache.protocol_versions_counter.write(); - counter.global_cache.insert(next_version, 7501); + counter.global_cache.insert(next_version, required); } let result = platform .check_for_desired_protocol_upgrade_v1(10000, platform_version) .expect("expected no error"); assert_eq!(result, Some(next_version)); }🤖 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/protocol_upgrade/check_for_desired_protocol_upgrade/v1/mod.rs` around lines 232 - 253, In test_v1_upgrade_with_large_number_of_hpmns replace the hardcoded 7501 with a dynamic calculation: read the upgrade percentage via platform_version.protocol_version_upgrade_percentage_needed (or the equivalent accessor used by the other tests), compute required = 1 + (active_hpmns * percent / 100) (using the same integer rounding semantics as the other tests), and insert that computed value into counter.global_cache.insert(next_version, required) so the test adapts if the percentage config changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.rs`:
- Around line 47-68: The test `test_dispatcher_none_version_returns_none` is
incomplete—after setting
`platform_version.drive_abci.methods.block_end.should_checkpoint = None` it
never calls the dispatcher or asserts behavior; either remove the test or
complete it by constructing a `BlockExecutionContext` (reuse the setup from the
neighboring test), call the dispatcher entrypoint `should_checkpoint` (the
function under test) with the `platform`, `platform_version`, and the
`BlockExecutionContext`, and assert that the result is Ok(None); ensure you
reference the same `TestPlatformBuilder` setup and `PlatformVersion::latest()`
used above so the dispatcher short-circuits on the None v0 method and returns
Ok(None).
In
`@packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/upgrade_protocol_version/v0/mod.rs`:
- Around line 247-251: The current early return when PlatformVersion::first()
matches platform_version.protocol_version silently skips the test; replace the
implicit pass with a hard precondition check: instead of returning, assert or
panic with a clear message so the test fails loudly when the precondition isn't
met (e.g., use an assert! or panic! referencing older_version and
platform_version.protocol_version). Locate the block using
PlatformVersion::first(), older_version, and platform_version.protocol_version
in the test and change the early return to an assertion that the versions
differ, providing a descriptive message.
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/check_for_desired_protocol_upgrade/v1/mod.rs`:
- Around line 232-253: In test_v1_upgrade_with_large_number_of_hpmns replace the
hardcoded 7501 with a dynamic calculation: read the upgrade percentage via
platform_version.protocol_version_upgrade_percentage_needed (or the equivalent
accessor used by the other tests), compute required = 1 + (active_hpmns *
percent / 100) (using the same integer rounding semantics as the other tests),
and insert that computed value into counter.global_cache.insert(next_version,
required) so the test adapts if the percentage config changes.
In
`@packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/mod.rs`:
- Around line 82-115: Rename the test function
test_perform_events_when_version_method_is_none to reflect that it validates the
no-op transition when previous_protocol_version == current (e.g.,
test_perform_events_noop_same_version) and update the inline comments to state
that PlatformVersion::latest() has Some(0) and that the test exercises the
same-version no-op path; ensure you keep the call to
platform.perform_events_on_first_block_of_protocol_change and its assertions
unchanged.
In
`@packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/upgrade_protocol_version/v0/mod.rs`:
- Around line 383-386: The test setup currently inserts a magic vote count of
100 into platform.drive.cache.protocol_versions_counter for voted_version;
change this to compute the required vote count dynamically from the same state
used by the code under test (e.g., derive from the validator set size or
threshold function) and insert that computed value instead; update the setup
where protocol_versions_counter.global_cache is populated (referencing
voted_version and platform.drive.cache.protocol_versions_counter) so it uses the
computed threshold (e.g., ceil(validator_count * required_fraction) or by
calling the same threshold helper used in the implementation) rather than the
hardcoded 100; apply the same change for the other occurrences in the block
covering lines ~388-399.
In
`@packages/rs-drive-abci/src/execution/platform_events/tokens/validate_token_aggregated_balance/v0/mod.rs`:
- Around line 45-82: Add a new test that injects a deliberate token-balance
mismatch and asserts the error path: create a platform (use
TestPlatformBuilder), start a transaction, then either mutate the genesis state
to create an aggregated balance imbalance or mock calculate_total_tokens_balance
to return an inconsistent sum; call
validate_token_aggregated_balance_v0(&transaction, platform_version) and assert
it returns Err matching ExecutionError::CorruptedTokensNotBalanced when
execution.verify_token_sum_trees is true, then repeat with a config where
execution.verify_token_sum_trees = false and assert Ok(); reference the existing
test names
(test_validate_token_aggregated_balance_with_verification_enabled/disabled), the
function validate_token_aggregated_balance_v0, and the error type
ExecutionError::CorruptedTokensNotBalanced when implementing the assertions and
setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: faacd33c-976b-4a20-a9af-bfaed1c4e457
📒 Files selected for processing (11)
packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/update_drive_cache/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/block_end/validator_set_update/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/check_for_desired_protocol_upgrade/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/check_for_desired_protocol_upgrade/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/check_for_desired_protocol_upgrade/v1/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/upgrade_protocol_version/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/upgrade_protocol_version/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/tokens/validate_token_aggregated_balance/mod.rspackages/rs-drive-abci/src/execution/platform_events/tokens/validate_token_aggregated_balance/v0/mod.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3270 +/- ##
============================================
- Coverage 69.64% 67.60% -2.05%
============================================
Files 3292 3292
Lines 258981 259897 +916
============================================
- Hits 180372 175699 -4673
- Misses 78609 84198 +5589
🚀 New features to boost your workflow:
|
- Complete the should_checkpoint None-version test by actually calling should_checkpoint and asserting the result is None - Replace early return with assert_ne! in the protocol version mismatch test to avoid silently passing when preconditions are not met Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 test coverage for
packages/rs-drive-abci/src/execution/platform_events/modules, focusing on the lowest-coverage modules first (protocol_upgrade at 43%, block_end at 49%, tokens at 63%).What was done?
Added 38 new unit tests across 11 files covering:
protocol_upgrade (43% -> improved):
check_for_desired_protocol_upgradev0 and v1: threshold boundary tests (exact threshold, one below), no-vote/sufficient-vote/insufficient-vote cases, multiple-versions-passing incoherence error, block cache vote contribution, edge cases (zero hpmns, single hpmn, large hpmn count)upgrade_protocol_version_on_epoch_changev0: epoch change cache blocking behavior, fee version insertion on empty map, non-epoch-change same-version pass-through, protocol version mismatch error on non-epoch-change, genesis epoch bypass (no cache blocking), upgrade vote integrationperform_events_on_first_block_of_protocol_change: same-version no-op, unknown version dispatch errorblock_end (49% -> improved):
update_drive_cachev0: block cache merge without epoch change, global cache clear+unblock on epoch change, non-clearing behavior without epoch change, contracts cache mergevalidator_set_updatedispatcher: unknown version error handlingshould_checkpointdispatcher: None version returns None, unknown version errortokens (63% -> improved):
validate_token_aggregated_balancev0: verification enabled path on fresh genesis state, verification disabled path always succeedsHow Has This Been Tested?
All 38 new tests pass:
Code formatted with
cargo fmt --package drive-abci.Breaking Changes
None. This PR only adds tests.
Checklist:
Summary by CodeRabbit