fix(standards): prevent role delegation to a dead admin role from freezing role management - #3476
Conversation
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good!
This means we loose the ability for "irreversible finalization" of a role, i.e. a role without an effective admin. It seems like this may be useful in some cases, similar to renouncing ownership in Ownable2Step. If so, I think we may not need to add it right away, but we could open an issue to document the gap.
| push.ADMIN_ROLE swap | ||
| # => [is_memberless, ADMIN_ROLE, effective_admin_role_symbol] |
There was a problem hiding this comment.
| push.ADMIN_ROLE swap | |
| # => [is_memberless, ADMIN_ROLE, effective_admin_role_symbol] | |
| push.ADMIN_ROLE swap | |
| # => [is_memberless, admin_role, effective_admin_role_symbol] |
| dup exec.get_role_member_count_internal eq.0 | ||
| # => [is_memberless, effective_admin_role_symbol] | ||
|
|
||
| push.ADMIN_ROLE swap | ||
| # => [is_memberless, ADMIN_ROLE, effective_admin_role_symbol] |
There was a problem hiding this comment.
| dup exec.get_role_member_count_internal eq.0 | |
| # => [is_memberless, effective_admin_role_symbol] | |
| push.ADMIN_ROLE swap | |
| # => [is_memberless, ADMIN_ROLE, effective_admin_role_symbol] | |
| push.ADMIN_ROLE | |
| dup.1 exec.get_role_member_count_internal eq.0 | |
| # => [is_memberless, admin_role, effective_admin_role_symbol] |
nit: avoid unnecessary swap
| // MINTER is delegated to MINT_ADMIN, which has no members — an unpopulated or mistyped role. | ||
| let set_admin_note = build_note(admin, set_role_admin_script(&minter, Some(&mint_admin)))?; | ||
| let updated = execute_note_and_apply(&mock_chain, &account, &set_admin_note).await?; | ||
| assert_eq!(get_role_config(&updated, &mint_admin)?.0, Felt::ZERO); |
There was a problem hiding this comment.
nit: it's hard to read what this asserts just by looking at the code. Using a dedicated get_role_member_count helper (that wraps get_role_config) would be useful, or returning a test-only struct with public fields.
|
I somehow hadn't realized that we haven't merged this PR yet. I think the |
3 participants
Closes: #3475