From be6b03738e5819b4828209bc75e6edba3286cc14 Mon Sep 17 00:00:00 2001
From: Liam Aharon <liam.aharon@hotmail.com>
Date: Thu, 27 Apr 2023 18:55:01 +1000
Subject: [PATCH] collective pallet: sort genesis members and enforce max len
 constraint (#13988)

* insert members in sorted order

* improve variable name

* enforce genesis members length constraint
---
 substrate/frame/collective/src/lib.rs   |  6 ++++
 substrate/frame/collective/src/tests.rs | 40 ++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/substrate/frame/collective/src/lib.rs b/substrate/frame/collective/src/lib.rs
index 0ecf008fee..fd89a998ad 100644
--- a/substrate/frame/collective/src/lib.rs
+++ b/substrate/frame/collective/src/lib.rs
@@ -246,6 +246,10 @@ pub mod pallet {
 				self.members.len(),
 				"Members cannot contain duplicate accounts."
 			);
+			assert!(
+				self.members.len() <= T::MaxMembers::get() as usize,
+				"Members length cannot exceed MaxMembers.",
+			);
 
 			Pallet::<T, I>::initialize_members(&self.members)
 		}
@@ -1107,6 +1111,8 @@ impl<T: Config<I>, I: 'static> InitializeMembers<T::AccountId> for Pallet<T, I>
 	fn initialize_members(members: &[T::AccountId]) {
 		if !members.is_empty() {
 			assert!(<Members<T, I>>::get().is_empty(), "Members are already initialized!");
+			let mut members = members.to_vec();
+			members.sort();
 			<Members<T, I>>::put(members);
 		}
 	}
diff --git a/substrate/frame/collective/src/tests.rs b/substrate/frame/collective/src/tests.rs
index 4775133ffa..1165ebcec2 100644
--- a/substrate/frame/collective/src/tests.rs
+++ b/substrate/frame/collective/src/tests.rs
@@ -86,6 +86,7 @@ mod mock_democracy {
 }
 
 pub type MaxMembers = ConstU32<100>;
+type AccountId = u64;
 
 parameter_types! {
 	pub const MotionDuration: u64 = 3;
@@ -105,7 +106,7 @@ impl frame_system::Config for Test {
 	type RuntimeCall = RuntimeCall;
 	type Hash = H256;
 	type Hashing = BlakeTwo256;
-	type AccountId = u64;
+	type AccountId = AccountId;
 	type Lookup = IdentityLookup<Self::AccountId>;
 	type Header = Header;
 	type RuntimeEvent = RuntimeEvent;
@@ -161,19 +162,26 @@ impl Config for Test {
 	type MaxProposalWeight = MaxProposalWeight;
 }
 
-pub struct ExtBuilder {}
+pub struct ExtBuilder {
+	collective_members: Vec<AccountId>,
+}
 
 impl Default for ExtBuilder {
 	fn default() -> Self {
-		Self {}
+		Self { collective_members: vec![1, 2, 3] }
 	}
 }
 
 impl ExtBuilder {
+	fn set_collective_members(mut self, collective_members: Vec<AccountId>) -> Self {
+		self.collective_members = collective_members;
+		self
+	}
+
 	pub fn build(self) -> sp_io::TestExternalities {
 		let mut ext: sp_io::TestExternalities = GenesisConfig {
 			collective: pallet_collective::GenesisConfig {
-				members: vec![1, 2, 3],
+				members: self.collective_members,
 				phantom: Default::default(),
 			},
 			collective_majority: pallet_collective::GenesisConfig {
@@ -219,6 +227,17 @@ fn motions_basic_environment_works() {
 	});
 }
 
+#[test]
+fn initialize_members_sorts_members() {
+	let unsorted_members = vec![3, 2, 4, 1];
+	let expected_members = vec![1, 2, 3, 4];
+	ExtBuilder::default()
+		.set_collective_members(unsorted_members)
+		.build_and_execute(|| {
+			assert_eq!(Collective::members(), expected_members);
+		});
+}
+
 #[test]
 fn proposal_weight_limit_works() {
 	ExtBuilder::default().build_and_execute(|| {
@@ -1424,6 +1443,19 @@ fn disapprove_proposal_works() {
 	})
 }
 
+#[should_panic(expected = "Members length cannot exceed MaxMembers.")]
+#[test]
+fn genesis_build_panics_with_too_many_members() {
+	let max_members: u32 = MaxMembers::get();
+	let too_many_members = (1..=max_members as u64 + 1).collect::<Vec<AccountId>>();
+	pallet_collective::GenesisConfig::<Test> {
+		members: too_many_members,
+		phantom: Default::default(),
+	}
+	.build_storage()
+	.unwrap();
+}
+
 #[test]
 #[should_panic(expected = "Members cannot contain duplicate accounts.")]
 fn genesis_build_panics_with_duplicate_members() {