Add a _canCancel function to improve granularity and allow roleAdminId to cancel the grant and revoke functions#6573
Conversation
to cancel the grant and revoke functions
|
WalkthroughThis PR refactors the authorization logic in the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/access/manager/AccessManager.sol`:
- Around line 737-744: The inline comment above the cancellation check is
misleading: it says callers must be "guardian of the required role" but the code
uses hasRole(roleId, msgsender) (via _getAdminRestrictions and getRoleAdmin) to
check membership, not guardianship, and skips ADMIN_ROLE; update the comment in
AccessManager to accurately state that for calls targeting this contract the
code checks whether msgsender is a member of the admin role returned by
_getAdminRestrictions (and that ADMIN_ROLE is already checked/skipped) rather
than claiming it verifies guardian status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c4633ab5-456e-465f-b260-2850daca231f
📒 Files selected for processing (1)
contracts/access/manager/AccessManager.sol
| // if the target of the call is this AccessManager and the call data matches an admin-restricted function, then the caller needs to be a guardian of the required role to cancel. | ||
| if (target == address(this)) { | ||
| (bool adminRestricted, uint64 roleId, ) = _getAdminRestrictions(data); | ||
| // Note: ADMIN_ROLE was already checked and can be skipped. | ||
| if (adminRestricted && roleId != ADMIN_ROLE) { | ||
| (bool inRole, ) = hasRole(roleId, msgsender); | ||
| return inRole; | ||
| } |
There was a problem hiding this comment.
Comment does not match the actual logic.
The comment states "the caller needs to be a guardian of the required role to cancel", but the code checks role membership via hasRole(roleId, msgsender), not guardianship. For grantRole/revokeRole operations, roleId is getRoleAdmin(roleId) from _getAdminRestrictions, so this allows members of the admin role to cancel, which aligns with the PR objective but contradicts the comment.
📝 Suggested comment fix
- // if the target of the call is this AccessManager and the call data matches an admin-restricted function, then the caller needs to be a guardian of the required role to cancel.
+ // if the target of the call is this AccessManager and the call data matches an admin-restricted function, then the caller needs to be a member of the required role to cancel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/access/manager/AccessManager.sol` around lines 737 - 744, The
inline comment above the cancellation check is misleading: it says callers must
be "guardian of the required role" but the code uses hasRole(roleId, msgsender)
(via _getAdminRestrictions and getRoleAdmin) to check membership, not
guardianship, and skips ADMIN_ROLE; update the comment in AccessManager to
accurately state that for calls targeting this contract the code checks whether
msgsender is a member of the admin role returned by _getAdminRestrictions (and
that ADMIN_ROLE is already checked/skipped) rather than claiming it verifies
guardian status.
1 participant
Fixes pM-01
PR Checklist
npx changeset add)