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 |
|
✅ Review complete (commit 2c653ce) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3849 +/- ##
=========================================
Coverage 70.73% 70.73%
=========================================
Files 20 20
Lines 2788 2788
=========================================
Hits 1972 1972
Misses 816 816
🚀 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.
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