Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Allow members of groups to change their key #4300

Merged
merged 1 commit into from
Dec 5, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion frame/membership/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use support::{
traits::{ChangeMembers, InitializeMembers},
weights::SimpleDispatchInfo,
};
use system::ensure_root;
use system::{ensure_root, ensure_signed};
use sp_runtime::traits::EnsureOrigin;

pub trait Trait<I=DefaultInstance>: system::Trait {
Expand Down Expand Up @@ -86,6 +86,8 @@ decl_event!(
MembersSwapped,
/// The membership was reset; see the transaction for who the new set is.
MembersReset,
/// One of the members' keys changed.
KeyChanged,
/// Phantom member, never used.
Dummy(rstd::marker::PhantomData<(AccountId, Event)>),
}
Expand Down Expand Up @@ -186,6 +188,31 @@ decl_module! {

Self::deposit_event(RawEvent::MembersReset);
}

/// Swap out the sending member for some other key `new`.
///
/// May only be called from `Signed` origin of a current member.
#[weight = SimpleDispatchInfo::FixedNormal(50_000)]
fn change_key(origin, new: T::AccountId) {
let remove = ensure_signed(origin)?;

if remove != new {
let mut members = <Members<T, I>>::get();
let location = members.binary_search(&remove).ok().ok_or("not a member")?;
members[location] = new.clone();
let _location = members.binary_search(&new).err().ok_or("already a member")?;
Copy link
Member

Choose a reason for hiding this comment

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

hmm, shouldn't this be performed after sorting?

After removed gets replaced with new the Vec is not necessarily sorted?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm? The vector is sorted after inserting the new key?

When you insert the new key, the sorting is very likely not the same as before.

Copy link
Member

Choose a reason for hiding this comment

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

I missed that the let _location = members.binary_search(&new).err().ok_or("already a member")?; expected it be an error sorry about that.

I still find Err(already a member)confusing because if got [10, 20, 30] and want to replace 30 -> 40 it will end up in error: already a member

Am I misunderstanding something?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh yeah! Fuck that is right, we should search before inserting.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, now I understand why the test works. Yep, that is an error.

members.sort();
<Members<T, I>>::put(&members);

T::MembershipChanged::change_members_sorted(
&[new],
&[remove],
&members[..],
);
}

Self::deposit_event(RawEvent::KeyChanged);
}
}
}

Expand Down Expand Up @@ -333,6 +360,17 @@ mod tests {
});
}

#[test]
fn change_key_works() {
new_test_ext().execute_with(|| {
assert_noop!(Membership::change_key(Origin::signed(3), 25), "not a member");
assert_noop!(Membership::change_key(Origin::signed(10), 20), "already a member");
assert_ok!(Membership::change_key(Origin::signed(10), 40));
assert_eq!(Membership::members(), vec![20, 30, 40]);
assert_eq!(MEMBERS.with(|m| m.borrow().clone()), Membership::members());
});
}

#[test]
fn reset_members_works() {
new_test_ext().execute_with(|| {
Expand Down