Skip to content

fix(access): l-04 misleading documentations regarding default admin role#438

Open
0xisk wants to merge 1 commit intomainfrom
fix/l-04
Open

fix(access): l-04 misleading documentations regarding default admin role#438
0xisk wants to merge 1 commit intomainfrom
fix/l-04

Conversation

@0xisk
Copy link
Copy Markdown
Member

@0xisk 0xisk commented Apr 9, 2026

Types of changes

What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Fixes #429

PR Checklist

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Summary by CodeRabbit

Release Notes

  • Refactor
    • Updated the default admin role handling to use an explicit function that returns zero bytes, replacing the previous constant approach. This clarifies the default admin role behavior and ensures consistency across the system.

@0xisk 0xisk requested review from a team as code owners April 9, 2026 14:17
@0xisk 0xisk self-assigned this Apr 9, 2026
@0xisk 0xisk requested a review from andrew-fleming April 9, 2026 14:17
@0xisk 0xisk added the audit Issues reported by an audit label Apr 9, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 9, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9fef5a59-5c00-4fc9-8526-997e6e1250f8

📥 Commits

Reviewing files that changed from the base of the PR and between 25aa666 and 37ad882.

📒 Files selected for processing (2)
  • contracts/src/access/AccessControl.compact
  • contracts/src/access/test/mocks/MockAccessControl.compact

Walkthrough

Converted DEFAULT_ADMIN_ROLE from a ledger constant to a pure circuit function returning zero bytes, eliminating misleading documentation that implied the value could be customized while it always returned the default value.

Changes

Cohort / File(s) Summary
AccessControl Contract
contracts/src/access/AccessControl.compact
Replaced ledger constant DEFAULT_ADMIN_ROLE with a pure circuit function that returns default<Bytes<32>>. Updated getRoleAdmin() to call the function instead of referencing storage. Clarified module documentation to reflect that the default admin role is explicitly zero bytes and not customizable.
Mock AccessControl
contracts/src/access/test/mocks/MockAccessControl.compact
Removed AccessControl_DEFAULT_ADMIN_ROLE from exports and added a new pure circuit DEFAULT_ADMIN_ROLE() function that delegates to the underlying implementation.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 A constant masquerading as choice,
Now shines as truth—a function's voice!
Zero bytes forever pure and clear,
No more confusion, all sincere. ✨

🚥 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 accurately describes the main change: fixing misleading documentation regarding the DEFAULT_ADMIN_ROLE in AccessControl by converting it from a ledger constant to a pure circuit function.
Linked Issues check ✅ Passed The PR successfully addresses issue #429 by removing the stored DEFAULT_ADMIN_ROLE ledger entry and replacing it with a pure circuit function, while updating documentation to clarify that the default admin is explicitly zero bytes.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the DEFAULT_ADMIN_ROLE issue: modifications to AccessControl.compact and MockAccessControl.compact align with the requirement to remove the ledger entry and update behavior.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/l-04

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


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

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

Labels

audit Issues reported by an audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

L-04: Misleading Documentation Regarding DEFAULT_ADMIN_ROLE

1 participant