test(drive-abci): add tests for epoch-based token distribution for evonodes #2626
Conversation
WalkthroughThis update modifies panic messages for drive and platform state app hash mismatches in several handler files to add contextual suffixes. It also enhances test coverage by adding three new tests for perpetual token distribution under various conditions, and generalizes strategy logic for initial contract state transitions based on block height. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Token Distribution Test
participant Platform as Platform
participant State as State/DB
participant Identity as Identity
Test->>Platform: Initialize chain with token contract & identities
loop For each block
Platform->>State: Process block, update state
end
Test->>Platform: Create token claim transition (with Identity)
Platform->>State: Process claim, update token balances
State-->>Test: Return updated balances
Test->>Test: Assert balances and block proposals
Note over Test: Repeat for chain continuation and<br>different distribution parameters
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
QuantumExplorer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/rs-drive-abci/tests/strategy_tests/token_tests.rs (4)
639-646: Comment/documentation and assertion diverge – update the math in the commentThe explanatory comment states the expected reward is 250, but the integer-division result used in the assertion is 222.
Keeping both in sync avoids future confusion for anyone adjusting the reward algorithm or test constants.-// They should get 2 * 1000 * 4 / 36 -// This is equal to 250 +// They should get 2 * 1000 * 4 / 36 +// This equals 222 with integer division (floor)
740-744:verify_sum_treesdisabled – confirm intentTurning off
verify_sum_treesspeeds the tests up, but it also removes an integrity check that has previously caught state-root mismatches.
If the only goal is runtime reduction, consider parameterising this flag so “fast” and “full-verify” modes can be toggled centrally for all strategy tests instead of hard-coding per test.
Otherwise, please add a short comment clarifying why full verification is not required here.
2000-2012: Hard-coded expected balances make tests brittleThe expected token balances are derived from hand-rolled formulas scattered across comments.
If the distribution logic changes (or integer vs. fixed-point math is introduced) these literals will need manual updates.Consider extracting the reward formula into a helper so you can compute the expected value programmatically:
fn expected_linear_reward(blocks: u64, proposed: u64, a: i64, start: i64) -> u64 { let total = a * blocks as i64 + start; (total * proposed as i64 / blocks as i64) as u64 }This keeps tests self-documenting and resilient to parameter tweaks.
Also applies to: 2319-2333
80-90: Repeated boiler-plate could be extracted into helpersEach test recreates nearly identical blocks for:
- Seeding RNG & signer
- Creating two identities and their transitions
- Building
PlatformConfig/NetworkStrategyAbstracting these into helper functions (e.g.,
build_basic_token_contract(...),base_platform_config(day_ms)) would shorten the test file and emphasise the unique parts of each scenario.Also applies to: 237-247, 670-680, 1203-1215, 1800-1813
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/rs-drive-abci/src/abci/handler/info.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/prepare_proposal.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/process_proposal.rs(1 hunks)packages/rs-drive-abci/tests/strategy_tests/execution.rs(1 hunks)packages/rs-drive-abci/tests/strategy_tests/strategy.rs(2 hunks)packages/rs-drive-abci/tests/strategy_tests/token_tests.rs(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
packages/rs-drive-abci/tests/strategy_tests/execution.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:119-120
Timestamp: 2024-10-06T16:18:07.994Z
Learning: In the `run_block_proposal` function in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, it's acceptable to pass `platform_state` to `perform_events_on_first_block_of_protocol_change`, even if `block_platform_state` has been updated.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (7)
packages/rs-drive-abci/src/abci/handler/prepare_proposal.rs (1)
75-77: Approve enhanced panic context
The panic message now includes “(prepare proposal)” to explicitly identify the handler phase when a hash mismatch occurs. This change improves debug clarity and is consistent with sibling handlers. No behavior is altered.packages/rs-drive-abci/src/abci/handler/info.rs (1)
63-65: Approve enhanced panic context
Appending “(info)” clarifies that the app-hash mismatch is detected during the ABCI info handshake. This aligns with the updates in other handlers and preserves existing logic.packages/rs-drive-abci/src/abci/handler/process_proposal.rs (1)
219-221: Approve enhanced panic context
Adding “(process proposal)” makes it clear which phase triggered the panic on hash mismatch. This mirrors the pattern in the other handlers without impacting functionality.packages/rs-drive-abci/tests/strategy_tests/execution.rs (1)
1014-1014: Update method call to pass block_start parameter.The call to
strategy.state_transitions_for_blockhas been updated to pass theblock_startparameter, aligning with the modified method signature instrategy.rs. This change is necessary to support the generalized approach for handling initial contract state transitions at different block heights.packages/rs-drive-abci/tests/strategy_tests/strategy.rs (2)
1598-1598: Update method signature to support generalized block height handling.The
state_transitions_for_blockmethod signature now includes a new parameterstart_block_height, allowing the caller to specify when initial contract state transitions should be applied, rather than hardcoding this to block height 1.
1634-1648: Generalize initial contract state transitions logic.The code now generalizes when initial contract state transitions are added:
- Initial contracts are added when the current block height equals the provided
start_block_height(not just block 1) and there are existing identities- Operation transitions are skipped only if it's the first block (height 1), preserving the original behavior
This change enables more flexible testing scenarios, particularly for tests that continue chains from non-genesis blocks or run across multiple epochs.
packages/rs-drive-abci/tests/strategy_tests/token_tests.rs (1)
780-788: Great defensive check on state-transition execution resultsIterating over every transition and asserting
code == 0gives immediate, fine-grained feedback if something fails mid-chain – nice touch!
1 participant
Issue being fixed or feature implemented
This PR adds tests to verify the functionality of epoch-based token distribution for evonodes, ensuring that the distribution logic works as expected across multiple epochs.
What was done?
How Has This Been Tested?
The new tests were executed in the test suite, confirming that the epoch-based distribution logic behaves correctly under various scenarios, including different block counts and proposals.
Breaking Changes
None
Checklist
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
For repository code-owners and collaborators only
I have assigned this pull request to a milestone
Summary by CodeRabbit
Bug Fixes
Tests