Skip to content

fix(dpp): report main control group authorization#4000

Draft
thepastaclaw wants to merge 2 commits into
dashpay:v4.1-devfrom
thepastaclaw:fix/main-control-group-authorized-takers
Draft

fix(dpp): report main control group authorization#4000
thepastaclaw wants to merge 2 commits into
dashpay:v4.1-devfrom
thepastaclaw:fix/main-control-group-authorized-takers

Conversation

@thepastaclaw

@thepastaclaw thepastaclaw commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

TokenConfigurationChangeItem::MainControlGroup is authorized by the token field main_control_group_can_be_modified.

The validation gate already used that field correctly. This PR does not change who is allowed to update a token main control group, and it does not make contract group membership mutable.

The bug was in the helper used to describe an unauthorized token config update. For MainControlGroup, authorized_action_takers_for_configuration_item always returned NoOne, even when main_control_group_can_be_modified was configured as ContractOwner, MainGroup, Group(0), or a specific identity. That made failures look like the action was impossible for everyone, when the real rule could be something else.

In simpler terms, this changes an incorrect error/reporting answer from:

Access denied. Required actor: NoOne

to the configured rule, for example:

Access denied. Required actor: Group(0)

It is a consensus error/reporting correctness fix, not a validation-rule change.

What was done?

  • Changed authorized_action_takers_for_configuration_item(MainControlGroup(_)) to return main_control_group_can_be_modified instead of hardcoded NoOne.
  • Kept the default case covered: restrictive/default token configs still report NoOne.
  • Added regression coverage that configured values are reported verbatim for MainControlGroup.
  • Added Drive ABCI coverage documenting the existing valid path where a configured group can update a token main control group once enough group power has signed.

Regression coverage

The main regression test is:

main_control_group_returns_configured_can_be_modified

It proves that when main_control_group_can_be_modified is configured, the helper reports that configured authority instead of NoOne.

The supporting integration test is:

test_token_config_update_by_group_member_changing_main_control_group

It proves the existing validation behavior still works for a correctly configured token: one group member proposes the main-control-group update, the change is not applied with only 1/2 power, and the second group member completes the action.

How Has This Been Tested?

Local tests run from fix/main-control-group-authorized-takers:

cargo test -p dpp authorized_action_takers_for_configuration_item
cargo test -p drive-abci test_token_config_update_by_group_member_changing_main_control_group

Both passed.

Also ran a local pre-PR code review against upstream/v4.1-dev..HEAD; no significant issues found.

Breaking Changes

None.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

thepastaclaw and others added 2 commits July 4, 2026 14:59
…oup change item

authorized_action_takers_for_configuration_item returned NoOne for
MainControlGroup(_) regardless of configuration, so
UnauthorizedTokenActionError reported NoOne even when
main_control_group_can_be_modified allowed the change, making
misconfigured contracts look like group mutation was broken.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Covers the config-update path where main_control_group_can_be_modified
is Group(0): the proposer alone does not apply the change, and the
second group signature completes it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added this to the v4.1.0 milestone Jul 4, 2026
@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This change modifies TokenConfigurationV0::authorized_action_takers_for_configuration_item so the MainControlGroup variant returns the configured main_control_group_can_be_modified value instead of a hard-coded NoOne. Unit tests were updated accordingly, and a new integration test verifies group-governed main control group updates in rs-drive-abci.

Changes

MainControlGroup authorized action takers

Layer / File(s) Summary
Authorization logic and unit tests
packages/rs-dpp/src/data_contract/associated_token/token_configuration/methods/authorized_action_takers_for_configuration_item/v0/mod.rs
MainControlGroup now returns main_control_group_can_be_modified instead of hard-coded NoOne; tests updated to cover default behavior and configured AuthorizedActionTakers values.
Group-governed control update integration test
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/config_update/mod.rs
New async test validates that a main control group update requires sufficient group power before applying, verified across proposer and non-proposer group member submissions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Proposer
  participant GroupMember
  participant StateTransitionValidator
  participant TokenConfiguration

  Proposer->>StateTransitionValidator: Propose main control group update
  StateTransitionValidator->>TokenConfiguration: Check accumulated group power
  TokenConfiguration-->>StateTransitionValidator: main_control_group remains None
  GroupMember->>StateTransitionValidator: Submit corresponding update action
  StateTransitionValidator->>TokenConfiguration: Recheck accumulated group power
  TokenConfiguration-->>StateTransitionValidator: main_control_group set to Some(0)
Loading

Related PRs: None identified.

Suggested labels: rust, tests, tokens

Suggested reviewers: None identified.

🐰 A rabbit hops through control group gates,
No longer "no one" who legislates,
Power accrues from member to member,
Until the main group finds its timber,
Tests confirm the change validates.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly captures the main DPP change: reporting main control group authorization correctly.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant