diff --git a/substrate/frame/alliance/src/lib.rs b/substrate/frame/alliance/src/lib.rs index 30cb5a6f2665..2ca9870fb3e3 100644 --- a/substrate/frame/alliance/src/lib.rs +++ b/substrate/frame/alliance/src/lib.rs @@ -418,6 +418,14 @@ pub mod pallet { pub phantom: PhantomData<(T, I)>, } + #[pallet::hooks] + impl, I: 'static> Hooks> for Pallet { + #[cfg(feature = "try-runtime")] + fn try_state(_n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { + Self::do_try_state() + } + } + #[pallet::genesis_build] impl, I: 'static> BuildGenesisConfig for GenesisConfig { fn build(&self) { @@ -1123,3 +1131,132 @@ impl, I: 'static> Pallet { Ok(info.into()) } } + +#[cfg(any(feature = "try-runtime", test))] +impl, I: 'static> Pallet { + /// Ensure the correctness of the state of this pallet. + /// + /// This should be valid before or after each state transition of this pallet. + pub fn do_try_state() -> Result<(), sp_runtime::TryRuntimeError> { + Self::try_state_members_are_disjoint()?; + Self::try_state_members_are_sorted()?; + Self::try_state_retiring_members_are_consistent()?; + Self::try_state_deposit_of_is_consistent()?; + Self::try_state_unscrupulous_items_are_sorted()?; + Self::try_state_announcements_are_sorted()?; + Ok(()) + } + + /// # Invariants + /// + /// * The sets of `Fellows`, and `Allies` members must be mutually exclusive. + /// An account cannot hold more than one role at a time. + fn try_state_members_are_disjoint() -> Result<(), sp_runtime::TryRuntimeError> { + let fellows = Members::::get(MemberRole::Fellow); + let allies = Members::::get(MemberRole::Ally); + + for fellow in fellows.iter() { + ensure!( + allies.binary_search(fellow).is_err(), + "Member is both Fellow and Ally" + ); + } + + + Ok(()) + } + + /// # Invariants + /// + /// * The list of members for each role (`Fellow`, `Ally`, `Retiring`) must be sorted by + /// `AccountId`. This is crucial for efficient lookups using binary search. + fn try_state_members_are_sorted() -> Result<(), sp_runtime::TryRuntimeError> { + let roles = [MemberRole::Fellow, MemberRole::Ally, MemberRole::Retiring]; + for role in roles.iter() { + let members = Members::::get(role); + let mut sorted_members = members.clone(); + sorted_members.sort(); + ensure!( + members == sorted_members, + "Members of a role are not sorted" + ); + } + Ok(()) + } + + /// # Invariants + /// + /// * The set of accounts in `RetiringMembers` storage must be identical to the set of + /// members with the `Retiring` role. + fn try_state_retiring_members_are_consistent() -> Result<(), sp_runtime::TryRuntimeError> { + let retiring_in_members = Members::::get(MemberRole::Retiring); + let retiring_keys_count = RetiringMembers::::iter_keys().count(); + + ensure!( + retiring_in_members.len() == retiring_keys_count, + "Count mismatch between Members and RetiringMembers map" + ); + + for member in retiring_in_members.iter() { + ensure!( + RetiringMembers::::contains_key(member), + "Retiring member not found in RetiringMembers map" + ); + } + + Ok(()) + } + + /// # Invariants + /// + /// * Every account that has a deposit stored in `DepositOf` must be a member of the + /// alliance (either a `Fellow`, `Ally`, or `Retiring`). + fn try_state_deposit_of_is_consistent() -> Result<(), sp_runtime::TryRuntimeError> { + for (who, _) in DepositOf::::iter() { + ensure!( + Self::is_member(&who), + "Account with deposit is not an alliance member" + ); + } + Ok(()) + } + + /// # Invariants + /// + /// * The lists of `UnscrupulousAccounts` and `UnscrupulousWebsites` must be sorted. This allows + /// for efficient binary search lookups. + fn try_state_unscrupulous_items_are_sorted() -> Result<(), sp_runtime::TryRuntimeError> { + let accounts = UnscrupulousAccounts::::get(); + let mut sorted_accounts = accounts.clone(); + sorted_accounts.sort(); + ensure!( + accounts == sorted_accounts, + "UnscrupulousAccounts is not sorted" + ); + + let websites = UnscrupulousWebsites::::get(); + let mut sorted_websites = websites.clone(); + sorted_websites.sort(); + ensure!( + websites == sorted_websites, + "UnscrupulousWebsites is not sorted" + ); + + Ok(()) + } + + /// # Invariants + /// + /// * The list of `Announcements` must be sorted. This is necessary because `remove_announcement` + /// uses binary search. + fn try_state_announcements_are_sorted() -> Result<(), sp_runtime::TryRuntimeError> { + let announcements = Announcements::::get(); + let mut sorted_announcements = announcements.clone(); + sorted_announcements.sort(); + ensure!( + announcements == sorted_announcements, + "Announcements is not sorted" + ); + Ok(()) + } +} diff --git a/substrate/frame/alliance/src/mock.rs b/substrate/frame/alliance/src/mock.rs index 861602aeb0ed..78d5094b1eee 100644 --- a/substrate/frame/alliance/src/mock.rs +++ b/substrate/frame/alliance/src/mock.rs @@ -384,6 +384,13 @@ pub fn new_test_ext() -> sp_io::TestExternalities { ext } +pub fn build_and_execute(test: impl FnOnce()) { + new_test_ext().execute_with(|| { + test(); + Alliance::do_try_state().expect("All invariants must hold after a test"); + }) +} + #[cfg(feature = "runtime-benchmarks")] pub fn new_bench_ext() -> sp_io::TestExternalities { RuntimeGenesisConfig::default().build_storage().unwrap().into() diff --git a/substrate/frame/alliance/src/tests.rs b/substrate/frame/alliance/src/tests.rs index 2397ebfe7db4..11f135cd2e53 100644 --- a/substrate/frame/alliance/src/tests.rs +++ b/substrate/frame/alliance/src/tests.rs @@ -61,7 +61,7 @@ fn assert_powerless(user: RuntimeOrigin, user_is_member: bool) { #[test] fn init_members_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { // alliance must be reset first, no witness data assert_noop!( Alliance::init_members(RuntimeOrigin::root(), vec![8], vec![],), @@ -105,7 +105,7 @@ fn init_members_works() { #[test] fn disband_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { let id_deposit = test_identity_info_deposit(); let expected_join_deposit = ::AllyDeposit::get(); assert_eq!(Balances::free_balance(9), 1000 - id_deposit); @@ -168,7 +168,7 @@ fn disband_works() { #[test] fn propose_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { let (proposal, proposal_len, hash) = make_remark_proposal(42); // only voting member can propose proposal, 4 is ally not have vote rights @@ -208,7 +208,7 @@ fn propose_works() { #[test] fn vote_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { let (proposal, proposal_len, hash) = make_remark_proposal(42); assert_ok!(Alliance::propose( RuntimeOrigin::signed(1), @@ -242,7 +242,7 @@ fn vote_works() { #[test] fn close_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { let (proposal, proposal_len, hash) = make_remark_proposal(42); let proposal_weight = proposal.get_dispatch_info().call_weight; assert_ok!(Alliance::propose( @@ -312,7 +312,7 @@ fn close_works() { #[test] fn set_rule_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { let cid = test_cid(); assert_ok!(Alliance::set_rule(RuntimeOrigin::signed(1), cid.clone())); assert_eq!(alliance::Rule::::get(), Some(cid.clone())); @@ -325,7 +325,7 @@ fn set_rule_works() { #[test] fn announce_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { let cid = test_cid(); assert_noop!(Alliance::announce(RuntimeOrigin::signed(2), cid.clone()), BadOrigin); @@ -341,7 +341,7 @@ fn announce_works() { #[test] fn remove_announcement_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { let cid = test_cid(); assert_ok!(Alliance::announce(RuntimeOrigin::signed(3), cid.clone())); assert_eq!(alliance::Announcements::::get(), vec![cid.clone()]); @@ -361,7 +361,7 @@ fn remove_announcement_works() { #[test] fn join_alliance_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { let id_deposit = test_identity_info_deposit(); let join_deposit = ::AllyDeposit::get(); assert_eq!(Balances::free_balance(9), 1000 - id_deposit); @@ -421,7 +421,7 @@ fn join_alliance_works() { #[test] fn nominate_ally_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { // check already member assert_noop!( Alliance::nominate_ally(RuntimeOrigin::signed(1), 2), @@ -476,7 +476,7 @@ fn nominate_ally_works() { #[test] fn elevate_ally_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { assert_noop!( Alliance::elevate_ally(RuntimeOrigin::signed(2), 4), Error::::NotAlly @@ -494,7 +494,7 @@ fn elevate_ally_works() { #[test] fn give_retirement_notice_work() { - new_test_ext().execute_with(|| { + build_and_execute(|| { assert_noop!( Alliance::give_retirement_notice(RuntimeOrigin::signed(4)), Error::::NotMember @@ -517,7 +517,7 @@ fn give_retirement_notice_work() { #[test] fn retire_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { assert_noop!( Alliance::retire(RuntimeOrigin::signed(2)), Error::::RetirementNoticeNotGiven @@ -551,7 +551,7 @@ fn retire_works() { #[test] fn abdicate_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { assert_eq!(alliance::Members::::get(MemberRole::Fellow), vec![1, 2, 3]); assert_ok!(Alliance::abdicate_fellow_status(RuntimeOrigin::signed(3))); @@ -565,7 +565,7 @@ fn abdicate_works() { #[test] fn kick_member_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { assert_noop!(Alliance::kick_member(RuntimeOrigin::signed(4), 4), BadOrigin); assert_noop!( @@ -587,7 +587,7 @@ fn kick_member_works() { #[test] fn add_unscrupulous_items_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { assert_noop!(Alliance::add_unscrupulous_items(RuntimeOrigin::signed(2), vec![]), BadOrigin); assert_ok!(Alliance::add_unscrupulous_items( @@ -615,7 +615,7 @@ fn add_unscrupulous_items_works() { #[test] fn remove_unscrupulous_items_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { assert_noop!( Alliance::remove_unscrupulous_items(RuntimeOrigin::signed(2), vec![]), BadOrigin