Skip to content
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

Adds the ability to add/remove admins and super admins #782

Merged
merged 11 commits into from
May 28, 2024

Conversation

cameronvoell
Copy link
Contributor

Part of #664

Depends on proto update:

This PR adds the following:

  • add / remove admin
  • add / remove super admin
  • admin_list super_admin_list
  • is_admin, is_super_admin
  • updated bindings
  • New rule => can't remove super admin from a group (super_admin can remove super_admin status from an inbox_id, including themselves, but only if there is at least 1 super admin still remaining in the group).

Before merging:

  • Set dev/gen_protos.sh back to main

@cameronvoell cameronvoell marked this pull request as ready for review May 28, 2024 14:36
@cameronvoell cameronvoell requested a review from a team as a code owner May 28, 2024 14:36

pub fn is_admin(&self, account_address: &String) -> Result<bool, GenericError> {
let admin_list = self.admin_list()?;
Ok(admin_list.contains(account_address))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait. Isn't the admin list going to be defined in terms of inbox_id, not account_address?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 updated a7cbdce

@@ -628,6 +628,7 @@ fn extract_metadata_changes(
immutable_metadata,
old_mutable_metadata,
),
num_super_admins: old_mutable_metadata.super_admin_list.len() as u32,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be the new_mutable_metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I had a check in a world where you can only remove 1 super admin at a time, that there must be more than 1 before you remove. But checking that the new mutable metadata has at least one super admin is the better option so that removing multiple superadmins would evaluate correctly as well. fixed here - d5d06f4

@cameronvoell cameronvoell merged commit c86272d into main May 28, 2024
6 checks passed
@cameronvoell cameronvoell deleted the cv/permissions-add-remove-admin-main branch May 28, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants