Add AccessManager role-based account handler#248
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds a new Solidity file implementing two contracts: ChangesRole Account and Manager Contracts
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant RoleAccount
participant AccessManager
Caller->>RoleAccount: _rawSignatureValidation(hash, signature)
RoleAccount->>RoleAccount: roleId() via Clones immutable args
RoleAccount->>AccessManager: check role membership of signer
AccessManager-->>RoleAccount: membership result
alt signer holds role
RoleAccount->>RoleAccount: SignatureChecker.isValidSignatureNow
RoleAccount-->>Caller: validation result
else signer lacks role
RoleAccount-->>Caller: false
end
sequenceDiagram
participant Caller
participant AccessManagerWithRoleAccounts
participant Clones
Caller->>AccessManagerWithRoleAccounts: deployConfidentialHandler(roleId)
AccessManagerWithRoleAccounts->>AccessManagerWithRoleAccounts: _roleToSalt(roleId)
AccessManagerWithRoleAccounts-->>Caller: revert if PUBLIC_ROLE
AccessManagerWithRoleAccounts->>Clones: deploy deterministic clone with immutable args
Clones-->>AccessManagerWithRoleAccounts: clone address
AccessManagerWithRoleAccounts-->>Caller: clone address
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🧹 Nitpick comments (1)
contracts/access/manager/AccessManagerRoleAccount.sol (1)
52-55: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider whether
ADMIN_ROLEshould also be locked.
_roleToSaltrejectsPUBLIC_ROLEbut permitsADMIN_ROLE(0), allowing a handler bound to the admin role. Since any admin-role member could then sign on behalf of that account, confirm this is intended or rejectADMIN_ROLEas well.🤖 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/AccessManagerRoleAccount.sol` around lines 52 - 55, The _roleToSalt check currently blocks PUBLIC_ROLE but still allows ADMIN_ROLE, so review whether admin-bound handlers should be disallowed too. Update _roleToSalt in AccessManagerRoleAccount to either reject ADMIN_ROLE alongside PUBLIC_ROLE or clearly keep it permitted only if that is the intended behavior, using the existing AccessManagerLockedRole guard for the rejected role(s).
🤖 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/AccessManagerRoleAccount.sol`:
- Around line 44-50: The confidential handler clone helpers are encoding roleId
with abi.encode, which produces 32 bytes and breaks the expected 8-byte
immutable args read by roleId(). Update both getConfidentialHandler and
deployConfidentialHandler in AccessManagerRoleAccount to use packed encoding for
the role id so fetchCloneArgs() receives the correct 8-byte blob and the clone
can read the uint64 properly.
---
Nitpick comments:
In `@contracts/access/manager/AccessManagerRoleAccount.sol`:
- Around line 52-55: The _roleToSalt check currently blocks PUBLIC_ROLE but
still allows ADMIN_ROLE, so review whether admin-bound handlers should be
disallowed too. Update _roleToSalt in AccessManagerRoleAccount to either reject
ADMIN_ROLE alongside PUBLIC_ROLE or clearly keep it permitted only if that is
the intended behavior, using the existing AccessManagerLockedRole guard for the
rejected role(s).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df554d88-fc56-4499-b608-9191ebef4723
📒 Files selected for processing (1)
contracts/access/manager/AccessManagerRoleAccount.sol
Enable role members to execute via ERC7821 and refactor membership check into _isMember helper.
|
Thoughts on allowing execution from the handler by anyone with the role? Maybe it is overkill but it makes the usecase more generic. Then we don't need to use the phrasing |
The current version supports execution through the ERC-7821 inheritance |
2 participants
Summary
Adds
AccessManagerRoleAccount.sol, introducing role-based account handlers backed by anAccessManager.RoleAccount: an ERC-4337Account(with ERC-7739 support) whose signature validation delegates authorization to anAccessManagerrole. The boundroleIdis stored as clone immutable args, and a signature is valid only if the recovered signer holds the account's role in the manager.AccessManagerWithRoleAccounts: anAccessManagerextension that deterministically deploys (deployConfidentialHandler) and predicts (getConfidentialHandler) aRoleAccountclone per role, using the role id as salt.Notes
Summary by CodeRabbit