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

Conversation

gavofyork
Copy link
Member

No description provided.

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Dec 5, 2019
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.

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? 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.

@bkchr bkchr merged commit e1c674c into master Dec 5, 2019
@bkchr bkchr deleted the gav-membership-change-key branch December 5, 2019 15:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants