feat(standards): add Role Based Access Control Account Component#2712
feat(standards): add Role Based Access Control Account Component#2712
Conversation
…role-based-access-control
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Took an initial look, but haven't gotten through everything yet.
| proc get_admin_internal | ||
| exec.load_admin_info | ||
| # => [admin_suffix, admin_prefix, nominated_admin_suffix, nominated_admin_prefix] |
There was a problem hiding this comment.
Nit: Should we rename load_admin_info to get_admin_config to consistently use the get prefix? And similarly save_admin_info -> set_admin_config? (Config since the slot is called config rather than info). Similarly for other load_ and save_ procedures.
| #! - is_admin is 1 if the note sender is the current root admin, otherwise 0. | ||
| #! | ||
| #! Invocation: exec | ||
| proc is_sender_admin |
There was a problem hiding this comment.
Currently, it's ambiguous whether "admin" refers to the root admin or a role admin. I'd make this unambiguous by using "root admin" when we mean that, e.g.:
is_sender_admin->is_sender_root_adminassert_sender_is_admin_or_role_admin->assert_sender_is_root_or_role_adminget_admin_internal->get_root_admin_internalERR_SENDER_NOT_ADMIN->ERR_SENDER_NOT_ROOT_ADMIN- ...
| # Per-role reverse member index map slot. | ||
| # Map entries: [0, role_symbol, account_suffix, account_prefix] -> [member_index_plus_one, 0, 0, 0] | ||
| const ROLE_MEMBER_INDEX_SLOT = word("miden::standards::access::role_based_access_control::role_member_index") |
There was a problem hiding this comment.
We use member_index_plus_one instead of member_index because we use 0 to indicate that a member does not have the role, right? I think it might be cleaner to use [is_member, member_index, 0, 0] instead and use the is_member boolean flag to check for presence instead. That way, we don't need to do arithmetic on the member_index.
The same may apply to active_role_index_plus_one, but I haven't checked yet.
| proc ensure_role_exists | ||
| dup eq.0 | ||
| # => [is_zero, role_symbol] | ||
|
|
||
| if.true | ||
| drop | ||
| # => [] | ||
| else |
There was a problem hiding this comment.
From what I can tell, we never get here when the role symbol is zero, so can we not assume that the symbol is non-zero (and state this in the docs)? Alternatively, we can also call assert_role_symbol_non_zero, but that is technically redundant.
| pub proc role_exists | ||
| exec.is_role_exists |
There was a problem hiding this comment.
Nit: Should we rename is_role_exists to role_exists_internal?
| # Default config for a newly created role. | ||
| # Layout: [member_count, admin_role_symbol, active_role_index_plus_one, exists_flag] | ||
| const DEFAULT_ROLE_CONFIG = [0, 0, 0, 1] |
There was a problem hiding this comment.
Question: Is there a meaningful difference between 1) a role exists and has 0 members and 2) the role does not exist? If not, I think we wouldn't need the exists_flag.
2 participants
The design is specified in the issue: #2685
Related Issues in OpenZeppelin library:
Dependend PR:
TokenSymboltoSymboland add newTokenSymbolandRoleSymboltypes #2690 (comment)Potential Use Cases:
Follow-up Items: