-
Notifications
You must be signed in to change notification settings - Fork 12.4k
Add AccessManager contracts #4121
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
frangio
merged 79 commits into
OpenZeppelin:feat/access-manager
from
frangio:accessmanager
Mar 24, 2023
Merged
Changes from 1 commit
Commits
Show all changes
79 commits
Select commit
Hold shift + click to select a range
1327b8f
add AccessManager.sol
frangio b54b922
docs
frangio 5a0fd33
enforce team existence
frangio ee5de96
remove on-chain enumerability
frangio 6284768
add Authority interface
frangio bce3641
remove redundancy
frangio 223e6af
simplify and complete features
frangio eafb571
split in separate files
frangio 24af159
write AccessManager docs
frangio 833f219
add docs for AccessManagerAdapter
frangio 0f27739
add to docs site
frangio 2ac5aa1
docs for IAuthority
frangio f26c4d3
lint
frangio 63f9393
add initial AccessManaged docs
frangio 6fecd25
typos
frangio 589b5b1
Update contracts/access/manager/AccessManager.sol
frangio 619cc73
fix docs
frangio 1b681a5
rename team "all" -> "public"
frangio 48b31c0
ad hoc selector batching
frangio b078157
whitespace
frangio 274c04d
add docs on team number choice
frangio 082b05c
add missing function argument
frangio 52f27c8
fix team role decoding
frangio 1285c49
add ungrouped interface
frangio 5c7472a
improve docs
frangio 3cc1e73
remove todo
frangio ae3482d
add initial tests for teams
frangio 535ea13
rename team -> badge
frangio 68bb5fa
improve testing
frangio fa58ec5
Update contracts/access/manager/AccessManager.sol
frangio 6d76058
use mapping labels from solidity 0.8.18
frangio 3447438
add AccessManaged tests
frangio 1d10639
fix group encoding
frangio d875a7b
add test for allowing
frangio d517d27
lint
frangio 6d33cda
add changeset
frangio 9ed1aef
lint
frangio 6adc0d8
fix mock
frangio 0624794
remove contract groups
frangio 17ec3b6
add transferContractAuthority
frangio f9210c5
lint
frangio a54cd10
rename badge -> group
frangio 8af9ca4
change test name
frangio 2a05bcc
add setContractModeCustom and tests
frangio 5f22f2d
remove use of mapping labels
frangio 6a9fbed
Merge branch 'master' into accessmanager
frangio 0580198
tweak comments
frangio e01e362
add tests for adapter
frangio cb026bc
lint
frangio 76d35da
add revert reasons and group tests
frangio 4069781
add tests for allowing and disallowing roles
frangio 80ffd2a
lint
frangio 3094d0c
add docs for AccessManaged
frangio b3a8b1b
Update contracts/access/manager/AccessManager.sol
frangio 8a68322
Update contracts/access/manager/AccessManager.sol
frangio eeab8cf
Update contracts/access/manager/AccessManager.sol
frangio ca61e38
add tests for onlyDefaultAdmin
frangio 8824c31
lint
frangio 67e33b6
add internal _setContractMode
frangio 5f270c7
Update .changeset/quiet-trainers-kick.md
frangio 6f7ac96
Update contracts/access/manager/AccessManager.sol
frangio 7ec5311
Update contracts/access/manager/AccessManager.sol
frangio 82402d2
typo
frangio 27807e6
Apply suggestions from code review
frangio 80bc88b
roll back to 0.8.13
frangio cf4df22
add test for setFunctionAllowedGroup events
frangio bab4d34
remove mode restriction on setFunctionAllowedGroup
frangio 642e279
simplify use of setFunctionAllowedGroup
frangio 51aff23
lint
frangio 6e3da66
remove unused import
frangio 6b56c8f
remove onlyDefaultAdmin modifier
frangio b7e3b3f
lint
frangio 8200986
use return value names for _decodeGroupRole
frangio 18e53e2
rename RestrictedMode -> AccessMode
frangio f94a881
use Context._msgSender
frangio cd8babd
reorder arguments to grant/revoke/renounceGroup
frangio 597edc0
add IAccessControlDefaultAdminRules
frangio ee560d0
Apply suggestions from code review
frangio 33f5ace
Fix expected event parameter name in tests
ernestognw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
lint
- Loading branch information
commit f26c4d3abc49ee966aab3d07f5f2485f159a58d8
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I know that is restrictive, but making this immutable would save a non insignificant amount of gas.
Should we have two versions ?
In the case of proxy/clones created by a factory, would it make sens that all implementation use the same authority? I think yes.
Uh oh!
There was an error while loading. Please reload this page.
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.
You know I hate multiple versions... 😄
The main reason why I left this as a mutable variable is because I think it can be used to freeze the permissions in a contract by moving it off of an AccessManager onto an immutable Authority. We could also implement freezing in the AccessManager itself, by having frozen contract groups in which permissions can't be altered, and whose contracts can't be moved out of that group. I felt that having mutability of the authority at the managed contract was the simpler option, but it's true that it makes it more expensive.
I'm open to this discussion though.
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.
This makes sense, but I also think it violates the goal of consolidation. In this way, now a managed contract may have the right to update itself without the Manager's approval.
While that still makes sense to me in some cases, I am not sure if a significant share of
is AccessManagedcontracts will want that capability when the main value proposition is having everything on the manager.My opinion here would be to make it immutable since any freezing capability can be handled by the Manager by making it
is AccessManagedas well, and using theCLOSEDgroup. Maybe a bit complex, but I think users may naturally try it.Uh oh!
There was an error while loading. Please reload this page.
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.
I don't think this is true. Only the current manager should be able to "eject" a managed contract from itself.
Note that adding a contract in the Closed or Open group doesn't freeze the permissions, because the manager retains the ability to move it outside of the group.
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.
Why not? Any
is AccessManagedcontract will have access to that storage slot even if the variable is private.Now I see that a counterargument is that accessing that storage slot has to be deliberated.
We can keep this private, but I have some bad feelings about letting the user "eject" the contract itself even if done deliberately.
I see what you meant, but a previous idea I remember we discussed is suggesting overrides like:
Not sure exactly how this can play out, but I think there might be a setup we may like to explore (eg. not reopening by default unless explicitly stated).
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.
IMO it's fine if the target contract deliberately inserts a custom-gated
_setAuthorityfunction call. I wouldn't expect it normally, but I can imagine some sort of emergency mechanism.