Unit tests for platform-consensus
#14
Conversation
Add 57 new tests covering previously untested methods in governance, PBFT, stake-weighted consensus, and state management modules. Key areas tested: - Governance: all SudoAction types, proposal titles/descriptions, state mutations - PBFT: proposal validation, vote handling, timeouts, consensus thresholds - Stake-weighted: stake-based voting, double-vote prevention, thresholds - Stake governance: voting edge cases, rate limiting, bootstrap authority, proposal lifecycle (create/vote/execute/cancel), hybrid governance
|
Warning Rate limit exceeded@cuteolaf has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 40 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds intentional aliasing in sudo_action_to_governance_type and introduces extensive, additive test suites across consensus components (governance_integration, PBFT, stake_governance, stake_weighted_pbft, and consensus state). No public API signature changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/consensus/src/stake_governance.rs (1)
1571-2012: Tests are tightly coupled to internal fields; consider thin helpersSeveral tests (
test_vote_on_expired_proposal,test_vote_on_executed_proposal,test_vote_on_cancelled_proposal,test_cleanup_expired,test_rate_limit_resets_daily) mutate internal fields likeproposalsandproposal_countsdirectly. This works because tests share the module, but it tightly couples them to implementation details and can make refactors noisier.If this area evolves further, consider small, test-only helper methods (e.g.,
mark_expired_for_test,set_rate_limit_for_test) or constructing the desired state via public APIs where practical, to keep tests more robust against internal changes.crates/consensus/src/stake_weighted_pbft.rs (2)
947-958: You may want to assert the “vote NO on invalid config” behavior explicitly
test_handle_proposal_invalid_configcurrently only checks thathandle_proposalreturnsOk(())for an invalidAddChallengeconfig. Given the implementation callsvalidate_proposaland then internally votesfalse, you might consider also asserting that:
- No challenge was added to
chain_state.challenge_configs, or- The round exists with an initial reject vote.
That would more directly lock in the “invalid configs are rejected but don’t error” behavior.
1515-1521: Directly testingadd_vote_with_stakecouples tests to internals
test_add_vote_with_stake_round_not_foundcalls the private methodadd_vote_with_stakedirectly. This is fine in-module, but does couple tests to the internal API surface.If you later refactor the internals (e.g., rename or change visibility), consider re-expressing this via the public voting path (
handle_vote/vote_internal) plus setup that omits the round, so the behavior remains tested without depending on private method signatures.crates/consensus/src/governance_integration.rs (1)
232-250: Confirm that conflating some sudo actions into shared governance types is intentional
sodo_action_to_governance_typemaps:
SetMechanismConfig→GovernanceActionType::SetMechanismBurnRateRefreshChallenges→GovernanceActionType::UpdateChallengeand
test_sudo_action_conversion_all_variantsexplicitly asserts these mappings.If the goal is just to bucket related actions for governance classification/limits, this is fine; but if you ever need per-action visibility (e.g., rate-limiting
SetMechanismConfigseparately fromSetMechanismBurnRate), these tests will make that refactor slightly more involved.Might be worth a brief comment near the mapping explaining that this aliasing is intentional, so future readers don’t assume it’s a bug.
Also applies to: 508-621
crates/consensus/src/state.rs (1)
427-451:test_add_vote_updates_phasedoesn’t actually assert the phase changeThe comment says “should update phase”, but the test currently only checks that the round is still present:
let round = state.get_round(&proposal_id); assert!(round.is_some());To fully exercise the behavior in
add_vote(wherephaseis set toConsensusPhase::Preparewhen approvals > 0 and no consensus), consider asserting on the phase as well, e.g.assert_eq!(round.unwrap().phase, ConsensusPhase::Prepare);.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/consensus/src/governance_integration.rscrates/consensus/src/pbft.rscrates/consensus/src/stake_governance.rscrates/consensus/src/stake_weighted_pbft.rscrates/consensus/src/state.rs
🧰 Additional context used
🧬 Code graph analysis (3)
crates/consensus/src/stake_governance.rs (2)
crates/core/src/crypto.rs (1)
hotkey(62-64)crates/consensus/src/governance_integration.rs (1)
can_use_bootstrap(204-208)
crates/consensus/src/state.rs (3)
crates/consensus/src/pbft.rs (1)
vote(145-166)crates/consensus/src/types.rs (1)
default(121-127)crates/core/src/message.rs (1)
approve(596-603)
crates/consensus/src/pbft.rs (2)
crates/consensus/src/stake_weighted_pbft.rs (6)
new(53-62)new(195-207)apply_sudo_action(518-642)validate_sudo_action(664-671)validate_proposal(645-662)status(698-708)crates/consensus/src/governance_integration.rs (3)
new(43-57)new(454-456)apply_sudo_action(316-442)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (2)
crates/consensus/src/stake_governance.rs (1)
1524-1550: Good coverage of bootstrap execution paths
test_can_executeandtest_execute_bootstrap_action_non_ownernicely lock in owner vs non-owner behavior before/afterBOOTSTRAP_END_BLOCK, directly exercisingexecute_bootstrap_actionrather than going through higher layers. This should make future regressions around bootstrap authority obvious.crates/consensus/src/pbft.rs (1)
501-706: PBFT test coverage around sudo and proposal validation looks solidThe new tests around
handle_proposal/handle_vote, invalid challenge configs, andvalidate_sudo_action/validate_proposal(including non-sudo proposers and future block heights) line up well with the PBFT engine’s security checks. Combined with theapply_sudo_action_*tests, they provide good confidence that sudo flows and state mutations behave as expected without touching production code.
- Add test-only helper methods in stake_governance.rs to reduce coupling to internal fields - Improve test_handle_proposal_invalid_config to verify rejection behavior - Add documentation comment explaining intentional sudo action type aliasing in governance_integration.rs (SetMechanismConfig -> SetMechanismBurnRate, RefreshChallenges -> UpdateChallenge) - Fix test_add_vote_updates_phase to actually assert phase change to Prepare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @crates/consensus/src/governance_integration.rs:
- Line 503: Remove the unused import ChallengeConfig from the top of
governance_integration.rs: delete the use platform_core::ChallengeConfig line
and ensure any references already use ChallengeContainerConfig (no other code
changes required).
🧹 Nitpick comments (6)
crates/consensus/src/governance_integration.rs (1)
1082-1096: Consider asserting on therequiredstake value.The test retrieves
(required, threshold)but only asserts onthreshold. Consider adding an assertion thatrequiredis calculated correctly based on the total stake.💡 Suggested improvement
#[tokio::test] async fn test_stake_for_consensus() { let keypair = Keypair::generate(); let state = Arc::new(RwLock::new(ChainState::new( keypair.hotkey(), platform_core::NetworkConfig::default(), ))); let (tx, _rx) = tokio::sync::mpsc::channel(100); let pbft = Arc::new(PBFTEngine::new(keypair.clone(), state.clone(), tx)); let gov_pbft = GovernancePBFT::new(pbft, state, keypair); let (required, threshold) = gov_pbft.stake_for_consensus(); assert_eq!(threshold, crate::stake_governance::STAKE_THRESHOLD_PERCENT); + // With zero total stake, required should be 0 + assert_eq!(required.0, 0); }crates/consensus/src/stake_governance.rs (4)
2200-2221: Direct field access bypasses the test helper pattern.This test directly inserts a proposal via
gov.proposals.write().insert(...)rather than usingcreate_proposal. While this works for testingcheck_consensuswith no validators, it creates inconsistency with other tests that use the proper creation flow.Consider documenting why this approach is necessary here (no stake requirement for test setup).
2404-2429: Inconsistent use of test helpers vs direct field access.Lines 2424-2425 directly modify
proposal.cancelledand insert it viagov.proposals.write(), while other tests usemark_cancelled_for_test(). For consistency, consider using the helper:- // Mark as cancelled - proposal.cancelled = true; - gov.proposals.write().insert(proposal.id, proposal.clone()); + // Mark as cancelled using helper + gov.mark_cancelled_for_test(proposal.id);
2431-2456: Same inconsistency with expiration.Lines 2450-2452 directly modify
expires_atand reinsert the proposal. Themark_expired_for_testhelper already exists and is used elsewhere (e.g., line 1621). Consider using it here for consistency.
2355-2365: Test accesses private method directly.
increment_proposal_countis a private method being tested directly. While this works because tests are in the same module, consider whether this behavior should be tested indirectly throughcreate_proposalinstead, as that's how it's actually used.crates/consensus/src/stake_weighted_pbft.rs (1)
1597-1603: Consider using!timeouts.is_empty()instead of> 0.Clippy typically recommends
!timeouts.is_empty()overtimeouts.len() > 0for readability.♻️ Suggested fix
- assert!(timeouts.len() > 0); + assert!(!timeouts.is_empty());
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/consensus/src/governance_integration.rscrates/consensus/src/stake_governance.rscrates/consensus/src/stake_weighted_pbft.rscrates/consensus/src/state.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/consensus/src/state.rs
🔇 Additional comments (11)
crates/consensus/src/governance_integration.rs (4)
233-240: Good documentation for intentional aliasing.The added comments clearly explain why some
SudoActionvariants map to the sameGovernanceActionType. This documentation helps future maintainers understand the design decision.
518-631: Comprehensive coverage of all SudoAction variants.The test methodically verifies the mapping for each
SudoActionvariant, including the intentional aliasing cases (SetMechanismConfig→SetMechanismBurnRate,RefreshChallenges→UpdateChallenge). This is good practice for ensuring the mapping function behaves correctly.
805-941: Thorough state mutation testing.This test covers all
SudoActionvariants applied toChainState, verifying both success and the resulting state changes. The test properly validates state fields after each action.
969-1000: Async tests for GovernancePBFT initialization and block height.Tests properly use
tokio::testand verify basic initialization and state management. The MPSC channel setup pattern is appropriate for testing async consensus components.crates/consensus/src/stake_governance.rs (2)
1207-1236: Well-designed test helpers.These
#[cfg(test)]helper methods provide clean ways to manipulate internal state for testing edge cases without exposing implementation details to production code. The pattern of adding test-only methods inside animplblock is idiomatic Rust.
1971-2002: Good rate limit test coverage.This test verifies that the rate limit of
MAX_PROPOSALS_PER_DAYis enforced. The loop creating proposals and the final assertion checking rejection is correct.crates/consensus/src/stake_weighted_pbft.rs (5)
916-930: Good edge case test for proposal signature verification.Tests that a proposal with mismatched signer is rejected, which is an important security check.
946-967: Test correctly verifies invalid config rejection behavior.The test validates that an invalid challenge config (empty name/image) doesn't get added to state even though
handle_proposalreturnsOk. This documents the "vote NO internally" behavior described in the comment.
1463-1476: Clever use of negative timeout for immediate timeout testing.Using
-1as timeout creates an immediately timed-out round, which is a clean way to test timeout behavior without waiting. This pattern is reused intest_check_timeouts_removes_timed_out.
1533-1552: Good test for hash mismatch behavior.This test verifies that a
NewBlockproposal with a non-matching state hash doesn't increment the block height, which is correct defensive behavior.
1021-1075: Comprehensive sudo action variant testing.This test covers multiple sudo action variants (
SetChallengeWeight,SetMechanismBurnRate,SetMechanismConfig,EmergencyPause,Resume,ForceStateUpdate) in a single test. The pattern of acquiring/dropping locks between actions is correct for avoiding deadlocks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/consensus/src/stake_governance.rs (1)
2415-2431: Unnecessarymutonproposalvariable.The
proposalvariable is not mutated directly — only itsidfield is read. The modification happens throughgov.mark_cancelled_for_test(). Same issue occurs at line 2441.✨ Suggested fix
- let mut proposal = gov + let proposal = gov
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/consensus/src/governance_integration.rscrates/consensus/src/stake_governance.rscrates/consensus/src/stake_weighted_pbft.rs
🧰 Additional context used
🧬 Code graph analysis (3)
crates/consensus/src/stake_weighted_pbft.rs (1)
crates/consensus/src/pbft.rs (3)
create_test_engine(459-469)new(29-41)vote(145-166)
crates/consensus/src/governance_integration.rs (3)
crates/consensus/src/stake_governance.rs (7)
new(143-166)new(223-232)new(332-341)new(908-913)new(1051-1053)stake_governance(1146-1148)can_use_bootstrap(838-843)crates/consensus/src/stake_weighted_pbft.rs (3)
new(53-62)new(195-207)apply_sudo_action(518-642)crates/consensus/src/pbft.rs (2)
new(29-41)apply_sudo_action(237-362)
crates/consensus/src/stake_governance.rs (2)
crates/core/src/crypto.rs (1)
hotkey(62-64)crates/consensus/src/governance_integration.rs (1)
can_use_bootstrap(204-208)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Clippy
🔇 Additional comments (8)
crates/consensus/src/stake_governance.rs (2)
1207-1236: Well-designed test helpers that reduce internal coupling.These helper methods cleanly encapsulate the mutation of internal state for testing purposes, avoiding direct field access scattered throughout tests. The
#[cfg(test)]guards ensure they won't be compiled into production builds.
1555-2519: Comprehensive test coverage for stake governance.The test suite provides thorough coverage including:
- Bootstrap behavior (owner/non-owner)
- Proposal lifecycle (create, vote, execute, cancel, expire, cleanup)
- Rate limiting with daily reset
- Edge cases (not found, no stake, double voting, unauthorized cancellation)
- HybridGovernance authorization scenarios
The tests are well-structured and use the new helper methods effectively.
crates/consensus/src/governance_integration.rs (3)
232-260: Well-documented intentional aliasing for governance classification.The documentation clearly explains that
SetMechanismConfigandRefreshChallengesare intentionally mapped to existingGovernanceActionTypevariants for grouping related actions. This is appropriate for governance classification and rate-limiting purposes.The comment also provides guidance for future maintainers on when to add new variants.
517-940: Thorough test coverage for sudo action conversion and application.The tests comprehensively cover:
- All
SudoActionvariant conversions toGovernanceActionType- Title and description generation for all action types
- State mutations for each sudo action type with proper assertions
The tests verify the intentional aliasing behavior (
SetMechanismConfig→SetMechanismBurnRate,RefreshChallenges→UpdateChallenge) is working as documented.
968-1145: Good async test coverage for GovernancePBFT.The tests verify:
- Engine initialization and status
- Block height propagation to governance layer
- Stake updates from metagraph sync
- Bootstrap period authorization (owner vs non-owner)
- Post-bootstrap proposal creation flow
crates/consensus/src/stake_weighted_pbft.rs (3)
946-967: Test correctly verifies invalid config rejection flow.The test validates that when
handle_proposalreceives an invalid challenge config (empty name/image), it internally votes NO rather than returning an error. The assertion that the challenge isn't in chain state is correct since the proposal never achieved consensus.
1463-1476: Valid test technique using negative timeout.Using a negative timeout value (
-1) to create an immediately-timed-out round is an effective way to test timeout behavior without introducing actual delays. Thechrono::Duration::seconds(-1)results in a past deadline, correctly triggeringis_timed_out()to returntrue.
916-1603: Comprehensive test suite for stake-weighted PBFT.The test suite provides excellent coverage including:
- Proposal handling: wrong signer, non-sudo proposer, invalid configs
- Vote handling: non-validator, signature mismatch, zero stake
- All
SudoActionvariants with state mutation verification- Validation paths for proposals and sudo actions
- Consensus threshold computation and stake weighting
- Timeout detection and round cleanup
- Edge cases: zero total stake, hash mismatch, round not found
The tests complement the similar coverage in
pbft.rswhile focusing on stake-weighted specifics.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @crates/consensus/src/stake_governance.rs:
- Around line 1709-1735: Update the misleading test comment in
test_vote_rejection: change the comment that reads "Should be rejected due to
>50% stake voting NO" to reflect the actual threshold constant
STAKE_THRESHOLD_PERCENT = 33.0 (e.g., "Should be rejected due to >33% stake
voting NO") so the comment matches the governance threshold enforced by
gov.vote/proposal.id in the test_vote_rejection test.
- Around line 1685-1707: The test test_vote_no_stake registers only kp1 in
update_validator_stakes so kp2 (created with zero stake) is never a known
validator and the vote fails on "must be a validator" instead of "has no stake";
fix by registering kp2 as a validator with zero stake before calling vote —
e.g., capture the second stake from create_test_validator(0) and include it in
the vec passed to gov.update_validator_stakes so gov.vote(proposal.id,
&kp2.hotkey(), ...) exercises the zero-stake voting path.
🧹 Nitpick comments (3)
crates/consensus/src/stake_governance.rs (3)
1555-1569: Consider renaming test for clarity.The test name
test_can_executeis misleading sincecan_execute()is a method onGovernanceProposalthat checks if a proposal can be executed (not expired, not cancelled, voting period complete). This test actually validatesexecute_bootstrap_action. Consider renaming totest_execute_bootstrap_action_ownerfor consistency with the adjacent test.
2200-2223: Consider adding a test helper for proposal insertion.This test directly accesses
gov.proposals.write()to insert a proposal. While valid since tests are in the same module, this couples the test to internal field names. For consistency with the other test helpers (mark_expired_for_test, etc.), consider adding aninsert_proposal_for_testhelper.♻️ Suggested helper
Add to the test helpers block (lines 1207-1236):
#[cfg(test)] fn insert_proposal_for_test(&self, proposal: GovernanceProposal) -> Uuid { let id = proposal.id; self.proposals.write().insert(id, proposal); self.votes.write().insert(id, Vec::new()); id }
2357-2368: Testing private method creates implementation coupling.This test directly calls the private
increment_proposal_countmethod and accesses internal state. While this provides focused coverage,test_check_rate_limitalready validates this behavior through the public API. Consider whether this test adds sufficient value to justify the coupling.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/consensus/src/stake_governance.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/consensus/src/stake_governance.rs (2)
crates/consensus/src/governance_integration.rs (1)
can_use_bootstrap(204-208)crates/consensus/src/stake_weighted_pbft.rs (2)
total_stake(210-212)status(698-708)
🔇 Additional comments (5)
crates/consensus/src/stake_governance.rs (5)
1207-1236: Well-structured test helpers for state manipulation.These test-only helpers nicely encapsulate internal state mutations needed for testing various proposal lifecycle states. This reduces coupling between tests and internal implementation details.
1601-1683: Good coverage of proposal state transitions.These tests effectively use the new test helpers to verify voting behavior across different proposal states (expired, executed, cancelled). The pattern is consistent and the assertions match the expected behavior from the production code.
1971-2037: Comprehensive rate limiting tests.The rate limiting tests properly cover both the limit enforcement and the daily reset logic. Using
set_rate_limit_for_testto simulate a previous day's timestamp is a clean approach.
2106-2189: Thorough HybridGovernance authorization tests.These tests effectively validate all authorization paths using the closure-execution flag pattern to verify when actions are immediately executed versus deferred to proposal. Good coverage of bootstrap and post-bootstrap scenarios.
1554-2519: Comprehensive test suite for stake governance.This is an extensive addition of unit tests covering proposal lifecycle, voting, rate limiting, bootstrap authority, and hybrid governance authorization. The tests effectively use the newly-added test helpers to simulate various proposal states. Overall, this is a solid test coverage expansion.
A few minor issues were noted above regarding test accuracy for zero-stake paths and comment consistency, but these don't affect the overall quality of the test suite.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.