Resolve "OutgoingAuthorities event is triggered too many times." (!303)

* fix #243

* reveal that expires_on is non null

issue #243

* no need for event when no changes to authority

* add std for sp keyring

* deduplicate authorities events

pallets/authority-members/src/lib.rs

@@ -575,14 +575,25 @@ impl<T: Config> pallet_session::SessionManager<T::ValidatorId> for Pallet<T> {
             return None;
         }
 
+        // -- handle incoming members
+        // callback when smith is incoming
         for member_id in members_ids_to_add.iter() {
             T::OnIncomingMember::on_incoming_member(*member_id);
+        }
+        // a single event with all authorities if some
+        if !members_ids_to_add.is_empty() {
             Self::deposit_event(Event::IncomingAuthorities {
                 members: members_ids_to_add.clone(),
             });
         }
+
+        // -- handle outgoing members
+        // callback when smith is outgoing
         for member_id in members_ids_to_del.iter() {
             T::OnOutgoingMember::on_outgoing_member(*member_id);
+        }
+        // a single event with all authorities if some
+        if !members_ids_to_del.is_empty() {
             Self::deposit_event(Event::OutgoingAuthorities {
                 members: members_ids_to_del.clone(),
             });

pallets/smith-members/src/lib.rs

@@ -582,7 +582,7 @@ impl<T: Config> Pallet<T> {
                     if let Some(smith_meta) = maybe_smith_meta {
                         // As long as the smith is online, it cannot expire
                         smith_meta.expires_on = None;
-                        // FIXME: unschedule old expiry
+                        // FIXME: unschedule old expiry (#182)
                     }
                 });
             }
@@ -592,10 +592,12 @@ impl<T: Config> Pallet<T> {
     /// Handle the event when a Smith goes offline.
     pub fn on_smith_goes_offline(idty_index: T::IdtyIndex) {
         if let Some(smith_meta) = Smiths::<T>::get(idty_index) {
-            if smith_meta.expires_on.is_none() {
+            // Smith can go offline after main membership expiry
+            // in this case, there is no scheduled expiry since it is already excluded
+            if smith_meta.status != SmithStatus::Excluded {
                 Smiths::<T>::mutate(idty_index, |maybe_smith_meta| {
                     if let Some(smith_meta) = maybe_smith_meta {
-                        // As long as the smith is online, it cannot expire
+                        // schedule expiry
                         let new_expires_on =
                             CurrentSession::<T>::get() + T::SmithInactivityMaxDuration::get();
                         smith_meta.expires_on = Some(new_expires_on);

runtime/gdev/Cargo.toml

@@ -112,6 +112,7 @@ std = [
 	"sp-genesis-builder/std",
 	"sp-inherents/std",
 	"sp-io/std",
+	"sp-keyring/std",
 	"sp-membership/std",
 	"sp-offchain/std",
 	"sp-runtime/std",

runtime/gdev/tests/integration_tests.rs

@@ -25,6 +25,7 @@ use frame_support::{
 use gdev_runtime::*;
 use pallet_identity::{RevocationPayload, REVOCATION_PAYLOAD_PREFIX};
 use pallet_membership::MembershipRemovalReason;
+use pallet_session::historical::SessionManager;
 use pallet_smith_members::{SmithMeta, SmithStatus};
 use scale_info::prelude::num::NonZeroU16;
 use sp_core::{Encode, Pair};
@@ -1020,6 +1021,93 @@ fn test_smith_process() {
         })
 }
 
+// reveal bug from #243
+#[test]
+fn test_expired_smith_has_null_expires_on() {
+    // initial_authorities_len = 2 → Alice and Bob are online
+    // initial_smiths_len = 3 → Charlie is offline Smith
+    // initial_identities_len = 4 → Dave is member but not smith
+    ExtBuilder::new(2, 3, 4).build().execute_with(|| {
+        run_to_block(1);
+
+        // Bob is smith
+        assert_eq!(
+            SmithMembers::smiths(2),
+            Some(pallet_smith_members::SmithMeta {
+                status: SmithStatus::Smith,
+                expires_on: None, // because online
+                issued_certs: vec![1, 3],
+                received_certs: vec![1, 3],
+            })
+        );
+
+        // force Bob to leave by expiring his main WoT membership
+        Membership::do_remove_membership(2, MembershipRemovalReason::System);
+
+        // check events
+        // membership removal
+        System::assert_has_event(RuntimeEvent::Membership(
+            pallet_membership::Event::MembershipRemoved {
+                member: 2,
+                reason: MembershipRemovalReason::System,
+            },
+        ));
+        // smith membership removal
+        System::assert_has_event(RuntimeEvent::SmithMembers(
+            pallet_smith_members::Event::SmithMembershipRemoved { idty_index: 2 },
+        ));
+        System::assert_has_event(RuntimeEvent::AuthorityMembers(
+            pallet_authority_members::Event::MemberRemoved { member: 2 },
+        ));
+        // also events for certifications
+
+        // check state
+        // Bob is not Smith anymore
+        assert_eq!(
+            SmithMembers::smiths(2),
+            Some(pallet_smith_members::SmithMeta {
+                status: SmithStatus::Excluded, // automatically excluded
+                expires_on: None,              // because excluded, no expiry is scheduled
+                issued_certs: vec![1, 3],
+                received_certs: vec![], // received certs are deleted
+            })
+        );
+        // Alice smith cert to Bob has been deleted
+        assert_eq!(
+            SmithMembers::smiths(1),
+            Some(pallet_smith_members::SmithMeta {
+                status: SmithStatus::Smith,
+                expires_on: None,      // because online
+                issued_certs: vec![3], // cert to Bob has been deleted
+                received_certs: vec![2, 3],
+            })
+        );
+
+        // run to next block
+        run_to_block(2);
+
+        // simulate new session
+        AuthorityMembers::new_session(2);
+        // check event
+        System::assert_has_event(RuntimeEvent::AuthorityMembers(
+            pallet_authority_members::Event::OutgoingAuthorities { members: vec![2] },
+        ));
+
+        // control state is still ok
+        assert_eq!(
+            SmithMembers::smiths(2),
+            Some(pallet_smith_members::SmithMeta {
+                status: SmithStatus::Excluded, // still excluded
+                expires_on: None,              // should be still None
+                issued_certs: vec![1, 3],
+                received_certs: vec![],
+            })
+        );
+
+        // println!("{:#?}", System::events()); // with -- --nocapture
+    })
+}
+
 /// test new account creation
 #[test]
 fn test_create_new_account() {