Skip to content

fix(drive-abci): report configured main control group authority when unauthorized#4000

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

fix(drive-abci): report configured main control group authority when unauthorized#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, but the UnauthorizedTokenActionError produced when the check fails reported the takers from authorized_action_takers_for_configuration_item, which hardcodes NoOne for MainControlGroup. That made failures look like the action was impossible for everyone, when the configured rule could be ContractOwner, MainGroup, Group(0), or a specific identity:

Access denied. Required actor: NoOne      # reported
Access denied. Required actor: Group(0)   # actual configured rule

Since this PR was opened, #4154 added controlling_action_takers_for_configuration_item (which returns main_control_group_can_be_modified) and a version-gated state_v1 validation that uses it for the group-binding check. That superseded the original approach here of changing authorized_action_takers_for_configuration_item directly — the legacy helper's report must stay unchanged for protocol versions where it is already live.

However, the misleading report was still reachable at the new validation version: validate_state_v1 delegates the main authorization check to validate_state_v0, which still built the error from the legacy helper. This PR has been reworked on top of #4154 to fix that remaining path.

What was done?

  • Added validate_state_v0_with_reported_action_takers, which takes an optional override for the action takers reported in UnauthorizedTokenActionError. validate_state_v0 passes None, so behavior at validation version 0 is byte-for-byte unchanged.
  • validate_state_v1 now passes the already-computed controlling rule, so unauthorized MainControlGroup updates report main_control_group_can_be_modified instead of NoOne. For every other change item the controlling rule equals the legacy report, so nothing else changes.
  • Added a dpp unit test that controlling_action_takers_for_configuration_item reports each configured main_control_group_can_be_modified value verbatim.
  • Added Drive ABCI coverage:
    • test_token_config_update_main_control_group_unauthorized_reports_configured_rule — a non-member submitting a MainControlGroup change gets UnauthorizedTokenActionError reporting Group(0) (the configured rule), not NoOne.
    • test_token_config_update_by_group_member_changing_main_control_group — the existing valid path where a configured group updates the main control group once enough group power has signed.

How Has This Been Tested?

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

Breaking Changes

None. Validation version 0 output is unchanged; the corrected report only applies at validation version 1 (protocol v13, not yet released).

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 is concise and accurately reflects the core fix around main control group authorization reporting, though it understates the added DPP and test coverage changes.
✨ 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.

@thepastaclaw

thepastaclaw commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator Author

✅ Final review complete — no blockers (commit fe77f1f)

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Codex + Sonnet

PR #4000 fixes a narrow reporting bug: authorized_action_takers_for_configuration_item previously hardcoded NoOne for MainControlGroup change items instead of returning the actual main_control_group_can_be_modified field. I verified the diff directly — the actual authorization gate (can_apply_token_configuration_item, lines 285-292 of can_apply_token_configuration_item/v0/mod.rs) already used main_control_group_can_be_modified correctly and is untouched by this PR, so no authorization behavior changes, only the error message reported via UnauthorizedTokenActionError. I traced that error's serialization path to ExecTxResult.info (not .data), confirming it is non-consensus-critical per the ABCI spec, so no platform_version gate is required. Both new DPP unit tests and the drive-abci group-authorization integration test are correct, compile-consistent, and meaningfully cover the fix and the underlying two-step group-approval invariant. No in-scope defects found.

Review provenance

  • Codex reviewers: gpt-5.6-sol — general (completed)
  • Verifier: claude-sonnet-5 — verifier
  • Sonnet reviewers: claude-sonnet-5 — general (completed)

@QuantumExplorer
QuantumExplorer marked this pull request as ready for review July 23, 2026 03:33
@QuantumExplorer
QuantumExplorer self-requested a review as a code owner July 23, 2026 03:33
@QuantumExplorer QuantumExplorer changed the title fix(dpp): report main control group authorization fix(drive-abci): report configured main control group authority when unauthorized Jul 23, 2026
@QuantumExplorer

Copy link
Copy Markdown
Member

superceeded by 4209

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.

2 participants