Skip to content

feat: common roles component#161

Merged
remollemo merged 3 commits into
mainfrom
remo/roles/minimal-roles
Mar 19, 2026
Merged

feat: common roles component#161
remollemo merged 3 commits into
mainfrom
remo/roles/minimal-roles

Conversation

@remollemo

@remollemo remollemo commented Mar 18, 2026

Copy link
Copy Markdown
Collaborator

Note

High Risk
High risk because it refactors core authorization/role-management paths (grant/revoke/renounce, guards, and legacy role reclaim) and rewires multiple security- and upgrade-gated components to depend on the new role layer.

Overview
Introduces CommonRolesComponent, a new minimal role-management layer (ICommonRoles, Role enum serialization, role-admin hierarchy setup, guard helpers, and legacy role reclaim/disable flows) backed by OZ AccessControl.

Refactors RolesComponent to delegate role state/guards to CommonRolesComponent, adds category-scoped ABIs (IGovernanceRoles, ISecurityRoles, IAppRoles), and tightens non-renounceable role behavior by consolidating errors into AccessErrors::ROLE_CANNOT_BE_RENOUNCED.

Updates Tier-A components (blocklist, pausable, replaceability) and associated mocks/tests to authorize via CommonRolesComponent (including switching replaceability tests from IRoles to ICommonRoles).

Written by Cursor Bugbot for commit 80181c3. This will update automatically on new commits. Configure here.


This change is Reviewable

@remollemo remollemo requested a review from RoeeGross March 18, 2026 13:13
@remollemo remollemo self-assigned this Mar 18, 2026
@codecov

codecov Bot commented Mar 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.71817% with 37 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@62dbc26). Learn more about missing BASE report.

Files with missing lines Patch % Lines
packages/utils/src/components/roles/roles.cairo 72.58% 17 Missing ⚠️
...ils/src/components/common_roles/common_roles.cairo 89.15% 9 Missing ⚠️
...ls/src/components/common_roles/mock_contract.cairo 84.61% 6 Missing ⚠️
...src/components/replaceability/replaceability.cairo 66.66% 2 Missing ⚠️
...ges/utils/src/components/roles/mock_contract.cairo 33.33% 2 Missing ⚠️
...utils/src/components/blocklist/mock_contract.cairo 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #161   +/-   ##
=======================================
  Coverage        ?   94.69%           
=======================================
  Files           ?       57           
  Lines           ?     3993           
  Branches        ?        0           
=======================================
  Hits            ?     3781           
  Misses          ?      212           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Comment thread packages/utils/src/components/roles/roles.cairo
Comment thread packages/utils/src/components/common_roles/common_roles.cairo
@remollemo remollemo force-pushed the remo/roles/minimal-roles branch from d228675 to df25010 Compare March 18, 2026 13:35
@RoeeGross

Copy link
Copy Markdown
Contributor

packages/utils/src/components/blocklist/blocklist.cairo line 47 at r2 (raw file):

        fn add_to_blocklist(ref self: ComponentState<TContractState>, account: ContractAddress) {
            let common_roles = get_dep_component!(@self, CommonRoles);
            common_roles.only_security_admin();

Consider a security agent here

@RoeeGross

Copy link
Copy Markdown
Contributor

packages/utils/src/components/common_roles/common_roles.cairo line 17 at r2 (raw file):

    };

    pub const ROLE_ADMIN_PAIRS: [(RoleId, RoleId); 10] = [

Consider adding some documentation here

@RoeeGross

Copy link
Copy Markdown
Contributor

packages/utils/src/components/common_roles/common_roles.cairo line 85 at r2 (raw file):

        fn initialize(ref self: ComponentState<TContractState>, governance_admin: ContractAddress) {
            let mut access_comp = get_dep_component_mut!(ref self, Access);
            let un_initialized = access_comp.get_role_admin(role: GOVERNANCE_ADMIN).is_zero();

single word

@RoeeGross

Copy link
Copy Markdown
Contributor

packages/utils/src/components/common_roles/common_roles.cairo line 127 at r2 (raw file):

        }

        fn only_role(self: @ComponentState<TContractState>, role: Role) {

consider remove this function and comine them to a single function that got ROLE as arg

Comment thread packages/utils/src/components/roles/roles.cairo
@RoeeGross

Copy link
Copy Markdown
Contributor

packages/utils/src/components/roles/interface.cairo line 244 at r2 (raw file):

}

pub fn is_renounceable(role: Role) -> bool {

very nice!

@RoeeGross RoeeGross left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@RoeeGross reviewed 18 files and all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on remollemo).

@remollemo remollemo 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.

@remollemo made 6 comments.
Reviewable status: 10 of 18 files reviewed, 4 unresolved discussions (waiting on RoeeGross).


packages/utils/src/components/blocklist/blocklist.cairo line 47 at r2 (raw file):

Previously, RoeeGross wrote…

Consider a security agent here

security governor, but this is not for this PR


packages/utils/src/components/common_roles/common_roles.cairo line 17 at r2 (raw file):

Previously, RoeeGross wrote…

Consider adding some documentation here

Done.


packages/utils/src/components/common_roles/common_roles.cairo line 85 at r2 (raw file):

Previously, RoeeGross wrote…

single word

Done.


packages/utils/src/components/common_roles/common_roles.cairo line 127 at r2 (raw file):

Previously, RoeeGross wrote…

consider remove this function and comine them to a single function that got ROLE as arg

I don't understand

Comment thread packages/utils/src/components/common_roles/common_roles.cairo
Comment thread packages/utils/src/components/roles/roles.cairo

@RoeeGross RoeeGross left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@RoeeGross reviewed 8 files and all commit messages, and resolved 4 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on remollemo).

@remollemo remollemo merged commit 8f41888 into main Mar 19, 2026
6 checks passed
@remollemo remollemo deleted the remo/roles/minimal-roles branch March 19, 2026 14:17
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