feat: p/demo/accesscontrol & p/demo/timelock #2307
Conversation
kazai777
requested review from
thehowl and
ltzmaxwell
and removed request for
a team
Co-authored-by: deelawn <[email protected]>
Co-authored-by: deelawn <[email protected]>
Co-authored-by: Manfred Touron <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2307 +/- ##
==========================================
- Coverage 63.32% 63.23% -0.09%
==========================================
Files 548 548
Lines 78511 80400 +1889
==========================================
+ Hits 49715 50841 +1126
- Misses 25443 26136 +693
- Partials 3353 3423 +70
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…rantRole functions
Kouteki
added
review/triage-pending
| const ( | ||
| RoleName = "roleName" | ||
| Sender = "sender" | ||
| Account = "account" | ||
| NewAdminRole = "newAdminRole" | ||
| ) |
There was a problem hiding this comment.
Can we just place this directly in the events, and instead have as exported constants the events, in the format RoleCreatedEvent = "RoleCreated"?
I suggest you put information about the keys of each event in the godoc of each event name.
There was a problem hiding this comment.
Indeed it will bring more clarity
3b4c63a
| } | ||
|
|
||
| func validRoleName(name string) error { | ||
| if len(name) > 30 || name == "" { |
There was a problem hiding this comment.
The limit on 30 seems kind of arbitrary for a package; more like a kind of validation that should be done, if anything, on the side of a realm (as an end-user application). But I don't expect many realms to publicly allow adding roles, anyway.
|
|
||
| // Roles struct to store all Roles information | ||
| type Roles struct { | ||
| AllRoles []*Role |
There was a problem hiding this comment.
| AllRoles []*Role | |
| Roles []*Role | |
| UserToRoles avl.Tree // std.Address -> []*Role |
A suggestion. You'd then need to add the code to manage UserToRoles, but I think we should support being able to map a user to the roles they have.
(Of course, there should be a related function to modify it.)
| // HasRole check if an account has a specific role | ||
| func (r *Role) HasRole(account std.Address) bool { |
There was a problem hiding this comment.
Doesn't make sense. It should be Role.HasAccount.
| // RenounceRole allows an account to renounce a role it holds | ||
| func (rs *Roles) RenounceRole(name string) error { |
There was a problem hiding this comment.
I would say we remove this.
- Roles is not likely to be publicly exposed in a realm, anyway.
- But if it was, exposing
RenounceRolemeans that theRolescannot be used for a role likebanned; because the user can "renounce it".
There was a problem hiding this comment.
Can we make a case for how this is different / better than p/demo/acl?
I'm not saying it's perfect, just that demo/ should probably contain one preferred ACL implementation. We can decide to move this one to p/<name>/accesscontrol, or that one to p/nt/acl. (cc'ing also @moul for an opinion on what to do here)
There was a problem hiding this comment.
Although acl and accesscontrol may seem similar at first glance, accesscontrol stands out due to its ability to implement role hierarchies as well as dynamic permission options
There was a problem hiding this comment.
Can you give an example? Namely, of where this distinction is useful?
There was a problem hiding this comment.
I see we're using std.PrevRealm() to determine the owner and generally the "sender". This assumes there's an admin user doing management, and everyone else just following suit.
However, I think an equally possible flow is that of having a realm which has an access control list. In this case, actually, we shouldn't do any checks on PrevRealm(); the realm can just use it unexported. But I suggest you have an option for the ACL to not have a "owner"; in which case the PrevRealm checks are simply not performed. Allows someone else to compose other rules on top as well.
Btw if Roles is meant to be exposed in a realm, then its fields should be unexported.
| } | ||
|
|
||
| // TimeLockUtil stores the necessary parameters for the timelock operations | ||
| type TimeLockUtil struct { |
There was a problem hiding this comment.
Util to represent a "toolbox" of essential parameters and methods needed for managing locking operations
You don't find the word explicit enough ? @thehowl
There was a problem hiding this comment.
No, it just seems redundant. Also, these "toolboxes" are generally discouraged in Go (and Gno): https://dave.cheney.net/2019/01/08/avoid-package-names-like-base-util-or-common
There was a problem hiding this comment.
Thank for shared the article on avoid-package-names.
As a result changed TimelockUtil to Timelock
0ced75a
We have developed two packages:
accesscontrolandtimelockinspired by openzeppelin contracts. These packages were created in collaboration with @mous1985 , @DIGIX666 , and myself.The
accesscontrolpackage was primarily designed to support the development of thetimelockpackage, but it can also be used independently for many other use cases.Features
Accesscontrol
The
accesscontrolpackage provides a library for managing roles and permissions within Gno. It allows for the creation, assignment, and management of roles with specific administrative privileges, ensuring that only authorized accounts can perform certain actions.Timelock
The
timelockpackage offers a library for scheduling, canceling, and executing time-locked operations in Gno. It ensures that operations are only carried out after a specified delay and provides mechanisms to manage and verify the status of these operations. The creation of theaccesscontrolpackage was necessary to provide role and permission management required for the administrative tasks oftimelock.Use Cases
Accesscontrol
Timelock
These examples of use cases are not exhaustive, and many other things are possible with these packages.
Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description