test(drive): improve coverage for tokens subtree#3503
Merged
Conversation
Add 86 new tests targeting the lowest-coverage modules under packages/rs-drive/src/drive/tokens/ and their dispatch paths: - mint_many (0% → covered): 13 unit tests in rs-drive exercising the proportional weight distribution, u32::MAX weight clamp, last-recipient remainder, and total-supply update. Placed in rs-drive because TokenMintMany is only reachable internally via TokenOperationType, not via a user-facing state transition. - unfreeze (39% → covered): 11 drive-abci integration tests covering authorized/unauthorized actors, non-frozen-identity rejection, group multi-sig flows, and verifying the unfrozen state allows subsequent transfers. - burn (29% → covered): 21 new drive-abci integration tests added on top of the existing 6 — exercising supply-update read-back, public notes, zero-amount rejection, specific-identity/ContractOwner/NoOne/MainGroup authorization rules, sequential depletion, and several group-action error branches (already-signed, already-completed, modified-main-params, non-member proposer/confirmer, confirmer-with-note). - system (68% → covered): 21 inline unit tests across add_to_token_total_supply, remove_from_token_total_supply, fetch_token_total_supply, fetch_token_total_aggregated_identity_balances, and create_token_trees covering happy paths, overflow/underflow, saturation, first-mint semantics, corrupted-state errors, and idempotence. - info (73% → covered): 20 inline unit tests across fetch_identity_token_info, fetch_identity_token_infos, fetch_identities_token_infos, and queries.rs covering PathKeyNotFound→None coercion, partial-hit map branches, with_costs fee paths, and all four range-query branches. All 165 touched tests pass; no production bugs surfaced during coverage work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 44 minutes and 45 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 (4)
📝 WalkthroughWalkthroughThis pull request adds comprehensive test coverage for token-related state transitions and operations across the Dash blockchain codebase. New test modules cover token burn, unfreeze, and mint-many operations, along with unit tests for token info fetching, supply management, and system operations. No production logic is modified. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
Collaborator
Review GateCommit:
|
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/burn/mod.rs (1)
610-632:⚠️ Potential issue | 🟡 MinorMisleading inline comments contradict the assertions.
The assertions expect
identity1to retain 98,663 tokens andrecipientto hold 1,337, but the comments on lines 631–632 claim the original identity should have "no tokens" and that the recipient "should not keep transferred tokens if burn was enforced" — which would be the behavior of a failing scenario, not this one. These comments look copy-pasted from the "not enough balance" sibling test and will mislead readers. Same issue exists at line 874 ("Validate the burn still succeeded...") intest_token_burn_group_action_tokens_transferred_before_completion_not_enough_balancewhere the burn actually fails.✏️ Proposed fix
- assert_eq!(balance1, Some(98663)); // Original identity should have no tokens - assert_eq!(balance2, Some(1337)); // Recipient should not keep transferred tokens if burn was enforced + // Proposer had base_supply 100000, transferred 1337 away, then the + // group-finalized burn removed 100000 - 1337 = 98663 (capped by + // remaining balance). So the proposer ends at 98663 and the recipient + // keeps the 1337 that was transferred before the confirmation. + assert_eq!(balance1, Some(98663)); + assert_eq!(balance2, Some(1337));Also please fix the analogous misleading comment at line 874 in the
_not_enough_balancevariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/burn/mod.rs` around lines 610 - 632, Update the misleading inline comments to match the assertions and actual test behavior: in the test where balances assert balance1 == Some(98663) and balance2 == Some(1337) (e.g., test_token_burn_group_action_tokens_transferred_before_completion) replace the comments that say "Original identity should have no tokens" and "Recipient should not keep transferred tokens if burn was enforced" with ones stating the original identity retains 98,663 tokens and the recipient holds 1,337 because the burn succeeded; likewise, in the sibling test test_token_burn_group_action_tokens_transferred_before_completion_not_enough_balance update the comment at the analogous location (around line 874) to reflect that the burn failed and expected balances reflect that failure. Ensure you edit the comments near the fetch_identity_token_balance assertions to accurately describe the expected outcomes.
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/burn/mod.rs (1)
3482-3589: Test name does not match what it exercises.
test_token_burn_by_one_then_by_another_both_independentimplies two identities each perform a burn, but the body only executes a single burn fromowner—holderis merely seeded with a balance and never burns (the defaultContractOwnerrule would forbid it anyway, as the inline comment acknowledges). Consider renaming to something liketest_token_burn_only_affects_burner_balance_and_aggregates_in_total_supplyso the name reflects the actual behavior under test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/burn/mod.rs` around lines 3482 - 3589, Rename the test function test_token_burn_by_one_then_by_another_both_independent to a name that reflects the actual behavior (e.g. test_token_burn_only_affects_burner_balance_and_aggregates_in_total_supply) by updating the fn identifier and any references to it in this module; keep the test body unchanged (it seeds a second holder but only owner performs the burn), and update any inline comment or test description to match the new name so the test name accurately describes that only the burner’s balance is reduced and total supply changes accordingly.packages/rs-drive/src/drive/tokens/mint_many/v0/mod.rs (1)
563-572: Useassert_eq!for clearer failure messages.
assert!(x == y)loses the actual-vs-expected diagnostic thatassert_eq!provides on failure. SinceOption<u64>implementsPartialEq, you can assert the optional directly (or unwrap to0).♻️ Proposed fix
- assert!(balance_a.unwrap_or(0) == 0); - assert!(balance_b.unwrap_or(0) == 0); + assert_eq!(balance_a.unwrap_or(0), 0); + assert_eq!(balance_b.unwrap_or(0), 0); @@ - assert!(supply.unwrap_or(0) == 0); + assert_eq!(supply.unwrap_or(0), 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/tokens/mint_many/v0/mod.rs` around lines 563 - 572, Replace the three equality assertions that use assert!(x == y) with assert_eq! for better diagnostics: change the checks on balance_a and balance_b (currently using balance_a.unwrap_or(0) == 0 and balance_b.unwrap_or(0) == 0) to assert_eq!(balance_a.unwrap_or(0), 0) and assert_eq!(balance_b.unwrap_or(0), 0), and change the token supply check that compares supply.unwrap_or(0) == 0 (from drive.fetch_token_total_supply) to assert_eq!(supply.unwrap_or(0), 0); keep the same unwrap_or(0) semantics so failures print expected vs actual values.
🤖 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/src/drive/tokens/info/fetch_identities_token_infos/v0/mod.rs`:
- Around line 225-235: The test currently only asserts that any values present
in the returned map are None, which misses missing keys; update the assertion
after calling fetch_identities_token_infos_v0(token_id, &identity_ids, ...) to
ensure every requested identity is present and mapped to None by either
asserting infos.len() == identity_ids.len() or iterating over identity_ids and
for each id doing assert!(infos.get(id).is_some() &&
infos.get(id).unwrap().is_none(), ...) so that fetch_identities_token_infos_v0,
token_id, identity_ids, and infos are validated fully.
In
`@packages/rs-drive/src/drive/tokens/info/fetch_identity_token_infos/v0/mod.rs`:
- Around line 227-239: The test currently only asserts that returned values are
None but doesn't verify that every requested token ID is present in the map;
update the check after calling fetch_identity_token_infos_v0 so you iterate over
the original token_ids and assert infos.contains_key(id) and that
infos.get(id).unwrap().is_none() for each id — referencing the variables
token_ids and infos and the call to fetch_identity_token_infos_v0 to ensure the
function returns an entry (Some(None)) per requested ID rather than dropping
missing IDs.
In `@packages/rs-drive/src/drive/tokens/info/queries.rs`:
- Around line 201-214: The test
token_infos_for_range_query_ascending_with_no_start_uses_range_full currently
only asserts that the single QueryItem is RangeFull, but doesn't check its
direction; update the assertion to pattern-match QueryItem::RangeFull(range) and
assert range.ascending == true (since the test calls
Drive::token_infos_for_range_query with ascending=true) so the test fails if the
ascending flag is ignored; make the analogous change in the other test mentioned
(the one around the 290-301 range) to assert range.ascending == false for the
descending case.
In `@packages/rs-drive/src/drive/tokens/system/create_token_trees/v0/mod.rs`:
- Around line 392-436: The test currently only checks supply; update
should_respect_start_as_paused_flag to also fetch and assert each token's stored
paused status: after calling create_token_trees_v0 for token_id_active and
token_id_paused, call drive.fetch_token_status(token_id_active, None,
platform_version).expect(...) and assert the returned status has paused ==
false, and similarly call drive.fetch_token_status(token_id_paused, None,
platform_version).expect(...) and assert paused == true; reference the
create_token_trees_v0, fetch_token_total_supply, and fetch_token_status helpers
and the TokenStatus.paused field when adding these assertions.
---
Outside diff comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/burn/mod.rs`:
- Around line 610-632: Update the misleading inline comments to match the
assertions and actual test behavior: in the test where balances assert balance1
== Some(98663) and balance2 == Some(1337) (e.g.,
test_token_burn_group_action_tokens_transferred_before_completion) replace the
comments that say "Original identity should have no tokens" and "Recipient
should not keep transferred tokens if burn was enforced" with ones stating the
original identity retains 98,663 tokens and the recipient holds 1,337 because
the burn succeeded; likewise, in the sibling test
test_token_burn_group_action_tokens_transferred_before_completion_not_enough_balance
update the comment at the analogous location (around line 874) to reflect that
the burn failed and expected balances reflect that failure. Ensure you edit the
comments near the fetch_identity_token_balance assertions to accurately describe
the expected outcomes.
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/burn/mod.rs`:
- Around line 3482-3589: Rename the test function
test_token_burn_by_one_then_by_another_both_independent to a name that reflects
the actual behavior (e.g.
test_token_burn_only_affects_burner_balance_and_aggregates_in_total_supply) by
updating the fn identifier and any references to it in this module; keep the
test body unchanged (it seeds a second holder but only owner performs the burn),
and update any inline comment or test description to match the new name so the
test name accurately describes that only the burner’s balance is reduced and
total supply changes accordingly.
In `@packages/rs-drive/src/drive/tokens/mint_many/v0/mod.rs`:
- Around line 563-572: Replace the three equality assertions that use assert!(x
== y) with assert_eq! for better diagnostics: change the checks on balance_a and
balance_b (currently using balance_a.unwrap_or(0) == 0 and
balance_b.unwrap_or(0) == 0) to assert_eq!(balance_a.unwrap_or(0), 0) and
assert_eq!(balance_b.unwrap_or(0), 0), and change the token supply check that
compares supply.unwrap_or(0) == 0 (from drive.fetch_token_total_supply) to
assert_eq!(supply.unwrap_or(0), 0); keep the same unwrap_or(0) semantics so
failures print expected vs actual values.
🪄 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: a8889973-8bc2-47f1-affb-0f625cd6ab68
📒 Files selected for processing (14)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/burn/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/mint_many/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/unfreeze/mod.rspackages/rs-drive/src/drive/tokens/info/fetch_identities_token_infos/v0/mod.rspackages/rs-drive/src/drive/tokens/info/fetch_identity_token_info/v0/mod.rspackages/rs-drive/src/drive/tokens/info/fetch_identity_token_infos/v0/mod.rspackages/rs-drive/src/drive/tokens/info/queries.rspackages/rs-drive/src/drive/tokens/mint_many/v0/mod.rspackages/rs-drive/src/drive/tokens/system/add_to_token_total_supply/v0/mod.rspackages/rs-drive/src/drive/tokens/system/create_token_trees/v0/mod.rspackages/rs-drive/src/drive/tokens/system/fetch_token_total_aggregated_identity_balances/v0/mod.rspackages/rs-drive/src/drive/tokens/system/fetch_token_total_supply/v0/mod.rspackages/rs-drive/src/drive/tokens/system/remove_from_token_total_supply/v0/mod.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3503 +/- ##
============================================
+ Coverage 84.84% 85.05% +0.21%
============================================
Files 2476 2476
Lines 267915 269313 +1398
============================================
+ Hits 227303 229062 +1759
+ Misses 40612 40251 -361
🚀 New features to boost your workflow:
|
- fetch_identities_token_infos: assert the full expected map so missing identity IDs are caught, not just that returned values are None. - fetch_identity_token_infos: same — assert missing-token entries via full-map equality. - queries.rs: assert left_to_right direction in the two RangeFull tests so regressions that ignore the ascending flag get caught. - create_token_trees should_respect_start_as_paused_flag: actually fetch and assert TokenStatus.paused for both tokens — the old assertion was identical to should_create_token_trees_and_initialize_supply_to_zero and would pass even if start_as_paused were ignored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 21, 2026
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
The
packages/rs-drive/src/drive/tokens/subtree is currently at ~75% coverage, with three modules near or at zero:mint_many(0%),burn(29%), andunfreeze(39%). The larger mid-tiersystem(68%) andinfo(73%) modules together account for another ~660 uncovered lines. This PR addresses all five.What was done?
Added 86 new tests — 65 unit tests inline in
rs-drive, 21 integration tests indrive-abci:mint_manyrs-driveunit (not reachable via a user-facing state transition — only TokenOperationType)unfreezedrive-abciintegrationburndrive-abciintegrationsystem/*rs-driveunitinfo/*rs-driveunitSpecific paths now exercised:
mint_many — proportional weight distribution via
div_ceil,u32::MAXweight saturation clamp (lines 102-104), last-recipient remainder absorption, total-supply update conservation,allow_first_mint: falsewith pre-existing supply, mint amount 0, zero-weight recipients.unfreeze — authorized/unauthorized actors,
IdentityTokenAccountNotFrozenErroron never-frozen identity, already-unfrozen rejection, multi-sig group flows (single-member sufficient, two-member with/without history), verifying unfrozen state truly allows subsequent transfers, public-note attachment.burn — supply-update read-back assertion (was only balance-side before), public note happy path + too-large rejection, zero-amount
InvalidTokenAmountError, explicitContractOwner/Identity(_)/NoOne/MainGroupauthorization rules,Identity(other)rejecting the owner, sequential balance+supply depletion, multi-holder supply aggregation, group action error branches (AlreadySignedByIdentity,AlreadyCompleted,ModificationOfGroupActionMainParameters,IdentityNotMemberOfGroupon propose/confirm,TokenNoteOnlyAllowedWhenProposer).system —
add_to_token_total_supply_v0replace vs first-insert,allow_first_mint+amount > i64::MAXerror, non-existent-token-without-first-mintCriticalCorruptedState, saturation toi64::MAX, non-saturating overflow;remove_from_token_total_supply_v0normal + exact-zero +checked_subunderflow + missing-supplyCorruptedDriveState;fetch_token_total_supply_v0None/Some + with_cost fee path;fetch_token_total_aggregated_identity_balances_v0one/multi/zero-holders;create_token_trees_v0creation +allow_already_existsduplicate error + idempotent no-op +start_as_pausedflag.info —
fetch_identity_token_info_v0Some/None/PathKeyNotFound→None coercion +apply=truestateful path +_with_costs;fetch_identity_token_infos_v0andfetch_identities_token_infos_v0partial-hit map branches + empty-list edge case; all 8 public query constructors inqueries.rscovering all four ascending/descending × inclusive/exclusive range branches plusstart_at = Nonefull-range branches.How Has This Been Tested?
cargo test -p drive --lib 'drive::tokens::'— 96 passedcargo test -p drive-abci --lib 'token_burn_tests'— 27 passedcargo test -p drive-abci --lib 'token_unfreeze_tests'— 11 passedcargo check --tests -p driveandcargo check --tests -p drive-abci— clean (no new warnings)cargo fmt -p drive -p drive-abci— cleanNo production bugs were surfaced during the coverage work. One note: the
allow_first_mintinsert-only branch inadd_to_token_total_supply_v0is technically only reachable when the supply entry is missing — in the normal flowcreate_token_trees_v0seeds it asSumItem(0), so code paths that run the documented setup sequence always take the replace branch. The testshould_error_when_first_mint_amount_exceeds_i64_maxreaches the insert branch by skippingcreate_token_trees.Breaking Changes
None. Tests only.
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit