Resolve "smithMembers.expiresOn is only growing" (!305)

* fix bug from #276

* reveal bug from #276

* fix an uncaught bug

pallets/smith-members/src/lib.rs

@@ -240,10 +240,13 @@ pub mod pallet {
                         received_certs: issuers_,
                     },
                 );
-                ExpiresOn::<T>::append(
-                    CurrentSession::<T>::get() + T::SmithInactivityMaxDuration::get(),
-                    receiver,
-                );
+                // if smith is offline, schedule expire
+                if !*is_online {
+                    ExpiresOn::<T>::append(
+                        CurrentSession::<T>::get() + T::SmithInactivityMaxDuration::get(),
+                        receiver,
+                    );
+                }
             }
 
             for (issuer, issued_certs) in cert_meta_by_issuer {
@@ -516,7 +519,7 @@ impl<T: Config> Pallet<T> {
 
     /// Handle the removal of Smiths whose expiration time has been reached at a given session index.
     fn on_exclude_expired_smiths(at: SessionIndex) {
-        if let Some(smiths_to_remove) = ExpiresOn::<T>::get(at) {
+        if let Some(smiths_to_remove) = ExpiresOn::<T>::take(at) {
             for smith in smiths_to_remove {
                 if let Some(smith_meta) = Smiths::<T>::get(smith) {
                     if let Some(expires_on) = smith_meta.expires_on {

pallets/smith-members/src/tests.rs

@@ -628,6 +628,48 @@ fn certifying_an_online_smith() {
     });
 }
 
+/// Test that scheduled expiration is removed after session
+#[test]
+fn expires_on_cleans_up() {
+    new_test_ext(GenesisConfig {
+        initial_smiths: btreemap![
+            1 => (true, vec![2, 3]),
+            2 => (true, vec![1,3]),
+            3 => (true, vec![1,2]),
+        ],
+    })
+    .execute_with(|| {
+        // Alice goes offline, and is set to expire
+        Pallet::<Runtime>::on_smith_goes_offline(1);
+        // The expiration block is present in smith data
+        assert_eq!(
+            Smiths::<Runtime>::get(1),
+            Some(SmithMeta {
+                status: Smith,
+                expires_on: Some(5),
+                issued_certs: vec![2, 3],
+                received_certs: vec![2, 3]
+            })
+        );
+        // It is also present in ExpiresOn schedule
+        assert_eq!(ExpiresOn::<Runtime>::get(5), Some(vec![1]));
+        // Go to expiration session
+        Pallet::<Runtime>::on_new_session(5);
+        // Alice is expired
+        assert_eq!(
+            Smiths::<Runtime>::get(1),
+            Some(SmithMeta {
+                status: Excluded,
+                expires_on: None,
+                issued_certs: vec![2, 3],
+                received_certs: vec![]
+            })
+        );
+        // ExpiresOn is clean
+        assert_eq!(ExpiresOn::<Runtime>::get(5), None);
+    });
+}
+
 #[test]
 fn invitation_on_non_wot_member() {
     new_test_ext(GenesisConfig {