fix(drive)!: mint base supply of tokens added by contract update - #4835
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
✨ 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 in progress — actively reviewing now (commit d18e725) · triage: normal |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the supplied finding against exact head 2d5b104: the protocol changelog omits the newly introduced base-supply minting behavior. The change remains gated to protocol version 14, so this is a documentation suggestion rather than a blocking versioning defect. Diff whitespace checks passed; runtime tests were not independently rerun.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The production diff in update_contract_operations_v2 and mint_base_supply_of_added_token changes token supply and balances, but is a contained initialization fix with straightforward guards and batch-operation replacement, while most added lines are tests, so it does not meet the large-or-intricate requirement for critical. - Phase 1 reviewers: not run (skipped for throughput: 16 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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/src/drive/contract/update/update_contract/v2/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/contract/update/update_contract/v2/mod.rs:215-217: Record the minting change in the protocol-version changelog
This comment describes the new minting behavior, but the release changelog in `packages/rs-platform-version/src/version/v14.rs` does not record it, and `DRIVE_CONTRACT_METHOD_VERSIONS_V4` still describes update_contract v2 only as creating distribution storage. Both `book/src/versioning/platform-version.md` and `book/src/versioning/versioned-dispatch.md` explicitly require documenting consensus changes in the snapshot's numbered doc comment. Add an entry explaining that protocol 14 credits newly added tokens' base supply to the configured destination or contract owner, initializes their total supply accordingly, and deliberately does not mint retroactively for tokens added under earlier protocols; extend the method-table documentation to match. The existing dispatch is correctly isolated to protocol 14 and needs no additional version bump.
PR HygieneState: waiting-bots · commit
Self-review is an author attestation that you have read the diff: This report does not bypass CI or repository protection rules. |
PR HygieneState: waiting-bots · commit
Self-review is an author attestation that you have read the diff: This report does not bypass CI or repository protection rules. |
`insert_contract` v1 credits a token's base supply to its `new_tokens_destination_identity` (the contract owner when unset) and starts the total supply at that amount. The update path only called `create_token_trees_operations`, which starts the supply at zero and writes no balance, although `validate_update_tokens` lets an update add tokens. A token added by update with a base supply therefore executed as a paid success and came into existence with nothing minted. `update_contract` v2, selected by protocol version 14 only and still unreleased, now mints the base supply of tokens absent from the original contract. The zero total supply insert queued by `create_token_trees_operations` is replaced instead of followed by a second operation on the same path and key, which the batch consistency check rejects. Tokens the contract already had are left alone, so a token config update and later contract updates never mint again, and an update can not remove a token, so a token is new exactly once. A base supply over `i64::MAX` is refused as in the insert. The update's basic structure validation already rejects it as a consensus error, so the internal error is not reachable from a state transition. Tokens added by update before protocol version 14 are not minted retroactively. The claim tests of tokens added by update now use a token without base supply, so the asserted balance is what the claim paid on every protocol version. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2d5b104 to
d18e725
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4835 +/- ##
=============================================
- Coverage 88.02% 76.32% -11.71%
=============================================
Files 2977 2981 +4
Lines 388320 438879 +50559
=============================================
- Hits 341836 334956 -6880
- Misses 46484 103923 +57439
🚀 New features to boost your workflow:
|
Conflict in the data contract update tests: this branch and #4837 both appended a helper and its tests at the end of `token_tests`. Both blocks are kept whole, #4837's first. The protocol version 14 changelog merged cleanly but with two entries numbered 13, since #4837 added 13 and 14. The entry for tokens added by a contract update becomes 15. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e entry Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
The base supply of a token added through a data contract update is never minted.
insert_contractv1 creditsbase_supplyto the token'snew_tokens_destination_identity(the contract owner when unset) and starts the total supply at that amount. The update path only callscreate_token_trees_operations, which starts the total supply atSumItem(0)and writes no balance, and nothing else in the update flow mints.validate_update_tokensexplicitly allows an update to add tokens, so the transition executes as a paid success and the token comes into existence with nothing minted.Confirmed with failing tests before the fix, at both levels. After an update adding a token with
base_supply = 1_000_000:The existing
test_data_contract_update_can_add_new_tokenadds exactly such a token but never looked at the balance or the supply. #4834 already listed this as a follow-up.For a token created from the
default_most_restrictivepreset (manual minting by no one), this is permanent: the token can never have a supply.What was done?
update_contractv2 mints the base supply of the tokens an update adds. v2 is selected by protocol version 14 only (DRIVE_CONTRACT_METHOD_VERSIONS_V4, amended in place by #4834) and has not shipped, so it is extended in place. v0 and v1 are untouched, and no version table changes.original_contract.tokens()are minted, the same gate fix(drive)!: create distribution trees for tokens added by contract update #4834 uses for distribution storage. A token config update reaches this method with its token present in the original, andvalidate_update_tokensrejects removing a token, so a token is new exactly once and nothing can mint twice.new_tokens_destination_identity, falling back to the contract owner, as in the insert.create_token_trees_operationsalready queues the insert of a zero total supply, and the batch consistency check rejects two operations on one path and key (insertion order error). So the queued insert is replaced with one carrying the base supply, and the balance insert is appended. The existingadd_to_token_total_supply/add_to_identity_token_balancehelpers do not fit: they read the current values from state, where the token's trees do not exist until the batch applies.CorruptedDriveStateinstead of overwriting a live supply.base_supply > i64::MAXreturns the sameCriticalCorruptedCreditsCodeExecutionas the insert. On the update transition this would be anInternalError(dropped from the proposer's block, a block containing one is rejected, no halt), but it is not reachable: the update's basic structure validation already rejects such a base supply with the consensusInvalidTokenBaseSupplyError, covered by the existingtest_data_contract_update_can_not_add_new_token_with_large_base_supply.apply_contracttakes the insert branch), so no estimation layer info is needed. The updater pays for the one extra insert.Test adjustment in #4834's claim tests. Their token kept the preset's default base supply of 100 000, which used to vanish. With the fix the owner held
100200and100445instead of200and445. The helper now sets the base supply to 0, so those tests measure the claim alone and expect the same value on every protocol version.Tokens already added by update: nothing minted retroactively
This PR does not mint retroactively. A token added by update under protocol version 13 or earlier is present in the original contract of every later update, so v2 never touches it.
should_not_mint_base_supply_of_token_added_by_update_before_protocol_version_14pins that: added under 13, updated again under 14, still no balance and a total supply of 0. It is the test to flip if one of the retroactive options below is chosen.Mainnet has nothing to mint. The scan done for #4834 at block 436796 answers this as well: all 20 data contract updates ever broadcast (12 succeeded) decode to a contract without tokens, and an update that adds a token has to carry it, so no mainnet token was ever added by an update. Testnet and devnets were not checked, and the window stays open until protocol version 14 activates, so it is worth one more look right before the mainnet upgrade.
Why a backfill is hard here: a missing tree is visible in state, an unminted base supply is not. (#4834 shipped without a backfill: on mainnet no contract update ever added a token, checked at block 436796, so option 1 costs nothing there.)
total supply == 0withbase_supply > 0also describes a token registered with its base supply and burned since.max_supply.new_tokens_destination_identitymay have changed since the token was added, so "who should have received it" is a judgement call too.create_token_trees_operationsalways writes a token status item, the insert only whenstart_as_paused. An unpaused token with a status item was added by update, or has since had an emergency action.Options:
(token_id, identity, amount)entries intransition_to_version_14. Deterministic and auditable, handles the "owner already compensated" cases by leaving them off the list. Costs a scan per network and a hardcoded table.transition_to_version_14: mint wherebase_supply > 0, total supply is 0 and no balances exist. No scan needed, but it cannot tell an unminted token from a fully burned one and would resurrect the latter. I would not do this one.With nothing to repair on mainnet, option 1 matches what #4834 settled on. It stays a monetary decision: if an affected token turns up on a network that matters before the upgrade, option 2 is the one to take, as a small follow-up independent of this PR.
Notes on the merged base
v2 now delegates to v1 and adds to its operations (as merged in #4834), so the mint replaces the zero total-supply insert inside the v1 operations. The protocol version 14 changelog (
v14.rs, entry 15) and theDRIVE_CONTRACT_METHOD_VERSIONS_V4docs record the change.How Has This Been Tested?
Red first: the minting tests below failed with the output quoted above before the fix.
cargo test -p drive --lib -- drive::contract::update drive::contract::insert drive::tokens::system::create_token_trees(243 passed). New inupdate_contract::v2::tests:should_mint_base_supply_to_contract_owner_for_token_added_by_update(also checks the token's balances sum to its total supply)should_mint_base_supply_to_new_tokens_destination_identity_for_token_added_by_update(and the owner gets nothing)should_mint_base_supply_of_every_token_added_by_one_update(two tokens in one batch)should_mint_base_supply_only_in_the_update_that_adds_the_token(a registered token and an added token, then a second update: neither is minted again)should_leave_token_without_base_supply_added_by_update_at_zero_supplyshould_not_mint_base_supply_of_token_added_by_update_on_protocol_version_13(the frozen side, through the same dispatcher)should_not_mint_base_supply_of_token_added_by_update_before_protocol_version_14(nothing retroactive)should_refuse_to_mint_an_unstorable_base_supply_or_over_an_existing_total_supplycargo test -p drive-abci --lib -- data_contract_update token::config_update protocol_upgrade(124 passed). New, throughprocess_raw_state_transitions:should_mint_base_supply_of_token_added_by_updateshould_not_mint_base_supply_of_token_added_by_update_on_protocol_version_13cargo clippy -p drive -p drive-abci --all-features --all-targets -- -D warningscargo fmt --allFull suites were not run locally. Strategy tests never add tokens through an update, so no pinned state hash moves.
Breaking Changes
Consensus change, gated on protocol version 14: a data contract update that adds a token with a non-zero base supply now writes the destination's balance and a non-zero total supply, where protocol version 13 writes a zero total supply and no balance. Protocol version 13 and earlier replay unchanged (
update_contractv0 and v1 are untouched).Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code