-
Notifications
You must be signed in to change notification settings - Fork 156
feat: access control roles #132
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
Conversation
fdd512f to
f7c492d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces role-based access control to the MasterVault system, replacing the previous owner-centric model with granular permissions for vault management, fee management, and pausing operations. The changes enable more flexible delegation of responsibilities without requiring all operations to go through the factory contract.
Key changes:
- Replaces
OwnableUpgradeablewithAccessControlUpgradeableand defines four roles:DEFAULT_ADMIN_ROLE,VAULT_MANAGER_ROLE,FEE_MANAGER_ROLE, andPAUSER_ROLE - Adds pausability to the MasterVault with pause/unpause functions restricted to
PAUSER_ROLE - Removes
setSubVaultmethod from factory contract, allowing direct vault management by role holders
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| contracts/tokenbridge/libraries/vault/MasterVault.sol | Implements AccessControl and Pausable, updates modifiers from onlyOwner to role-based checks, adds pause/unpause functions, fixes maxMint edge case |
| contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol | Removes setSubVault method, changes vault initialization to grant roles to owner instead of transferring ownership |
| contracts/tokenbridge/libraries/vault/IMasterVaultFactory.sol | Removes setSubVault function and SubVaultSet event from interface |
| test-foundry/libraries/vault/MasterVault.t.sol | Adds comprehensive tests for role-based access control and pause functionality |
| test-foundry/libraries/vault/MasterVaultFactory.t.sol | Updates tests to verify role grants instead of ownership |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
godzillaba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just a couple minor comments
Motivation:
We don't want to give the owner always full power with all master vault functionalities. so I'm introducing access control roles, with that requests doesn't have to always go thought the factory contract like
setSubvaulttherefore we dropped this call.changes:
setSubvaultfrom factory contract