Resolve #176 "PromotedToSmith is issued even for Smith" (!232)

* fix(#176): last certification does postpone expiry

* test(#176): reveal that last smith certification does not postpone expiry

* fix(#176): clippy

* fix(#176): ExpiresOn is now correctly updated for Pending smiths

* test(#176): show that ExpiresOn is not correctly updated for Pending smiths

* refac(#176): move SmithCertAdded event closer to its storage updates + add docs

* fix(#176): expiration should be postponed only for Pending smiths

* test(#176): reveal that a ceritification on a online smith is bugged

* fix(#176): review: comment

* fix(#176): avoid emitting SmithMembershipAdded twice

* test(#176): reveal bug

pallets/smith-members/src/lib.rs

@@ -430,6 +430,7 @@ impl<T: Config> Pallet<T> {
     }
 
     fn do_certify_smith(receiver: T::IdtyIndex, issuer: T::IdtyIndex) {
+        // - adds a certification in issuer issued list
         Smiths::<T>::mutate(issuer, |maybe_smith_meta| {
             if let Some(smith_meta) = maybe_smith_meta {
                 smith_meta.issued_certs.push(receiver);
@@ -438,25 +439,39 @@ impl<T: Config> Pallet<T> {
         });
         Smiths::<T>::mutate(receiver, |maybe_smith_meta| {
             if let Some(smith_meta) = maybe_smith_meta {
+                // - adds a certification in receiver received list
                 smith_meta.received_certs.push(issuer);
                 smith_meta.received_certs.sort();
+                Self::deposit_event(Event::<T>::SmithCertAdded { receiver, issuer });
+
+                // - receiving a certification either lead us to Pending or Smith status
+                let previous_status = smith_meta.status;
                 smith_meta.status =
                     if smith_meta.received_certs.len() >= T::MinCertForMembership::get() as usize {
+                        // - if the number of certification received by the receiver is enough, win the Smith status (or keep it)
                         SmithStatus::Smith
                     } else {
+                        // - otherwise we are (still) a pending smith
                         SmithStatus::Pending
                     };
-                // expiry postponed
+
+                if previous_status != SmithStatus::Smith {
+                    // - postpone the expiration: a Pending smith cannot do anything but wait
+                    // this postponement is here to ease the process of becoming a smith
                     let new_expires_on =
                         CurrentSession::<T>::get() + T::SmithInactivityMaxDuration::get();
                     smith_meta.expires_on = Some(new_expires_on);
-                Self::deposit_event(Event::<T>::SmithCertAdded { receiver, issuer });
-                if smith_meta.status == SmithStatus::Smith {
+                    ExpiresOn::<T>::append(new_expires_on, receiver);
+                }
+
+                // - if the status is smith but wasn't, notify that smith gained membership
+                if smith_meta.status == SmithStatus::Smith && previous_status != SmithStatus::Smith
+                {
                     Self::deposit_event(Event::<T>::SmithMembershipAdded {
                         idty_index: receiver,
                     });
                 }
-                // TODO: unschedule old expiry
+                // TODO: (optimization) unschedule old expiry
             }
         });
     }

pallets/smith-members/src/tests.rs

@@ -187,6 +187,52 @@ fn process_to_become_a_smith_and_lose_it() {
     });
 }
 
+#[test]
+fn avoid_multiple_events_for_becoming_smith() {
+    new_test_ext(GenesisConfig {
+        initial_smiths: btreemap![
+            1 => (false, vec![2, 3, 4]),
+            2 => (false, vec![3, 4]),
+            3 => (false, vec![1, 2]),
+            4 => (false, vec![]),
+        ],
+    })
+    .execute_with(|| {
+        // Go online to be able to invite+certify
+        Pallet::<Runtime>::on_smith_goes_online(1);
+        Pallet::<Runtime>::on_smith_goes_online(2);
+        Pallet::<Runtime>::on_smith_goes_online(3);
+        // Events cannot be recorded on genesis
+        run_to_block(1);
+        // State before
+        assert_eq!(Smiths::<Runtime>::get(5), None);
+        // Try to invite
+        assert_ok!(Pallet::<Runtime>::invite_smith(RuntimeOrigin::signed(1), 5));
+        assert_ok!(Pallet::<Runtime>::accept_invitation(RuntimeOrigin::signed(
+            5
+        )));
+        assert_ok!(Pallet::<Runtime>::certify_smith(
+            RuntimeOrigin::signed(1),
+            5
+        ));
+        assert_ok!(Pallet::<Runtime>::certify_smith(
+            RuntimeOrigin::signed(2),
+            5
+        ));
+        System::assert_has_event(RuntimeEvent::Smith(
+            Event::<Runtime>::SmithMembershipAdded { idty_index: 5 },
+        ));
+        run_to_block(2);
+        assert_ok!(Pallet::<Runtime>::certify_smith(
+            RuntimeOrigin::signed(3),
+            5
+        ));
+        // Should not be promoted again
+        assert!(!System::events().iter().any(|record| record.event
+            == RuntimeEvent::Smith(Event::<Runtime>::SmithMembershipAdded { idty_index: 5 },)));
+    });
+}
+
 #[test]
 fn should_have_checks_on_certify() {
     new_test_ext(GenesisConfig {
@@ -510,6 +556,80 @@ fn certifying_on_different_status() {
     });
 }
 
+#[test]
+fn certifying_an_online_smith() {
+    new_test_ext(GenesisConfig {
+        initial_smiths: btreemap![
+            1 => (false, vec![2, 3, 4]),
+            2 => (false, vec![3, 4]),
+            3 => (false, vec![1, 2]),
+            4 => (false, vec![]),
+        ],
+    })
+    .execute_with(|| {
+        // Go online to be able to invite+certify
+        Pallet::<Runtime>::on_smith_goes_online(1);
+        Pallet::<Runtime>::on_smith_goes_online(2);
+        Pallet::<Runtime>::on_smith_goes_online(3);
+        assert_ok!(Pallet::<Runtime>::invite_smith(RuntimeOrigin::signed(1), 5));
+        assert_ok!(Pallet::<Runtime>::accept_invitation(RuntimeOrigin::signed(
+            5
+        )));
+        Pallet::<Runtime>::on_new_session(2);
+        assert_ok!(Pallet::<Runtime>::certify_smith(
+            RuntimeOrigin::signed(1),
+            5
+        ));
+        Pallet::<Runtime>::on_new_session(3);
+        assert_ok!(Pallet::<Runtime>::certify_smith(
+            RuntimeOrigin::signed(2),
+            5
+        ));
+        // Smith can expire
+        assert_eq!(
+            Smiths::<Runtime>::get(5),
+            Some(SmithMeta {
+                status: Smith,
+                expires_on: Some(8),
+                issued_certs: vec![],
+                received_certs: vec![1, 2]
+            })
+        );
+        assert_eq!(ExpiresOn::<Runtime>::get(7), Some(vec![5]));
+        assert_eq!(ExpiresOn::<Runtime>::get(8), Some(vec![5]));
+
+        Pallet::<Runtime>::on_smith_goes_online(5);
+        // After going online, the expiration disappears
+        assert_eq!(
+            Smiths::<Runtime>::get(5),
+            Some(SmithMeta {
+                status: Smith,
+                expires_on: None,
+                issued_certs: vec![],
+                received_certs: vec![1, 2]
+            })
+        );
+        // ExpiresOn is not unscheduled, but as expires_on has switched to None it's not a problem
+        assert_eq!(ExpiresOn::<Runtime>::get(7), Some(vec![5]));
+        assert_eq!(ExpiresOn::<Runtime>::get(8), Some(vec![5]));
+
+        // We can receive certification without postponing the expiration (because we are online)
+        assert_ok!(Pallet::<Runtime>::certify_smith(
+            RuntimeOrigin::signed(3),
+            5
+        ));
+        assert_eq!(
+            Smiths::<Runtime>::get(5),
+            Some(SmithMeta {
+                status: Smith,
+                expires_on: None,
+                issued_certs: vec![],
+                received_certs: vec![1, 2, 3]
+            })
+        );
+    });
+}
+
 #[test]
 fn invitation_on_non_wot_member() {
     new_test_ext(GenesisConfig {