ronantakizawa/codereview-bench huggingface dataset:
Repository: paritytech/substrate
File: frame/utility/src/benchmarking.rs (Rust)

Code before:

	as_derivative {
		let caller = account("caller", SEED, SEED);
		let call = Box::new(frame_system::Call::remark { remark: vec![] }.into());
		// Whitelist caller account from further DB operations.
		let caller_key = frame_system::Account::<T>::hashed_key_for(&caller);
		frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
	}: _(RawOrigin::Signed(caller), SEED as u16, call)

	batch_all {
		let c in 0 .. 1000;
		let mut calls: Vec<<T as Config>::Call> = Vec::new();
		for i in 0 .. c {
			let call = frame_system::Call::remark { remark: vec![] }.into();
			calls.push(call);
		}
		let caller = whitelisted_caller();
	}: _(RawOrigin::Signed(caller), calls)
	verify {
		assert_last_event::<T>(Event::BatchCompleted.into())
	}

	dispatch_as {
		let caller = account("caller", SEED, SEED);
		let call = Box::new(frame_system::Call::remark { remark: vec![] }.into());
		let origin : T::Origin = RawOrigin::Signed(caller).into();
		let pallets_origin: <T::Origin as frame_support::traits::OriginTrait>::PalletsOrigin = origin.caller().clone();
		let pallets_origin = Into::<T::PalletsOrigin>::into(pallets_origin.clone());
	}: _(RawOrigin::Root, Box::new(pallets_origin), call)

	impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::tests::Test);
}


Review comment (suggestion):
```suggestion
		let origin: T::Origin = RawOrigin::Signed(caller).into();
```

Code after:
	}

	as_derivative {
		let caller = account("caller", SEED, SEED);
		let call = Box::new(frame_system::Call::remark { remark: vec![] }.into());
		// Whitelist caller account from further DB operations.
		let caller_key = frame_system::Account::<T>::hashed_key_for(&caller);
		frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
	}: _(RawOrigin::Signed(caller), SEED as u16, call)

	batch_all {
		let c in 0 .. 1000;
		let mut calls: Vec<<T as Config>::Call> = Vec::new();
		for i in 0 .. c {
			let call = frame_system::Call::remark { remark: vec![] }.into();
			calls.push(call);
		}
		let caller = whitelisted_caller();
	}: _(RawOrigin::Signed(caller), calls)
	verify {
		assert_last_event::<T>(Event::BatchCompleted.into())
	}

	dispatch_as {
		let caller = account("caller", SEED, SEED);
		let call = Box::new(frame_system::Call::remark { remark: vec![] }.into());
		let origin: T::Origin = RawOrigin::Signed(caller).into();
		let pallets_origin: <T::Origin as frame_support::traits::OriginTrait>::PalletsOrigin = origin.caller().clone();
		let pallets_origin = Into::<T::PalletsOrigin>::into(pallets_origin.clone());
	}: _(RawOrigin::Root, Box::new(pallets_origin), call)

	impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::tests::Test);
}
Repository: paritytech/substrate
File: primitives/staking/src/lib.rs (Rust)

Code before:

/// Simple index type with which we can count sessions.
pub type SessionIndex = u32;

/// Counter for the number of eras that have passed.
pub type EraIndex = u32;

/// Trait describing something that implements a hook for any operations to perform when a staker is
/// slashed.
pub trait OnStakerSlash<AccountId, Balance> {
	/// A hook for any operations to perform when a staker is slashed.
	///
	/// # Arguments
	///
	/// * `stash` - The stash of the staker whom the slash was applied to.
	/// * `slashed_bonded` - The new bonded balance of the staker after the slash was applied.
	/// * `slashed_unlocking` - A map from eras that the staker is unbonding in to the new balance
	///   after the slash was applied.
	fn on_slash(
		stash: &AccountId,
		slashed_bonded: Balance,
		slashed_unlocking: &BTreeMap<EraIndex, Balance>,
	);
}

impl<AccountId, Balance> OnStakerSlash<AccountId, Balance> for () {
	fn on_slash(_: &AccountId, _: Balance, _: &BTreeMap<EraIndex, Balance>) {
		// Nothing to do here
	}
}


Review comment (suggestion):
```suggestion
#[cfg(feature = "std")]
impl<AccountId, Balance> OnStakerSlash<AccountId, Balance> for () {
```

Code after:

/// Simple index type with which we can count sessions.
pub type SessionIndex = u32;

/// Counter for the number of eras that have passed.
pub type EraIndex = u32;

/// Trait describing something that implements a hook for any operations to perform when a staker is
/// slashed.
pub trait OnStakerSlash<AccountId, Balance> {
	/// A hook for any operations to perform when a staker is slashed.
	///
	/// # Arguments
	///
	/// * `stash` - The stash of the staker whom the slash was applied to.
	/// * `slashed_active` - The new bonded balance of the staker after the slash was applied.
	/// * `slashed_unlocking` - a map of slashed eras, and the balance of that unlocking chunk after
	///   the slash is applied. Any era not present in the map is not affected at all.
	fn on_slash(
		stash: &AccountId,
		slashed_active: Balance,
		slashed_unlocking: &BTreeMap<EraIndex, Balance>,
	);
}

impl<AccountId, Balance> OnStakerSlash<AccountId, Balance> for () {
	fn on_slash(_: &AccountId, _: Balance, _: &BTreeMap<EraIndex, Balance>) {
		// Nothing to do here
	}
}
Repository: paritytech/substrate
File: client/network/sync/src/block_request_handler.rs (Rust)

Code before:
				direction,
				max_blocks,
				support_multiple_justifications,
			)?;

			// If any of the blocks contains any data, we can consider it as successful request.
			if block_response
				.blocks
				.iter()
				.any(|b| !b.header.is_empty() || !b.body.is_empty() || b.is_empty_justification)
			{
				if let Some(value) = self.seen_requests.get_mut(&key) {
					// If this is the first time we have processed this request, we need to change
					// it to `Fulfilled`.
					if let SeenRequestsValue::First = value {
						*value = SeenRequestsValue::Fulfilled(1);
					}
				}
			}

			Some(block_response)
		} else {
			None
		};

		debug!(
			target: LOG_TARGET,
			"Sending result of block request from {peer} starting at `{from_block_id:?}`: \
			blocks: {:?}, data: {:?}",
			maybe_block_response.as_ref().map(|res| res.blocks.len()),
			maybe_block_response.as_ref().map(|res| res.encoded_len()),
		);

		let result = if let Some(block_response) = maybe_block_response {
			let mut data = Vec::with_capacity(block_response.encoded_len());
			block_response.encode(&mut data)?;
			Ok(data)
		} else {
			Err(())
		};

		pending_response
			.send(OutgoingResponse {
				result,
				reputation_changes: reputation_change.into_iter().collect(),
				sent_feedback: None,
			})
			.map_err(|_| HandleRequestError::SendResponse)
	}

	fn get_block_response(

Review comment (suggestion):
This should be put into the `if` branch? Because we don't send any block response in the `None` case? We can instead add a second logging that says that we don't send any response.

Code after:
				support_multiple_justifications,
			)?;

			// If any of the blocks contains any data, we can consider it as successful request.
			if block_response
				.blocks
				.iter()
				.any(|b| !b.header.is_empty() || !b.body.is_empty() || b.is_empty_justification)
			{
				if let Some(value) = self.seen_requests.get_mut(&key) {
					// If this is the first time we have processed this request, we need to change
					// it to `Fulfilled`.
					if let SeenRequestsValue::First = value {
						*value = SeenRequestsValue::Fulfilled(1);
					}
				}
			}

			Some(block_response)
		} else {
			None
		};

		debug!(
			target: LOG_TARGET,
			"Sending result of block request from {peer} starting at `{from_block_id:?}`: \
			blocks: {:?}, data: {:?}",
			maybe_block_response.as_ref().map(|res| res.blocks.len()),
			maybe_block_response.as_ref().map(|res| res.encoded_len()),
		);

		let result = if let Some(block_response) = maybe_block_response {
			let mut data = Vec::with_capacity(block_response.encoded_len());
			block_response.encode(&mut data)?;
			Ok(data)
		} else {
			Err(())
		};

		pending_response
			.send(OutgoingResponse {
				result,
				reputation_changes: reputation_change.into_iter().collect(),
				sent_feedback: None,
			})
			.map_err(|_| HandleRequestError::SendResponse)
	}

	fn get_block_response(
		&self,
		attributes: BlockAttributes,
Repository: paritytech/substrate
File: frame/support/src/traits/filter.rs (Rust)

Code before:
				}
			}

			impl FilterStack<$call> for $target {
				type Stack = Vec<Box<dyn Fn(&$call) -> bool + 'static>>;
				fn push(f: impl Fn(&$call) -> bool + 'static) {
					FILTER.0.borrow_mut().push(Box::new(f));
				}
				fn pop() {
					FILTER.0.borrow_mut().pop();
				}
				fn take() -> Self::Stack {
					take(FILTER.0.borrow_mut().as_mut())
				}
				fn restore(mut s: Self::Stack) {
					swap(FILTER.0.borrow_mut().as_mut(), &mut s);
				}
			}
		}
	}
}

/// Type that provide some integrity tests.
///
/// This implemented for modules by `decl_module`.
#[impl_trait_for_tuples::impl_for_tuples(100)]
pub trait IntegrityTest {
	/// Run integrity test.
	///
	/// The test is not executed in a externalities provided environment.
	fn integrity_test() {}
}

#[cfg(test)]
pub mod test_impl_filter_stack {
	use super::*;

	pub struct IsCallable;
	pub struct BaseFilter;
	impl Contains<u32> for BaseFilter {
		fn contains(x: &u32) -> bool {
			x % 2 == 0
		}
	}
	impl_filter_stack!(
		crate::traits::filter::test_impl_filter_stack::IsCallable,
		crate::traits::filter::test_impl_filter_stack::BaseFilter,
		u32,
		is_callable
	);


Review comment (suggestion):
Can you do some simple comparison of `cargo build -p frame-support` between 30 and 100 as parameters here. 

Code after:
				}
			}

			impl FilterStack<$call> for $target {
				type Stack = Vec<Box<dyn Fn(&$call) -> bool + 'static>>;
				fn push(f: impl Fn(&$call) -> bool + 'static) {
					FILTER.0.borrow_mut().push(Box::new(f));
				}
				fn pop() {
					FILTER.0.borrow_mut().pop();
				}
				fn take() -> Self::Stack {
					take(FILTER.0.borrow_mut().as_mut())
				}
				fn restore(mut s: Self::Stack) {
					swap(FILTER.0.borrow_mut().as_mut(), &mut s);
				}
			}
		}
	}
}

#[cfg(test)]
pub mod test_impl_filter_stack {
	use super::*;

	pub struct IsCallable;
	pub struct BaseFilter;
	impl Contains<u32> for BaseFilter {
		fn contains(x: &u32) -> bool {
			x % 2 == 0
		}
	}
	impl_filter_stack!(
		crate::traits::filter::test_impl_filter_stack::IsCallable,
		crate::traits::filter::test_impl_filter_stack::BaseFilter,
		u32,
		is_callable
	);

	#[test]
	fn impl_filter_stack_should_work() {
		assert!(IsCallable::contains(&36));
		assert!(IsCallable::contains(&40));
		assert!(IsCallable::contains(&42));
		assert!(!IsCallable::contains(&43));

		IsCallable::push(|x| *x < 42);
		assert!(IsCallable::contains(&36));
		assert!(IsCallable::contains(&40));
		assert!(!IsCallable::contains(&42));
Repository: paritytech/substrate
File: utils/frame/benchmarking-cli/src/storage/write.rs (Rust)

Code before:
						'child_retry: loop {
							rng.fill_bytes(&mut new_v[..]);
							match check_new_value::<Block>(
								db.clone(),
								&trie,
								&ck.0,
								&new_v,
								self.state_version(),
								state_col,
								Some(&info),
							) {
								true => break,
								false => continue 'child_retry,
							}
						}

						let res = calculate_bench::<Block>(
							db.clone(),
							&trie,
							ck.0,
							new_v.to_vec(),
							self.state_version(),
							state_col,
							Some(&info),
						)?;
						record.append(res.0, res.1)?;
					}
				}
			}
		}

		Ok(record)
	}
}

/// Converts a Trie transaction into a DB transaction.
/// Removals are ignored and will not be included in the final tx.
/// `invert_inserts` replaces all inserts with removals.
fn convert_tx<B: BlockT>(
	db: Arc<dyn sp_database::Database<DbHash>>,
	mut tx: PrefixedMemoryDB<HashFor<B>>,
	invert_inserts: bool,
	col: ColumnId,
) -> Transaction<DbHash> {
	let mut ret = Transaction::<DbHash>::default();

	for (mut k, (v, rc)) in tx.drain().into_iter() {
		if rc > 0 {
			db.sanitize_key(&mut k);
			if invert_inserts {
				ret.remove(col, &k);

Review comment (suggestion):
In the future we can split up the results into a `top_record` and `child_record` and then have two resulting weight constants.  
Then the developer can decide how much he wants to use them. But for now it should be good like this.  



Code after:
							&key.0,
							&new_v,
							self.state_version(),
							state_col,
							Some(&info),
						) {
							break
						}
					}

					let (size, duration) = measure_write::<Block>(
						db.clone(),
						&trie,
						key.0,
						new_v.to_vec(),
						self.state_version(),
						state_col,
						Some(&info),
					)?;
					record.append(size, duration)?;
				}
			}
		}

		Ok(record)
	}
}

/// Converts a Trie transaction into a DB transaction.
/// Removals are ignored and will not be included in the final tx.
/// `invert_inserts` replaces all inserts with removals.
fn convert_tx<B: BlockT>(
	db: Arc<dyn sp_database::Database<DbHash>>,
	mut tx: PrefixedMemoryDB<HashFor<B>>,
	invert_inserts: bool,
	col: ColumnId,
) -> Transaction<DbHash> {
	let mut ret = Transaction::<DbHash>::default();

	for (mut k, (v, rc)) in tx.drain().into_iter() {
		if rc > 0 {
			db.sanitize_key(&mut k);
			if invert_inserts {
				ret.remove(col, &k);
			} else {
				ret.set(col, &k, &v);
			}
		}
		// < 0 means removal - ignored.
		// 0 means no modification.
	}
Repository: paritytech/substrate
File: frame/contracts/src/wasm/mod.rs (Rust)

Code before:
			(i32.eq
				(i32.load (i32.const 32))
				(i32.const 8)
			)
		)

		;; return gas left
		(call $seal_return (i32.const 0) (i32.const 0) (i32.const 8))

		(unreachable)
	)
	(func (export "deploy"))
)
"#;

	#[test]
	fn gas_left() {
		let mut ext = MockExt::default();
		let gas_limit = ext.gas_meter.gas_left();

		let output = execute(CODE_GAS_LEFT, vec![], &mut ext).unwrap();

		let gas_left = Weight::decode(&mut &*output.data).unwrap();
		let actual_left = ext.gas_meter.gas_left();
		assert!(gas_left.any_lt(gas_limit), "gas_left must be less than initial");
		assert!(gas_left.any_gt(actual_left), "gas_left must be greater than final");
	}

	const CODE_VALUE_TRANSFERRED: &str = r#"
(module
	(import "seal0" "seal_value_transferred" (func $seal_value_transferred (param i32 i32)))
	(import "env" "memory" (memory 1 1))

	;; size of our buffer is 32 bytes
	(data (i32.const 32) "\20")

	(func $assert (param i32)
		(block $ok
			(br_if $ok
				(get_local 0)
			)
			(unreachable)
		)
	)

	(func (export "call")
		;; This stores the value transferred in the buffer
		(call $seal_value_transferred (i32.const 0) (i32.const 32))

		;; assert len == 8
		(call $assert

Review comment (suggestion):
I think just calling a contract should reduce any resource.

```suggestion
		assert!(gas_left.all_lt(gas_limit), "gas_left must be less than initial");
		assert!(gas_left.all_gt(actual_left), "gas_left must be greater than final");
```

Code after:
			(i32.eq
				(i32.load (i32.const 32))
				(i32.const 8)
			)
		)

		;; return gas left
		(call $seal_return (i32.const 0) (i32.const 0) (i32.const 8))

		(unreachable)
	)
	(func (export "deploy"))
)
"#;

	#[test]
	fn gas_left() {
		let mut ext = MockExt::default();
		let gas_limit = ext.gas_meter.gas_left();

		let output = execute(CODE_GAS_LEFT, vec![], &mut ext).unwrap();

		let gas_left = Weight::decode(&mut &*output.data).unwrap();
		let actual_left = ext.gas_meter.gas_left();
		assert!(gas_left.all_lt(gas_limit), "gas_left must be less than initial");
		assert!(gas_left.all_gt(actual_left), "gas_left must be greater than final");
	}

	const CODE_VALUE_TRANSFERRED: &str = r#"
(module
	(import "seal0" "seal_value_transferred" (func $seal_value_transferred (param i32 i32)))
	(import "env" "memory" (memory 1 1))

	;; size of our buffer is 32 bytes
	(data (i32.const 32) "\20")

	(func $assert (param i32)
		(block $ok
			(br_if $ok
				(get_local 0)
			)
			(unreachable)
		)
	)

	(func (export "call")
		;; This stores the value transferred in the buffer
		(call $seal_value_transferred (i32.const 0) (i32.const 32))

		;; assert len == 8
		(call $assert
Repository: paritytech/substrate
File: frame/support/src/traits/storage.rs (Rust)

Code before:
	fn partial_storage_info() -> Vec<StorageInfo>;
}

/// Allows a pallet to specify storage keys to whitelist during benchmarking.
/// This means those keys will be excluded from the benchmarking performance
/// calculation.
pub trait WhitelistedStorageKeys {
	/// Returns a [`Vec<TrackedStorageKey>`] indicating the storage keys that
	/// should be whitelisted during benchmarking. This means that those keys
	/// will be excluded from the benchmarking performance calculation.
	fn whitelisted_storage_keys() -> Vec<TrackedStorageKey>;
}

#[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))]
#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))]
#[cfg_attr(feature = "tuples-128", impl_for_tuples(128))]
impl WhitelistedStorageKeys for Tuple {
	fn whitelisted_storage_keys() -> Vec<TrackedStorageKey> {
		// de-duplicate the storage keys
		let mut combined_keys: BTreeSet<TrackedStorageKey> = BTreeSet::new();
		for_tuples!( #(
			for storage_key in Tuple::whitelisted_storage_keys() {
				combined_keys.insert(storage_key);
			}
		 )* );
		combined_keys.iter().cloned().collect::<Vec<_>>()
	}
}


Review comment (suggestion):
```suggestion
		combined_keys.into_iter().collect::<Vec<_>>()
```

Code after:
	fn partial_storage_info() -> Vec<StorageInfo>;
}

/// Allows a pallet to specify storage keys to whitelist during benchmarking.
/// This means those keys will be excluded from the benchmarking performance
/// calculation.
pub trait WhitelistedStorageKeys {
	/// Returns a [`Vec<TrackedStorageKey>`] indicating the storage keys that
	/// should be whitelisted during benchmarking. This means that those keys
	/// will be excluded from the benchmarking performance calculation.
	fn whitelisted_storage_keys() -> Vec<TrackedStorageKey>;
}

#[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))]
#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))]
#[cfg_attr(feature = "tuples-128", impl_for_tuples(128))]
impl WhitelistedStorageKeys for Tuple {
	fn whitelisted_storage_keys() -> Vec<TrackedStorageKey> {
		// de-duplicate the storage keys
		let mut combined_keys: BTreeSet<TrackedStorageKey> = BTreeSet::new();
		for_tuples!( #(
			for storage_key in Tuple::whitelisted_storage_keys() {
				combined_keys.insert(storage_key);
			}
		 )* );
		combined_keys.into_iter().collect::<Vec<_>>()
	}
}
Repository: paritytech/substrate
File: primitives/beefy/src/lib.rs (Rust)

Code before:
		let set_id = 0;
		let validators = ValidatorSet::<Public>::new(vec![alice.public()], set_id).unwrap();

		assert_eq!(validators.id(), set_id);
		assert_eq!(validators.validators(), &vec![alice.public()]);
	}

	#[test]
	fn beefy_verify_works() {
		let msg = &b"test-message"[..];
		let (pair, _) = crypto::Pair::generate();

		let signature: crypto::Signature =
			pair.as_inner_ref().sign_prehashed(&keccak_256(msg)).into();

		// Verification works
		assert!(BeefyVerify::<Keccak256, _, Identity>::verify(&signature, msg, &pair.public()));
		// Other Hashing fn doesn't work
		assert!(!BeefyVerify::<BlakeTwo256, _, Identity>::verify(&signature, msg, &pair.public()));
		// Other public key doesn't work
		let (other_pair, _) = crypto::Pair::generate();
		assert!(!BeefyVerify::<Keccak256, _, Identity>::verify(
			&signature,
			msg,
			&other_pair.public()
		));
	}
}


Review comment (bug):
would cover full ground by testing vice-versa with blake2, that is
```suggestion
		let signature_blake2_256: crypto::Signature =
			pair.as_inner_ref().sign_prehashed(&blake2_256(msg)).into();

		// Verification works if same hashing function is used when signing and verifying.
		assert!(BeefyVerify::<Keccak256, _, Identity>::verify(&signature, msg, &pair.public()));
		assert!(BeefyVerify::<BlakeTwo256, _, Identity>::verify(&signature_blake2_256, msg, &pair.public()));
		// Verification fails if distinct hashing functions are used when signing and verifying.
		assert!(!BeefyVerify::<Keccak256, _, Identity>::verify(&signature_blake2_256, msg, &pair.public()));
		assert!(!BeefyVerify::<BlakeTwo256, _, Identity>::verify(&signature, msg, &pair.public()));

		// Other public key doesn't work
		let (other_pair, _) = crypto::Pair::generate();
		assert!(!BeefyVerify::<Keccak256, _, Identity>::verify(
			&signature,
			msg,
			&other_pair.public()
		));
		assert!(!BeefyVerify::<BlakeTwo256, _, Identity>::verify(
			&signature_blake2_256,
			msg,
			&other_pair.public()
		));
```

Code after:

		let blake2_256_signature: crypto::Signature =
			pair.as_inner_ref().sign_prehashed(&blake2_256(msg)).into();

		// Verification works if same hashing function is used when signing and verifying.
		assert!(BeefyVerify::<Keccak256>::verify(&keccak_256_signature, msg, &pair.public()));
		assert!(BeefyVerify::<BlakeTwo256>::verify(&blake2_256_signature, msg, &pair.public()));
		// Verification fails if distinct hashing functions are used when signing and verifying.
		assert!(!BeefyVerify::<Keccak256>::verify(&blake2_256_signature, msg, &pair.public()));
		assert!(!BeefyVerify::<BlakeTwo256>::verify(&keccak_256_signature, msg, &pair.public()));

		// Other public key doesn't work
		let (other_pair, _) = crypto::Pair::generate();
		assert!(!BeefyVerify::<Keccak256>::verify(
			&keccak_256_signature,
			msg,
			&other_pair.public()
		));
		assert!(!BeefyVerify::<BlakeTwo256>::verify(
			&blake2_256_signature,
			msg,
			&other_pair.public()
		));
	}
}
Repository: paritytech/substrate
File: frame/nfts/src/features/roles.rs (Rust)

Code before:

use crate::*;
use sp_std::collections::btree_map::BTreeMap;

impl<T: Config<I>, I: 'static> Pallet<T, I> {
	pub(crate) fn has_role(
		collection_id: &T::CollectionId,
		account_id: &T::AccountId,
		role: CollectionRole,
	) -> bool {
		CollectionRoleOf::<T, I>::get(&collection_id, &account_id)
			.map_or(false, |roles| roles.has_role(role))
	}

	/// Groups provided roles by account, give one account could have multiple roles.
	///
	/// - `input`: A vector of (Account, Role) tuples.
	///
	/// Returns a grouped vector.
	pub fn group_roles_by_account(
		input: Vec<(T::AccountId, CollectionRole)>,
	) -> Vec<(T::AccountId, CollectionRoles)> {
		let mut result = BTreeMap::new();
		for (account, role) in input.into_iter() {
			let roles = result.entry(account).or_insert(CollectionRoles::none());
			roles.add_role(role);
		}
		result.into_iter().collect()
	}
}


Review comment (suggestion):
```suggestion
			result.entry(account).or_insert(CollectionRoles::none()).add_role(role);
```

Code after:

use crate::*;
use frame_support::pallet_prelude::*;
use sp_std::collections::btree_map::BTreeMap;

impl<T: Config<I>, I: 'static> Pallet<T, I> {
	/// Clears all the roles in a specified collection.
	///
	/// - `collection_id`: A collection to clear the roles in.
	///
	/// Throws an error if some of the roles were left in storage.
	/// This means the `CollectionRoles::max_roles()` needs to be adjusted.
	pub(crate) fn clear_roles(collection_id: &T::CollectionId) -> Result<(), DispatchError> {
		let res = CollectionRoleOf::<T, I>::clear_prefix(
			&collection_id,
			CollectionRoles::max_roles() as u32,
			None,
		);
		ensure!(res.maybe_cursor.is_none(), Error::<T, I>::RolesNotCleared);
		Ok(())
	}

	/// Returns true if a specified account has a provided role within that collection.
	///
	/// - `collection_id`: A collection to check the role in.
	/// - `account_id`: An account to check the role for.
	/// - `role`: A role to validate.
	///
	/// Returns boolean.
	pub(crate) fn has_role(
		collection_id: &T::CollectionId,
		account_id: &T::AccountId,
		role: CollectionRole,
	) -> bool {
		CollectionRoleOf::<T, I>::get(&collection_id, &account_id)
			.map_or(false, |roles| roles.has_role(role))
	}

	/// Groups provided roles by account, given one account could have multiple roles.
	///
	/// - `input`: A vector of (Account, Role) tuples.
	///
	/// Returns a grouped vector.
	pub fn group_roles_by_account(
		input: Vec<(T::AccountId, CollectionRole)>,
	) -> Vec<(T::AccountId, CollectionRoles)> {
		let mut result = BTreeMap::new();
		for (account, role) in input.into_iter() {
			result.entry(account).or_insert(CollectionRoles::none()).add_role(role);
		}
		result.into_iter().collect()