fix(dpp): bind token config update action_id to payload value (v1)#3346
Merged
QuantumExplorer merged 13 commits intoMar 17, 2026
Conversation
The v0 action_id calculation for token config updates used only the u8 enum discriminant (via u8_item_index()), not the actual payload value. This allowed a group member to propose one config value, collect votes, then swap to a different value with the same discriminant during finalization (e.g. MaxSupply(100) vs MaxSupply(999999999999) both hashed identically). Introduce a versioned action_id calculation: - v0 (unchanged): hashes only the u8 discriminant for backward compat - v1 (new): serializes the full TokenConfigurationChangeItem via bincode and includes it in the double-hash, binding the voted-on value The version is controlled by a new platform version field (token_config_update_action_id_version) so v1 activates in a future protocol version without breaking existing production data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements version-aware token config update action ID calculation across multiple token transition types. The trait method Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant TokenTransition
participant TokenConfigUpdate
participant PlatformVersion
participant ActionIDCalculator
Client->>TokenTransition: new_token_config_update_transition(platform_version)
TokenTransition->>PlatformVersion: check token_config_update_action_id_version
alt version = 1
TokenTransition->>TokenConfigUpdate: calculate_action_id_with_fields_v1(payload)
TokenConfigUpdate->>ActionIDCalculator: hash with payload
ActionIDCalculator-->>TokenConfigUpdate: identifier_v1
else version = 0
TokenTransition->>TokenConfigUpdate: calculate_action_id_with_fields_v0()
TokenConfigUpdate->>ActionIDCalculator: hash without payload
ActionIDCalculator-->>TokenConfigUpdate: identifier_v0
end
TokenConfigUpdate-->>TokenTransition: Result<Identifier>
TokenTransition-->>Client: transition with action_id or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3346 +/- ##
============================================
+ Coverage 74.90% 75.22% +0.32%
============================================
Files 3000 3052 +52
Lines 271665 283091 +11426
============================================
+ Hits 203478 212952 +9474
- Misses 68187 70139 +1952
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs (1)
308-328: Reduce duplicate action-id routing to avoid branch drift.Line 313–327 duplicates
calculate_action_idlogic almost entirely. Consider delegating non-ConfigUpdatecases toself.calculate_action_id(owner_id).♻️ Suggested simplification
fn calculate_action_id_versioned( &self, owner_id: Identifier, platform_version: &PlatformVersion, ) -> Option<Identifier> { - match self { - TokenTransition::Burn(t) => Some(t.calculate_action_id(owner_id)), - TokenTransition::Mint(t) => Some(t.calculate_action_id(owner_id)), - TokenTransition::Freeze(t) => Some(t.calculate_action_id(owner_id)), - TokenTransition::Unfreeze(t) => Some(t.calculate_action_id(owner_id)), - TokenTransition::Transfer(_) => None, - TokenTransition::DestroyFrozenFunds(t) => Some(t.calculate_action_id(owner_id)), - TokenTransition::Claim(_) => None, - TokenTransition::EmergencyAction(t) => Some(t.calculate_action_id(owner_id)), - TokenTransition::ConfigUpdate(t) => { - Some(t.calculate_action_id_versioned(owner_id, platform_version)) - } - TokenTransition::DirectPurchase(_) => None, - TokenTransition::SetPriceForDirectPurchase(t) => Some(t.calculate_action_id(owner_id)), - } + match self { + TokenTransition::ConfigUpdate(t) => { + Some(t.calculate_action_id_versioned(owner_id, platform_version)) + } + _ => self.calculate_action_id(owner_id), + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs` around lines 308 - 328, The match in calculate_action_id_versioned duplicates the per-variant call to calculate_action_id for almost every TokenTransition; refactor by delegating all non-ConfigUpdate variants to self.calculate_action_id(owner_id) and only special-case TokenTransition::ConfigUpdate to call t.calculate_action_id_versioned(owner_id, platform_version); update calculate_action_id_versioned to return self.calculate_action_id(owner_id) wrapped in Some/None as appropriate for the default path and keep TokenTransition::ConfigUpdate using calculate_action_id_versioned so you avoid branch drift and duplicated per-variant logic.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs (1)
256-272: Test may need updating when v1 is activated.This test uses
PlatformVersion::latest()and asserts that versioned and plain methods produce the same ID. When a future platform version activates v1 (viatoken_config_update_action_id_version = 1), this test will fail. Consider adding a comment noting this expected behavior change, or using an explicit platform version constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs` around lines 256 - 272, The test versioned_dispatch_uses_v0_on_current_platform_version assumes PlatformVersion::latest() maps to v0; update the test to avoid future breakage by either using an explicit platform version constant (e.g., construct a PlatformVersion with token_config_update_action_id_version = 0) or add a clarifying comment explaining this will fail once token_config_update_action_id_version becomes 1; locate the test and change the PlatformVersion usage around the calls to calculate_action_id and calculate_action_id_versioned (or add the comment near PlatformVersion::latest() and references to calculate_action_id/calculate_action_id_versioned) so intent is clear and the test won’t unexpectedly break when v1 is activated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs`:
- Around line 256-272: The test
versioned_dispatch_uses_v0_on_current_platform_version assumes
PlatformVersion::latest() maps to v0; update the test to avoid future breakage
by either using an explicit platform version constant (e.g., construct a
PlatformVersion with token_config_update_action_id_version = 0) or add a
clarifying comment explaining this will fail once
token_config_update_action_id_version becomes 1; locate the test and change the
PlatformVersion usage around the calls to calculate_action_id and
calculate_action_id_versioned (or add the comment near PlatformVersion::latest()
and references to calculate_action_id/calculate_action_id_versioned) so intent
is clear and the test won’t unexpectedly break when v1 is activated.
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs`:
- Around line 308-328: The match in calculate_action_id_versioned duplicates the
per-variant call to calculate_action_id for almost every TokenTransition;
refactor by delegating all non-ConfigUpdate variants to
self.calculate_action_id(owner_id) and only special-case
TokenTransition::ConfigUpdate to call t.calculate_action_id_versioned(owner_id,
platform_version); update calculate_action_id_versioned to return
self.calculate_action_id(owner_id) wrapped in Some/None as appropriate for the
default path and keep TokenTransition::ConfigUpdate using
calculate_action_id_versioned so you avoid branch drift and duplicated
per-variant logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71d7c64c-ed81-4f4d-b1ff-a7f6d908e8ad
📒 Files selected for processing (9)
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/multi_party_action.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v1/v1_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/validation/validate_basic_structure/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/v1.rspackages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/v2.rs
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Well-structured security fix for a real vote-swap vulnerability in token config update action_id calculation. The versioning approach correctly follows the project's established pattern — staging the fix as dormant code to be activated in a future protocol version. The implementation is clean and the test suite thoroughly documents both the vulnerability and the fix. Two architectural observations worth discussing.
Reviewed commit: 47acf03
🟡 2 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-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0/v0_methods.rs`:
- [SUGGESTION] line 120: panic! on unknown version — consider returning an error
The fallback arm `version => panic!("unsupported token_config_update_action_id_version {version}")` will crash the node on an unexpected version. The rest of the codebase uses `Err(Error::UnknownVersionMismatch { ... })` for this pattern. The root cause is that the `AllowedAsMultiPartyAction` trait returns `Identifier` rather than `Result<Identifier, ProtocolError>`, so proper error propagation would require a trait signature change. If changing the trait is out of scope for this fix, documenting this as a known limitation would help. In practice the risk is low since version values come from hardcoded `PlatformVersion` structs, but a panic in a consensus node is a harsher failure mode than a rejected transition.
In `packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs`:
- [SUGGESTION] lines 117-119: v1 hash uses derived bincode serialization — consider explicit encoding for consensus stability
`calculate_action_id_with_fields_v1()` hashes `bincode::encode_to_vec(item, bincode::config::standard())` where `TokenConfigurationChangeItem` derives `Encode`. The serialized discriminant comes from enum declaration order, which is a weaker consensus contract than the explicitly pinned `u8_item_index()` mapping used in v0. If a future change inserts or reorders enum variants without bumping `token_config_update_action_id_version`, nodes on the old and new code would compute different action_ids for the same logical config update, causing a consensus split. A safer approach for the v1 hash would be: `u8_item_index()` (pinned discriminant) plus an explicit encoding of the variant's payload, keeping the stable tag while still binding the full value.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-platform-version/src/version/v12.rs`:
- Line 13: PLATFORM_V12 is currently bound to TOKEN_VERSIONS_V2 which enables
new action_id behavior; revert this binding so v12 preserves prior behavior by
changing the binding of PLATFORM_V12 to the older token-versions constant
(replace TOKEN_VERSIONS_V2 with TOKEN_VERSIONS_V1) in v12.rs, ensuring
PLATFORM_V12 references the legacy token versions instead of the v2 token
versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d4ff501-8010-4c2b-beac-31ad1a92254a
📒 Files selected for processing (1)
packages/rs-platform-version/src/version/v12.rs
The test assumed calculate_action_id_versioned should match calculate_action_id (v0) on the current platform version. But v12 uses TOKEN_VERSIONS_V2 which sets token_config_update_action_id_version to 1, so the versioned call correctly uses v1 (full payload hash) while the plain call uses v0 (discriminant only). Updated the test to assert they differ and verify the versioned result matches calculate_action_id_with_fields_v1 directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean incremental push activating the action_id fix in v12 (TOKEN_VERSIONS_V1 → TOKEN_VERSIONS_V2). The test correctly verifies v12 now dispatches to v1, confirming the fix is no longer dormant. Prior suggestions (panic! fallback, bincode consensus stability) remain applicable.
Reviewed commit: ebcb500
…sult Merge calculate_action_id and calculate_action_id_versioned into a single calculate_action_id(owner_id, platform_version) -> Result method. This removes the separate versioned dispatch and makes platform_version a required parameter for all action_id calculations. Changes: - AllowedAsMultiPartyAction::calculate_action_id now takes platform_version and returns Result<Identifier, ProtocolError> - Remove calculate_action_id_versioned (merged into calculate_action_id) - Rename calculate_action_id_with_fields to calculate_action_id_with_fields_v0 - Replace panic! with ProtocolError::UnknownVersionMismatch - Update all 7 token transition impls and all callers - Update TokenTransitionV0Methods::calculate_action_id signature Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…at call site Revert AllowedAsMultiPartyAction trait back to the simple signature: fn calculate_action_id(&self, owner_id: Identifier) -> Identifier The v0/v1 version dispatch for TokenConfigUpdateTransition now happens at the call site (v1_methods.rs) rather than inside the trait impl. Key changes: - Trait has no platform_version parameter, no Result return - All 7 non-config-update transitions use simple calculate_action_id - calculate_action_id_with_fields_v1 takes (u8, Option<&[u8]>) instead of &TokenConfigurationChangeItem — callers serialize and pass payload - Added calculate_action_id_with_platform_version on TokenTransition for use in validation where version-aware calculation is needed - Version dispatch in v1_methods.rs ConfigUpdate arm only Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ispatch at call site" This reverts commit fb77572. Restoring the unified calculate_action_id with platform_version and Result return type as designed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… Option payload Change v1 signature from &TokenConfigurationChangeItem to (u8 discriminant, Option<&[u8]> payload). Serialization of the config item now happens at the call site, keeping the hash function generic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0_methods.rs (1)
76-95:⚠️ Potential issue | 🔴 CriticalHash the full direct-purchase schedule.
calculate_action_id_with_fields_v0()only appends.1fromminimum_purchase_amount_and_price(), so proposals that share a price but differ in minimum purchase amount collapse to the sameaction_id. That reintroduces the same vote-swap class this PR is fixing for token config updates. Please add a versioned path that binds the fullTokenPricingSchedulepayload and route multi-party direct-purchase action ID calculation through it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0_methods.rs` around lines 76 - 95, calculate_action_id_with_fields_v0 currently only appends the price (.1 of minimum_purchase_amount_and_price()) which collapses distinct schedules; modify calculate_action_id_with_fields_v0 to include the full TokenPricingSchedule payload when present (not just the price) by serializing the entire schedule (use the existing TokenPricingSchedule representation/serialization used elsewhere) and appending those bytes into the bytes buffer before hashing with hash_double; add a versioned path/marker string (e.g., include a version suffix in the initial bytes like "action_token_set_price_for_direct_purchase_v1") so future changes are versioned, and ensure callers that compute multi-party direct-purchase action IDs route through this updated function so the full schedule differentiates action_id values.
🧹 Nitpick comments (3)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/mint/mod.rs (1)
1605-1610: Centralize mintaction_idderivation instead of repeating raw_v0calls.These eight call sites duplicate the mint hash inputs and pin the suite to the v0 helper. Where the proposer transition already exists, prefer its
calculate_action_id(..., platform_version); for the “other signer goes first” case, a small local helper would keep the raw field list in one place.Also applies to: 1871-1876, 2194-2199, 2414-2419, 2633-2638, 2922-2927, 3338-3343, 3475-3480
🤖 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/mint/mod.rs` around lines 1605 - 1610, The tests duplicate mint action_id derivation by calling TokenMintTransition::calculate_action_id_with_fields_v0 directly; replace those raw _v0 calls with the transition's stable API and centralize the field list: use TokenMintTransition::calculate_action_id(..., platform_version) where the proposer transition is available, and for the "other signer goes first" scenario extract a small local helper in the test (e.g., local fn derive_mint_action_id(token_id, identity, quantity, nonce, platform_version)) that calls the single derive implementation so the four input fields are defined in one place and the suite is no longer pinned to the v0 helper. Ensure all eight sites mentioned (including the given line and the other ranges) are updated to call the centralized helper or calculate_action_id with platform_version.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/freeze/mod.rs (1)
1434-1439: Prefer the transition’scalculate_action_idin these tests.These cases already have the proposer transition in scope, but they still hard-code the
_v0helper while running againstPlatformVersion::latest(). Calling the versioned API here would keep the tests aligned with runtime dispatch and remove the duplicated nonce/target literals.Also applies to: 1778-1783, 2226-2231
🤖 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/freeze/mod.rs` around lines 1434 - 1439, Tests are calling the hardcoded versioned helper TokenFreezeTransition::calculate_action_id_with_fields_v0; replace those calls with the transition's non-versioned calculate_action_id API (e.g., call the proposer transition instance's calculate_action_id with the same fields/nonce/target) so the tests follow runtime dispatch via PlatformVersion::latest(); update all occurrences of calculate_action_id_with_fields_v0 (including the other similar sites flagged) to use the transition.calculate_action_id entrypoint instead.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/config_update/mod.rs (1)
1081-1086: Avoid pinning latest-version tests to the v0 hash.These cases use
PlatformVersion::latest()but still derive the confirmation ID viacalculate_action_id_with_fields_v0(...). When the latest platform version switches token-config action IDs to the payload-binding variant, these confirmations will reference the wrongaction_idand the tests will start failing for the wrong reason. Please compute the ID through the versioned transition API, or pin these tests to an explicitly v0 platform version.Also applies to: 1296-1304, 1464-1469, 1687-1695, 1912-1920, 2149-2157, 2436-2444, 2580-2588
🤖 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/config_update/mod.rs` around lines 1081 - 1086, The tests call TokenConfigUpdateTransition::calculate_action_id_with_fields_v0(...) while using PlatformVersion::latest(), which will break when the canonical action-id format changes; update each test (e.g. the call at calculate_action_id_with_fields_v0 and similar sites) to obtain the action id via the versioned transition API for the current platform version (e.g. use TokenConfigUpdateTransition::calculate_action_id_with_fields(platform_version, ...) or the equivalent version-dispatch helper) or else explicitly construct a PlatformVersion pinned to v0 and pass it into the versioned API so the confirmation ID matches v0; update all occurrences noted in the comment (lines around 1081, 1296-1304, 1464-1469, 1687-1695, 1912-1920, 2149-2157, 2436-2444, 2580-2588) accordingly.
🤖 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-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs`:
- Around line 248-258: The test function
versioned_dispatch_uses_v1_on_current_platform_version currently calls
PlatformVersion::latest(), which will drift as new protocol versions are added;
replace that call with PlatformVersion::get(2).expect("expected version 2") so
the test is pinned to the protocol version where
token_config_update_action_id_version becomes 1—update the PlatformVersion value
used when calling t.calculate_action_id(owner_id, platform_version) and keep the
rest of the test (TokenConfigurationChangeItem, calculate_action_id) unchanged.
---
Outside diff comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0_methods.rs`:
- Around line 76-95: calculate_action_id_with_fields_v0 currently only appends
the price (.1 of minimum_purchase_amount_and_price()) which collapses distinct
schedules; modify calculate_action_id_with_fields_v0 to include the full
TokenPricingSchedule payload when present (not just the price) by serializing
the entire schedule (use the existing TokenPricingSchedule
representation/serialization used elsewhere) and appending those bytes into the
bytes buffer before hashing with hash_double; add a versioned path/marker string
(e.g., include a version suffix in the initial bytes like
"action_token_set_price_for_direct_purchase_v1") so future changes are
versioned, and ensure callers that compute multi-party direct-purchase action
IDs route through this updated function so the full schedule differentiates
action_id values.
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/config_update/mod.rs`:
- Around line 1081-1086: The tests call
TokenConfigUpdateTransition::calculate_action_id_with_fields_v0(...) while using
PlatformVersion::latest(), which will break when the canonical action-id format
changes; update each test (e.g. the call at calculate_action_id_with_fields_v0
and similar sites) to obtain the action id via the versioned transition API for
the current platform version (e.g. use
TokenConfigUpdateTransition::calculate_action_id_with_fields(platform_version,
...) or the equivalent version-dispatch helper) or else explicitly construct a
PlatformVersion pinned to v0 and pass it into the versioned API so the
confirmation ID matches v0; update all occurrences noted in the comment (lines
around 1081, 1296-1304, 1464-1469, 1687-1695, 1912-1920, 2149-2157, 2436-2444,
2580-2588) accordingly.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/freeze/mod.rs`:
- Around line 1434-1439: Tests are calling the hardcoded versioned helper
TokenFreezeTransition::calculate_action_id_with_fields_v0; replace those calls
with the transition's non-versioned calculate_action_id API (e.g., call the
proposer transition instance's calculate_action_id with the same
fields/nonce/target) so the tests follow runtime dispatch via
PlatformVersion::latest(); update all occurrences of
calculate_action_id_with_fields_v0 (including the other similar sites flagged)
to use the transition.calculate_action_id entrypoint instead.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/mint/mod.rs`:
- Around line 1605-1610: The tests duplicate mint action_id derivation by
calling TokenMintTransition::calculate_action_id_with_fields_v0 directly;
replace those raw _v0 calls with the transition's stable API and centralize the
field list: use TokenMintTransition::calculate_action_id(..., platform_version)
where the proposer transition is available, and for the "other signer goes
first" scenario extract a small local helper in the test (e.g., local fn
derive_mint_action_id(token_id, identity, quantity, nonce, platform_version))
that calls the single derive implementation so the four input fields are defined
in one place and the suite is no longer pinned to the v0 helper. Ensure all
eight sites mentioned (including the given line and the other ranges) are
updated to call the centralized helper or calculate_action_id with
platform_version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9523e435-d9f2-43c7-8af4-c32ff6cffc5e
📒 Files selected for processing (26)
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/multi_party_action.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_burn_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_burn_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_destroy_frozen_funds_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_destroy_frozen_funds_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_freeze_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_freeze_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v1/v1_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/validation/validate_basic_structure/v0/mod.rspackages/rs-drive-abci/src/execution/check_tx/v0/mod.rspackages/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/config_update/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/freeze/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/mint/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/transfer/mod.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs (1)
306-320:⚠️ Potential issue | 🟡 MinorPin this test to the activation protocol version instead of
PlatformVersion::latest().Line 313 currently uses
latest(), which can change over time and make this test nondeterministic as new protocol versions are introduced.Proposed patch
- let platform_version = PlatformVersion::latest(); + let platform_version = PlatformVersion::get(2).expect("expected version 2");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs` around lines 306 - 320, The test versioned_dispatch_uses_v1_on_current_platform_version is using PlatformVersion::latest(), making it non-deterministic; replace that call with the specific activation protocol PlatformVersion instance where token_config_update_action_id_version is 1 (i.e., pin to the protocol version that introduced v1 behavior) so calculate_action_id(owner_id, platform_version) uses a stable, known PlatformVersion rather than PlatformVersion::latest(); update the test to obtain that concrete PlatformVersion instead of calling latest().
🧹 Nitpick comments (5)
packages/rs-dpp/src/data_contract/change_control_rules/authorized_action_takers.rs (1)
61-75: Consider adding strict length validation for single-byte variants.The
IdentityandGroupvariants enforce exact byte lengths (33 and 3 respectively), butNoOne,ContractOwner, andMainGroupaccept any input with a matching tag byte, ignoring trailing bytes. For example,from_bytes(&[0, 0xFF, 0xFF])returnsOk(NoOne).For consistency and to avoid potential ambiguity in security-sensitive contexts (action_id calculation), consider validating exact lengths for all variants.
♻️ Proposed fix for consistent length validation
match tag { - 0 => Ok(AuthorizedActionTakers::NoOne), - 1 => Ok(AuthorizedActionTakers::ContractOwner), + 0 => { + if bytes.len() != 1 { + return Err(ProtocolError::DecodingError(format!( + "expected 1 byte for AuthorizedActionTakers::NoOne, got {}", + bytes.len() + ))); + } + Ok(AuthorizedActionTakers::NoOne) + } + 1 => { + if bytes.len() != 1 { + return Err(ProtocolError::DecodingError(format!( + "expected 1 byte for AuthorizedActionTakers::ContractOwner, got {}", + bytes.len() + ))); + } + Ok(AuthorizedActionTakers::ContractOwner) + } 2 => { if bytes.len() != 33 { return Err(ProtocolError::DecodingError(format!( "expected 33 bytes for AuthorizedActionTakers::Identity, got {}", bytes.len() ))); } let identifier = Identifier::from_bytes(&bytes[1..]) .map_err(|e| ProtocolError::DecodingError(e.to_string()))?; Ok(AuthorizedActionTakers::Identity(identifier)) } - 3 => Ok(AuthorizedActionTakers::MainGroup), + 3 => { + if bytes.len() != 1 { + return Err(ProtocolError::DecodingError(format!( + "expected 1 byte for AuthorizedActionTakers::MainGroup, got {}", + bytes.len() + ))); + } + Ok(AuthorizedActionTakers::MainGroup) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/change_control_rules/authorized_action_takers.rs` around lines 61 - 75, The deserializer currently accepts extra trailing bytes for single-byte variants (tags 0,1,3); update the from_bytes implementation (the match on tag that constructs AuthorizedActionTakers::NoOne, ::ContractOwner, ::MainGroup and the existing Identity branch) to enforce exact lengths: require bytes.len() == 1 for NoOne, ContractOwner and MainGroup (keep bytes.len()==33 for Identity and bytes.len()==3 for Group where applicable), and return a ProtocolError::DecodingError with a clear message when lengths mismatch so all variants are validated consistently against trailing data.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0/v0_methods.rs (1)
74-75: Clarify the doc comment to reflect version-dispatched behavior.The current wording reads like unconditional v0 hashing, but this method accepts
platform_versionand routes through version-aware calculation. A short note listing both v0/v1 behaviors would prevent confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0/v0_methods.rs` around lines 74 - 75, Update the doc comment for the function calculate_action_id to state that it is version-dispatched via the platform_version parameter and describe both behaviors: for v0 it uses only the u8 discriminant (backward-compatible hashing), while for v1 (and later) it performs the newer, full action-id hashing; mention the platform_version argument so readers know the method routes to the appropriate versioned implementation rather than always using v0 logic.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs (1)
168-170: Use the production payload serializer in tests to avoid encoding drift.The test helper re-encodes items directly with bincode. Reusing
payload_serialization()in test vectors would keep tests aligned with the production hashing path if serialization details evolve.Also applies to: 224-237, 255-268, 291-297, 331-337, 355-368
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs` around lines 168 - 170, The test helper function serialize_item currently encodes TokenConfigurationChangeItem directly with bincode; replace that direct bincode call with the production serializer payload_serialization() so test vectors use the same serialization path as production (i.e., call payload_serialization().serialize(item) or the equivalent API used elsewhere), and make the same replacement for the other test helper re-encodings in this file (the additional serialize_* helpers around the other item arrays) so all test encodings mirror the production hashing/serialization implementation.packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs (2)
100-123: Add explicit boundary tests for the option-backed variants.
MaxSupply,PerpetualDistribution,NewTokensDestinationIdentity, andMainControlGroupnow define consensus bytes here with differentNoneconventions. A small table-driven regression test aroundNonevsSome(...)for these four branches would directly protect the collision boundary this PR is closing before version 1 is enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs` around lines 100 - 123, Add table-driven unit tests for the TokenConfigurationChangeItem serialization branches to assert explicit boundary behavior for option-backed variants: MaxSupply, PerpetualDistribution, NewTokensDestinationIdentity, and MainControlGroup. For each variant (use TokenConfigurationChangeItem::MaxSupply, ::PerpetualDistribution, ::NewTokensDestinationIdentity, ::MainControlGroup) create test cases covering None and at least one Some(...) value, encode to consensus bytes using the same path exercised in the file (the code that maps to vec bytes / bincode encoding), and assert that None and Some(...) produce distinct, expected byte sequences and do not collide with each other or with empty vec conventions used in MainControlGroup. Place tests near existing token_configuration_item tests to protect the boundary behavior before v1.
69-70: Avoid modeling an empty hash payload asSome(vec![]).The caller in
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rson Lines 152-153 only appends bytes whenpayloadisSome, soNoneandSome(empty)already collapse to the sameaction_idinput. ReturningSome(vec![])forMainControlGroup(None)makes that equivalence easy to miss in a consensus path. Either normalize empty cases toNoneor make this API return raw bytes so the contract is explicit.Also applies to: 120-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs` around lines 69 - 70, The payload_serialization method currently returns Some(vec![]) for empty hash payloads (e.g., when matching MainControlGroup(None)), which conflates None and Some(empty) for callers like state_transitions::...::v0_methods.rs that only check for Some; update payload_serialization (and the similar match at the other site around lines 120-123) to normalize empty payloads to None instead of Some(empty) — i.e., when the computed Vec<u8> is empty return Ok(None), otherwise return Ok(Some(bytes)); reference the payload_serialization function and the MainControlGroup match arm to locate where to implement this normalization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs`:
- Around line 306-320: The test
versioned_dispatch_uses_v1_on_current_platform_version is using
PlatformVersion::latest(), making it non-deterministic; replace that call with
the specific activation protocol PlatformVersion instance where
token_config_update_action_id_version is 1 (i.e., pin to the protocol version
that introduced v1 behavior) so calculate_action_id(owner_id, platform_version)
uses a stable, known PlatformVersion rather than PlatformVersion::latest();
update the test to obtain that concrete PlatformVersion instead of calling
latest().
---
Nitpick comments:
In
`@packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs`:
- Around line 100-123: Add table-driven unit tests for the
TokenConfigurationChangeItem serialization branches to assert explicit boundary
behavior for option-backed variants: MaxSupply, PerpetualDistribution,
NewTokensDestinationIdentity, and MainControlGroup. For each variant (use
TokenConfigurationChangeItem::MaxSupply, ::PerpetualDistribution,
::NewTokensDestinationIdentity, ::MainControlGroup) create test cases covering
None and at least one Some(...) value, encode to consensus bytes using the same
path exercised in the file (the code that maps to vec bytes / bincode encoding),
and assert that None and Some(...) produce distinct, expected byte sequences and
do not collide with each other or with empty vec conventions used in
MainControlGroup. Place tests near existing token_configuration_item tests to
protect the boundary behavior before v1.
- Around line 69-70: The payload_serialization method currently returns
Some(vec![]) for empty hash payloads (e.g., when matching
MainControlGroup(None)), which conflates None and Some(empty) for callers like
state_transitions::...::v0_methods.rs that only check for Some; update
payload_serialization (and the similar match at the other site around lines
120-123) to normalize empty payloads to None instead of Some(empty) — i.e., when
the computed Vec<u8> is empty return Ok(None), otherwise return Ok(Some(bytes));
reference the payload_serialization function and the MainControlGroup match arm
to locate where to implement this normalization.
In
`@packages/rs-dpp/src/data_contract/change_control_rules/authorized_action_takers.rs`:
- Around line 61-75: The deserializer currently accepts extra trailing bytes for
single-byte variants (tags 0,1,3); update the from_bytes implementation (the
match on tag that constructs AuthorizedActionTakers::NoOne, ::ContractOwner,
::MainGroup and the existing Identity branch) to enforce exact lengths: require
bytes.len() == 1 for NoOne, ContractOwner and MainGroup (keep bytes.len()==33
for Identity and bytes.len()==3 for Group where applicable), and return a
ProtocolError::DecodingError with a clear message when lengths mismatch so all
variants are validated consistently against trailing data.
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs`:
- Around line 168-170: The test helper function serialize_item currently encodes
TokenConfigurationChangeItem directly with bincode; replace that direct bincode
call with the production serializer payload_serialization() so test vectors use
the same serialization path as production (i.e., call
payload_serialization().serialize(item) or the equivalent API used elsewhere),
and make the same replacement for the other test helper re-encodings in this
file (the additional serialize_* helpers around the other item arrays) so all
test encodings mirror the production hashing/serialization implementation.
In
`@packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0/v0_methods.rs`:
- Around line 74-75: Update the doc comment for the function calculate_action_id
to state that it is version-dispatched via the platform_version parameter and
describe both behaviors: for v0 it uses only the u8 discriminant
(backward-compatible hashing), while for v1 (and later) it performs the newer,
full action-id hashing; mention the platform_version argument so readers know
the method routes to the appropriate versioned implementation rather than always
using v0 logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03b70135-1b65-4ca4-b760-306f4c3e855a
📒 Files selected for processing (23)
packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rspackages/rs-dpp/src/data_contract/change_control_rules/authorized_action_takers.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/multi_party_action.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_burn_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_burn_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_destroy_frozen_funds_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_destroy_frozen_funds_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_freeze_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_freeze_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_mint_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/v0/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v1/v1_methods.rspackages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/validation/validate_basic_structure/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/config_update/mod.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_unfreeze_transition/v0_methods.rs
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/multi_party_action.rs
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_set_price_for_direct_purchase_transition/v0/v0_methods.rs
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_destroy_frozen_funds_transition/v0_methods.rs
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/v0/v0_methods.rs
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The vulnerability fix is correct and the versioning wiring is sound. TOKEN_VERSIONS_V1 uses action_id v0 (existing behavior), TOKEN_VERSIONS_V2 uses action_id v1 (payload-bound). The core fix works. However, 4 of 6 unit tests construct expected v1 hashes using bincode serialization of the whole enum rather than the production payload_serialization() method, which means regressions in that method would go undetected for most variants. One integration-style test (versioned_dispatch) does exercise the production path, partially mitigating this.
Reviewed commit: 6d290b1
🟡 2 suggestion(s) | 💬 3 nitpick(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-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/v0_methods.rs`:
- [SUGGESTION] lines 168-170: 4 of 6 v1 unit tests use different serialization than production code
`serialize_item()` calls `bincode::encode_to_vec(item, bincode::config::standard())` which serializes the entire `TokenConfigurationChangeItem` enum (including bincode's variant encoding). Production uses `payload_serialization()` which returns custom per-variant bytes — e.g. `MaxSupply(Some(100))` returns `100u64.to_be_bytes()` (8 bytes), not bincode encoding of the full enum.
The `versioned_dispatch_uses_v1_on_current_platform_version` test (line 306) does exercise the production path via `item.payload_serialization()` at line 336, so the dispatch integration is tested. But the 4 direct v1 tests (`v1_action_id_same_discriminant_different_values_produces_different_ids`, `v1_action_id_different_item_types_produces_different_ids`, `v0_and_v1_produce_different_ids_for_same_input`, `v1_action_id_identical_items_produces_same_id`) all use `serialize_item()` — they prove the v1 mechanism works but with non-production bytes.
- [SUGGESTION] lines 186-244: No test exercises payload_serialization() for AuthorizedActionTakers variants
20+ variants carry `AuthorizedActionTakers` payloads serialized via `to_bytes()` in production, but every v1 test only uses `MaxSupply` and `MintingAllowChoosingDestination`. A regression in `AuthorizedActionTakers::to_bytes()` or in how `payload_serialization()` handles those variants would not be caught. Consider adding at least one test with an `AuthorizedActionTakers`-based variant (e.g. `ManualMinting(AuthorizedActionTakers::Group(5))`) to verify `to_bytes()` is correctly integrated.
Comment on lines
+120
to
+123
| TokenConfigurationChangeItem::MainControlGroup(position) => match position { | ||
| Some(pos) => Some(pos.to_be_bytes().to_vec()), | ||
| None => Some(vec![]), | ||
| }, |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: MainControlGroup(None) returns Some(vec![]) while other Option::None variants return None
MaxSupply(None), NewTokensDestinationIdentity(None), and PerpetualDistribution(None) all return None from payload_serialization(), but MainControlGroup(None) returns Some(vec![]). Functionally this is harmless — Some(&[]) causes extend_from_slice(&[]) which is a no-op, producing the same hash as the None path. But the inconsistency may confuse future maintainers. Consider returning None for consistency, or adding a comment explaining the intentional difference.
source: ['claude-rust-quality']
The 3 v1 tests were using a bincode serialize_item() helper that serialized the entire enum (including bincode's variant discriminant), but production uses payload_serialization() which serializes only the inner value. This meant the tests proved hash diffusion (trivially true) rather than that the production serialization differentiates values. All 3 tests now go through the full production calculate_action_id() path with PlatformVersion::latest(), directly proving the vote-swap fix works end-to-end. Removed the serialize_item() helper entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
Mar 17, 2026
…hedule The action_id for TokenSetPriceForDirectPurchaseTransition only hashed minimum_purchase_amount_and_price().1 (the credit price of the lowest tier). This meant different pricing schedules with the same minimum-tier price produced identical action_ids, enabling the same vote-swap attack as the token config update vulnerability fixed in PR #3346. Examples of collisions in the old code: - SetPrices({1: 100, 10: 800}) vs SetPrices({1: 100, 10: 9999}) - SinglePrice(100) vs SetPrices({1: 100}) - SetPrices({5: 100, ...}) vs SetPrices({10: 100, ...}) Fix: serialize the full TokenPricingSchedule with bincode and include it in the hash. This binds the voted-on pricing schedule into the action_id, preventing vote-swap attacks. Note: Unlike the config update fix, this is NOT gated behind a platform version because SetPriceForDirectPurchase is a new feature (v3.1) that has not been used in production yet. There is no backward compatibility concern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
…onsistency - Fix MainControlGroup(None) to return None from payload_serialization() instead of Some(vec![]), matching MaxSupply(None) and other None variants - Add test for AuthorizedActionTakers-based variants (ManualMinting with Group(5) vs Group(9) vs NoOne) through the production path - Add test proving None-payload variants (MaxSupply(None), NewTokensDestinationIdentity(None), PerpetualDistribution(None), MainControlGroup(None)) produce different action_ids via discriminant Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
Mar 17, 2026
…hedule The action_id for TokenSetPriceForDirectPurchaseTransition only hashed minimum_purchase_amount_and_price().1 (the credit price of the lowest tier). This meant different pricing schedules with the same minimum-tier price produced identical action_ids, enabling the same vote-swap attack as the token config update vulnerability fixed in PR #3346. Examples of collisions in the old code: - SetPrices({1: 100, 10: 800}) vs SetPrices({1: 100, 10: 9999}) - SinglePrice(100) vs SetPrices({1: 100}) - SetPrices({5: 100, ...}) vs SetPrices({10: 100, ...}) Fix: serialize the full TokenPricingSchedule with bincode and include it in the hash. This binds the voted-on pricing schedule into the action_id, preventing vote-swap attacks. Note: Unlike the config update fix, this is NOT gated behind a platform version because SetPriceForDirectPurchase is a new feature (v3.1) that has not been used in production yet. There is no backward compatibility concern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
Mar 17, 2026
- calculate_action_id_with_fields returns Result with map_err instead of using expect for bincode encoding - Update wrapper and v0 impls to match the AllowedAsMultiPartyAction trait signature from #3346 (platform_version + Result) - Fix all tests to pass platform_version and unwrap Results - Rebase on #3346 branch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
Mar 17, 2026
…hedule The action_id for TokenSetPriceForDirectPurchaseTransition only hashed minimum_purchase_amount_and_price().1 (the credit price of the lowest tier). This meant different pricing schedules with the same minimum-tier price produced identical action_ids, enabling the same vote-swap attack as the token config update vulnerability fixed in PR #3346. Examples of collisions in the old code: - SetPrices({1: 100, 10: 800}) vs SetPrices({1: 100, 10: 9999}) - SinglePrice(100) vs SetPrices({1: 100}) - SetPrices({5: 100, ...}) vs SetPrices({10: 100, ...}) Fix: serialize the full TokenPricingSchedule with bincode and include it in the hash. This binds the voted-on pricing schedule into the action_id, preventing vote-swap attacks. Note: Unlike the config update fix, this is NOT gated behind a platform version because SetPriceForDirectPurchase is a new feature (v3.1) that has not been used in production yet. There is no backward compatibility concern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
Mar 17, 2026
- calculate_action_id_with_fields returns Result with map_err instead of using expect for bincode encoding - Update wrapper and v0 impls to match the AllowedAsMultiPartyAction trait signature from #3346 (platform_version + Result) - Fix all tests to pass platform_version and unwrap Results - Rebase on #3346 branch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All four prior findings are genuinely resolved. The incremental changes are correct: MainControlGroup(None) refactor is hash-equivalent, tests now use the production path, and new tests fill prior coverage gaps. The two new agent findings (PlatformVersion::latest() brittleness, v0-vs-v1 test naming) are technically valid but represent standard idiomatic patterns in this codebase and do not warrant action.
Reviewed commit: fb32011
QuantumExplorer
added a commit
that referenced
this pull request
Mar 17, 2026
Gate the action_id fix behind platform version, matching the pattern from #3346 (token config update): - Add token_set_price_action_id_version field to DPPTokenVersions - v0: hashes only minimum_purchase_amount_and_price().1 (existing) - v1: hashes the full serialized TokenPricingSchedule (fix) - Create TOKEN_VERSIONS_V3 with both fixes enabled - Update v12 to use TOKEN_VERSIONS_V3 - Add v0 vulnerability regression test and v0-vs-v1 differentiation test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
Mar 17, 2026
After #3346 merged, calculate_action_id takes platform_version as a second argument. Updated 5 test calls to pass PlatformVersion::latest(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Fix a vote-swap vulnerability in the token config update
action_idcalculation.The v0
calculate_action_idforTokenConfigUpdateTransitionhashes only theu8enum discriminant (u8_item_index()), not the actual payload value. This means that, for example,MaxSupply(100)andMaxSupply(999999999999)produce identicalaction_idvalues. A malicious group member could propose one config value, collect votes from other members, then swap to a completely different value with the same discriminant during finalization.What was done?
Introduced a versioned action_id calculation that binds the full config change item value into the hash:
Platform version layer (
rs-platform-version):token_config_update_action_id_versionfield toDPPTokenVersionsTOKEN_VERSIONS_V1: sets field to0(current production behavior, unchanged)TOKEN_VERSIONS_V2: sets field to1(fixed behavior, to be activated in a future protocol version)DPP layer (
rs-dpp):calculate_action_id_with_fields_v1onTokenConfigUpdateTransitionthat serializes the fullTokenConfigurationChangeItemvia bincode and includes it in the double-hashAllowedAsMultiPartyActiontrait with acalculate_action_id_versioneddefault method (delegates to the existingcalculate_action_idby default, so all other transition types are unaffected)calculate_action_id_versionedonTokenConfigUpdateTransitionV0to dispatch between v0/v1 based on the platform versioncalculate_action_id_versionedtoTokenTransitionV0Methodsand its implementation onTokenTransitionvalidate_basic_structureto use the versioned methodv1_methods.rs) to use the versioned methodNo existing behavior changes: All current platform versions (v1-v12) use
TOKEN_VERSIONS_V1withtoken_config_update_action_id_version: 0, so the fix is dormant until a future protocol version activates it.How Has This Been Tested?
6 unit tests covering:
MaxSupplyvalues produce the same action_id under v0Full build verification:
Breaking Changes
None. This is a backward-compatible change. v0 behavior is preserved for all existing platform versions. The fix activates only when a future platform version references
TOKEN_VERSIONS_V2.Checklist:
Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor