Skip to content

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

Merged
QuantumExplorer merged 2 commits into
v4.1-devfrom
claude/pr-4000-review-fix-bc2c91
Jul 23, 2026
Merged

fix(drive-abci): report configured main control group authority when unauthorized#4209
QuantumExplorer merged 2 commits into
v4.1-devfrom
claude/pr-4000-review-fix-bc2c91

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Jul 23, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

Replaces #4000, reworked on top of the design introduced in #4154.

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 #4000 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 fixes 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

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved authorization handling for token configuration changes involving the main control group.
    • Unauthorized changes now report the correctly configured authorized action takers.
    • Valid group members can complete main control group updates once the required quorum is reached.
  • Tests

    • Added coverage for supported authorization rules, successful group-controlled changes, and unauthorized attempts.

…unauthorized

validate_state_v1 delegated the authorization check to validate_state_v0,
whose UnauthorizedTokenActionError reported the takers from
authorized_action_takers_for_configuration_item, which hardcodes NoOne for
MainControlGroup change items. The v0 report must stay unchanged where it is
already live, so v1 now passes the controlling rule
(main_control_group_can_be_modified) to a parameterized v0 helper and reports
it instead.

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

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@QuantumExplorer, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 22c1d691-b246-4cc4-9e5d-b4bb5c279746

📥 Commits

Reviewing files that changed from the base of the PR and between 5473526 and efe98e7.

📒 Files selected for processing (1)
  • packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_config_update_transition_action/state_v0/mod.rs
📝 Walkthrough

Walkthrough

Added override-aware token configuration validation and tests for group-controlled MainControlGroup changes, including quorum completion and unauthorized error reporting.

Changes

Token configuration authorization

Layer / File(s) Summary
Configured action-taker mapping
packages/rs-dpp/.../authorized_action_takers_for_configuration_item/v0/mod.rs
Adds coverage confirming configured action-taker variants are returned verbatim for MainControlGroup changes.
Validation reporting override
packages/rs-drive-abci/.../token_config_update_transition_action/state_v0/mod.rs, packages/rs-drive-abci/.../token_config_update_transition_action/state_v1/mod.rs
Adds an optional unauthorized-action reporting override and passes resolved v1 action takers into v0 validation.
Group-controlled update scenarios
packages/rs-drive-abci/.../tests/token/config_update/mod.rs
Tests group quorum completion and verifies unauthorized changes report AuthorizedActionTakers::Group(0) while leaving the main control group unchanged.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • dashpay/platform#4154: Introduced the related controlling_action_takers_for_configuration_item mapping covered by the new unit test.

Sequence Diagram(s)

sequenceDiagram
  participant GroupMember
  participant TokenConfigUpdateValidation
  participant TokenConfiguration
  participant State
  GroupMember->>TokenConfigUpdateValidation: submit MainControlGroup update
  TokenConfigUpdateValidation->>TokenConfiguration: resolve authorized action takers
  TokenConfigUpdateValidation->>TokenConfigUpdateValidation: validate with resolved action takers
  TokenConfigUpdateValidation->>State: apply update after quorum
  State-->>GroupMember: main control group becomes Some(0)
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: unauthorized MainControlGroup updates now report the configured authority.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/pr-4000-review-fix-bc2c91

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.

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.51%. Comparing base (0f2d4bb) to head (5473526).
⚠️ Report is 13 commits behind head on v4.1-dev.

Files with missing lines Patch % Lines
...en_config_update_transition_action/state_v0/mod.rs 60.00% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           v4.1-dev    #4209    +/-   ##
==========================================
  Coverage     87.51%   87.51%            
==========================================
  Files          2666     2667     +1     
  Lines        336904   337576   +672     
==========================================
+ Hits         294837   295427   +590     
- Misses        42067    42149    +82     
Components Coverage Δ
dpp 88.50% <98.47%> (+0.02%) ⬆️
drive 86.26% <95.91%> (+0.02%) ⬆️
drive-abci 89.51% <25.78%> (-0.07%) ⬇️
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.88% <94.67%> (-0.02%) ⬇️
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 49.79% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

… validator

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@QuantumExplorer
QuantumExplorer merged commit f894d10 into v4.1-dev Jul 23, 2026
30 of 32 checks passed
@QuantumExplorer
QuantumExplorer deleted the claude/pr-4000-review-fix-bc2c91 branch July 23, 2026 05:30
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