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

Read members from pallet-membership #6041

Closed
xlc opened this issue May 15, 2020 · 3 comments · Fixed by #6518
Closed

Read members from pallet-membership #6041

xlc opened this issue May 15, 2020 · 3 comments · Fixed by #6518

Comments

@xlc
Copy link
Contributor

xlc commented May 15, 2020

pallet-membership offers ability to store and modify membership data. It provides hooks when membership changed, but does not offer any public interface to read membership data.

This means the user of pallet-membership is likely need to store the membership data again, like pallet-collective does here

pub Members get(fn members): Vec<T::AccountId>;

This is just waste space. There should be some kind of trait Membership to allow query membership data from pallet-membership directly without duplicate data.

@xlc
Copy link
Contributor Author

xlc commented May 29, 2020

Just found trait Contains. I see no reason we can't implement it for membership pallets.

/// A trait for querying whether a type can be said to "contain" a value.
pub trait Contains<T: Ord> {

@shawntabrizi
Copy link
Member

This is just waste space. There should be some kind of trait Membership to allow query membership data from pallet-membership directly without duplicate data.

I have discussed this with Gav before already, and the choice to store the members twice here is important for making these interfaces and pallets truely generic.

The order in which the members are stored, or any additional metadata that wants to be stored along side the members means that a single location for the list of members will not be efficient for the needs of other pallets. Collective is sorted by AccountId if I remember, but you could imagine another pallet that sorts the members by some deposit they placed, and also store this deposit along side the account in a tuple in the array.

The Substrate runtime modules do not currently use this abstraction, so in this specific case, yes it does seem like we are double storing items, but I think for the purposes of high level API design, this is intended.

@xlc
Copy link
Contributor Author

xlc commented Jun 26, 2020

I will argue the trade-off is not worth it for a feature that who knows when people may found useful. Unless someone have specific requirement for the more general version, it shouldn't be something we optimize for at current stage.

pallet-collective can just depends on trait Contains and call it a day. When people want to use pallet-collective in a different way, then we can start thinking if we should duplicate the storage.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants