Uh oh!
There was an error while loading. Please reload this page.
test(drive-abci): add token supply edge-case coverage - #3849
Conversation
Add ABCI state-transition tests for token supply edge cases that were previously untested: exact max_supply boundary (mint to exactly the cap and one over), unbounded minting when max_supply is None, minting from a zero base_supply, burning below base_supply, burning the entire supply then minting again, and destroy-frozen-funds reducing total supply to zero and by the destroyed amount. Add config-update coverage for setting max_supply equal to the current supply and for the raise-max-then-mint-into-headroom lifecycle, with full TokenMintPastMaxSupplyError payload assertions. Document two real gaps via #[ignore]d tests asserting intended behavior: base_supply > max_supply is accepted at contract creation (no creation-time guard; the token is born over its cap and base_supply is immutable, so every mint is then blocked), and minting past i64::MAX on a max_supply=None token surfaces as an InternalError from the Drive checked_add guard rather than a graceful consensus rejection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rename the i64::MAX mint-overflow test to advertise that it characterizes current behavior (returns InternalError without mutating supply) rather than asserting a desired long-term API. Add a CHARACTERIZATION TEST note explaining that when a validation-layer guard for max_supply=None is added, this test is expected to break as the signal to update it to the graceful-rejection behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds multiple new token tests: minting boundary cases, burning invariants, config-update scenarios, frozen-funds destruction tests, and an ignored data-contract creation regression. No production code changes. ChangesToken Operations Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
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 docstrings
🧪 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 |
✅ Final review complete — no blockers (commit 4496d96) |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@## v4.1-dev #3849 +/- ##
=============================================
+ Coverage 65.42% 87.50% +22.08%
=============================================
Files 26 2667 +2641 Lines 2707 337434 +334727 =============================================
+ Hits 1771 295285 +293514 - Misses 936 42149 +41213
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Tests-only PR adding well-targeted token supply edge-case coverage and two characterization tests for documented gaps (#3848, #3850). Verified the new tests against the production code: assertions match current behavior and the helpers mirror the established patterns. One in-scope suggestion: the ignored regression test for base_supply > max_supply should narrow its assertion to UnpaidConsensusError(BasicError(_)) so it actually enforces the intended validation stage when the fix lands.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. 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/data_contract_create/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/mod.rs:1764-1768: Ignored regression test accepts too broad a class of errors
`test_data_contract_creation_with_base_supply_over_max_supply_should_cause_error` is the regression target for a future creation-time guard, but the assertion accepts either `UnpaidConsensusError` or any `PaidConsensusError`. A `base_supply > max_supply` violation is a contract-shape problem that the future guard should reject at basic structure validation (alongside the existing `base_supply > i64::MAX` check noted in the comment), which produces `UnpaidConsensusError(BasicError(_))`. As written, this test would still pass even if the fix only rejected the contract later in the paid execution path, defeating the regression intent. The `total_supply == None` assertion below proves the token was not created but not that rejection happened in the right stage.
Uh oh!
There was an error while loading. Please reload this page.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Tests-only PR adding token supply edge-case coverage in drive-abci. The single prior finding from 8c1fedf (overly broad assertion in the ignored regression test) is FIXED at 6354960 — the assertion now requires UnpaidConsensusError(ConsensusError::BasicError(_)), locking the future fix to basic-structure validation. No new in-scope issues; documented production gaps (#3848, #3850) are explicitly tracked as out-of-scope follow-ups.
Issue being fixed or feature implemented
Token supply handling lacked coverage for several edge cases (exact
max_supplyboundary, burning belowbase_supply, burning to zero, destroy-frozen-funds supply accounting, zero/i64::MAXbase_supply, etc.). While adding that coverage, two real gaps surfaced:base_supply > max_supplyis accepted at contract creation. There is no creation-time guard comparing the two (onlybase_supply > i64::MAXis rejected). The token is born already over its cap,base_supplyis immutable, so every subsequent mint is permanently blocked byTokenMintPastMaxSupplyError. Tracked in base_supply > max_supply is accepted at token contract creation, permanently blocking mints #3850i64::MAXon a token withmax_supply == Nonereturns anInternalErrorinstead of a graceful consensus error. The mint validation's supply check is wrapped inif let Some(max_supply) = ..., so the uncapped case has no validation-layer guard and is only caught by the low-level Drivechecked_add. Tracked in Minting past i64::MAX on an uncapped token returns InternalError instead of a consensus error #3848.This PR is tests only — no production behavior changes. The two gaps are documented (see below) for follow-up fixes.
What was done?
Added ABCI state-transition tests under
.../batch/tests/token/:max_supply(succeeds), one over (fails, fullTokenMintPastMaxSupplyErrorpayload asserted), unbounded mint whenmax_supply == None, mint from zerobase_supply, mint from zero to exact max.base_supply(allowed by design), burn entire supply to zero then mint again.max_supplyequal to current supply (boundary, succeeds), raisemax_supplythen mint into the new headroom.Documenting the two gaps:
test_data_contract_creation_with_base_supply_over_max_supply_should_cause_error(indata_contract_create) — runs a realDataContractCreateTransitionand asserts the intended rejection.#[ignore]d because it fails on current code; it is the regression target for adding the creation-time guard.test_token_mint_with_max_i64_base_supply_then_overflow_returns_internal_error_without_mutating_supply— a characterization test pinning the current
InternalError-without-mutation behavior (gap feat: enable mainnet for dashmate #2). Named to make clear it is not the desired long-term API; it should break (intentionally) when the validation guard is added.How Has This Been Tested?
cargo test -p drive-abcifor thetokenanddata_contract_create::tests::tokensmodules: all executable tests pass (200 + 21), with exactly one#[ignore]d test (the creation-guard regression target).--ignored(proving the gap is real and the assertion non-vacuous).cargo fmt -p drive-abciclean.Breaking Changes
None — tests only.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
max_supply = None,base_supplyedge cases, and overflow handling.