Repository: paritytech/substrate
File: frame/collective/src/lib.rs (Rust)

Code before:
	// Removes a proposal from the pallet, cleaning up votes and the vector of proposals.
	fn remove_proposal(proposal_hash: T::Hash) -> u32 {
		// remove proposal and vote
		ProposalOf::<T, I>::remove(&proposal_hash);
		Voting::<T, I>::remove(&proposal_hash);
		let num_proposals = Proposals::<T, I>::mutate(|proposals| {
			proposals.retain(|h| h != &proposal_hash);
			proposals.len() + 1 // calculate weight based on original length
		});
		num_proposals as u32
	}

	/// Ensure the correctness of the state of this pallet.
	///
	/// The following expectation must always apply.
	///
	/// ## Expectations:
	///
	/// Looking at proposals:
	///
	/// * Each hash of a proposal that is stored inside `Proposals` must have a
	/// call mapped to it inside the `ProposalsOf` storage map.
	/// * `ProposalCount` must always be more or equal to the number of
	/// proposals inside the `Proposals` storage value. The reason why
	/// `ProposalCount` can be more is because when a proposal is removed the
	/// count is not deducted.
	///
	/// Looking at votes:
	/// * The sum of aye and nay votes for a proposal can never exceed
	///  `MaxMembers`.
	/// * The proposal index inside the `Voting` storage map must be unique.
	///
	/// Looking at members:
	/// * The members count must never exceed `MaxMembers`.
	///
	/// Looking at prime account:
	/// * The prime account must be a member of the collective.
	#[cfg(any(feature = "try-runtime", test))]
	fn do_try_state() -> Result<(), &'static str> {
		Self::proposals().into_iter().for_each(|proposal| {
			assert!(Self::proposal_of(proposal).is_some());
		});

		assert!(Self::proposals().into_iter().count() <= Self::proposal_count() as usize);

		Self::proposals().into_iter().for_each(|proposal| {
			if let Some(votes) = Self::voting(proposal) {
				let ayes = votes.ayes.len();
				let nays = votes.nays.len();
				assert!(ayes.saturating_add(nays) <= T::MaxMembers::get() as usize);
			}

Review comment (suggestion):
I think another invariant to check is that the count of values in `ProposalsOf` matches the count of values in `Proposals`. Combined with the first check, this implicitly checks that each key (hash) in `ProposalsOf` exists in `Proposals`.

Code after:

		ensure!(
			Self::proposals().into_iter().count() <= Self::proposal_count() as usize,
			DispatchError::Other("The actual number of proposals is greater than `ProposalCount`")
		);
		ensure!(
			Self::proposals().into_iter().count() == <ProposalOf<T, I>>::iter_keys().count(),
			DispatchError::Other("Proposal count inside `Proposals` is not equal to the proposal count in `ProposalOf`")
		);

		Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult {
			if let Some(votes) = Self::voting(proposal) {
				let ayes = votes.ayes.len();
				let nays = votes.nays.len();

				ensure!(
					ayes.saturating_add(nays) <= T::MaxMembers::get() as usize,
					DispatchError::Other("The sum of ayes and nays is greater than `MaxMembers`")
				);
			}
			Ok(())
		})?;

		let mut proposal_indices = vec![];
		Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult {
			if let Some(votes) = Self::voting(proposal) {
				let proposal_index = votes.index;
				ensure!(
					!proposal_indices.contains(&proposal_index),
					DispatchError::Other("The proposal index is not unique.")
				);
				proposal_indices.push(proposal_index);
			}
			Ok(())
		})?;

		<Voting<T, I>>::iter_keys().try_for_each(|proposal_hash| -> DispatchResult {
			ensure!(
				Self::proposals().contains(&proposal_hash),
				DispatchError::Other(
					"`Proposals` doesn't contain the proposal hash from the `Voting` storage map."
				)
			);
			Ok(())
		})?;

		ensure!(
			Self::members().len() <= T::MaxMembers::get() as usize,
			DispatchError::Other("The member count is greater than `MaxMembers`.")
		);