Skip to content

Unshielded Access Control #182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 47 commits into from
Jul 23, 2025
Merged

Unshielded Access Control #182

merged 47 commits into from
Jul 23, 2025

Conversation

emnul
Copy link
Contributor

@emnul emnul commented Jul 17, 2025

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Documentation Update (if none of the other choices apply)

Fixes #89

This pull request adds an unshielded AccessControl module to the Compact contracts library. Some notable design decisions:

  • Implemented an _unsafeGrantRole function to allow users to grant roles to a ContractAddress
  • Did not implement an _unsafeRevokeRole function since I didn't see any major security concerns with implementing revokeRole and _revokeRole as is
  • No RoleData struct. Since struct types in Compact cannot contain ledger data types I decided to instead create a _operatorRoles ledger variable and _adminRoles ledger variable to Map roles -> accounts -> permissions and roles -> admin roles respectively.

PR Checklist

  • I have read the Contributing Guide
  • I have added tests that prove my fix is effective or that my feature works
  • I have added documentation of new methods and any new behavior or changes to existing behavior
  • CI Workflows Are Passing

@emnul emnul self-assigned this Jul 17, 2025
@emnul emnul added enhancement New feature or request work-in-progress Pull requests which are still being worked on, more changes will follow. labels Jul 17, 2025
@emnul emnul added this to the 1. current milestone Jul 17, 2025
Copy link
Contributor

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Great start, @emnul! The design looks good. I left some comments

Co-authored-by: Andrew Fleming <[email protected]>
Signed-off-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]>
Copy link
Contributor Author

@emnul emnul left a comment

Choose a reason for hiding this comment

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

Initial review of changes

@emnul
Copy link
Contributor Author

emnul commented Jul 18, 2025

@andrew-fleming after playing around with the contract it seems that setting the DEFAULT_ADMIN_ROLE to 0 in initialize() is redundant. We could potentially get rid of the Initializable import and checks entirely. I like the idea of getting rid of this and instructing users that if they want a custom a DEFAULT_ADMIN_ROLE they would need to implement the Initializable module along with all of its checks. I don't believe setting a custom DEFAULT_ADMIN_ROLE is common enough to justify keeping all of the Initializable boilerplate, but what do you think?

@emnul emnul removed the work-in-progress Pull requests which are still being worked on, more changes will follow. label Jul 21, 2025
@emnul emnul removed the needs-review Pull requests which need code review, and approval from maintainers or OpenZeppelin core team. label Jul 22, 2025
@emnul emnul requested a review from andrew-fleming July 22, 2025 22:02
@emnul emnul added the under-review Pull requests being reviewed by OpenZeppelin core team. label Jul 22, 2025
Copy link
Contributor

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Improvements look outstanding! I left a few more suggestions (super minor, with the code examples). Otherwise, I think we're good to go 🚀

Co-authored-by: Andrew Fleming <[email protected]>
Signed-off-by: ⟣ €₥ℵ∪ℓ ⟢ <[email protected]>
Copy link
Contributor Author

@emnul emnul left a comment

Choose a reason for hiding this comment

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

Completed review of most recent changes

@emnul emnul requested a review from andrew-fleming July 23, 2025 16:50
Copy link
Contributor

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

LGTM!

@emnul emnul requested a review from andrew-fleming July 23, 2025 18:01
@andrew-fleming andrew-fleming merged commit 7dcccc2 into main Jul 23, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request under-review Pull requests being reviewed by OpenZeppelin core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unshielded access control
2 participants