reveal certification counterintuitive behavior (!163)

* add test in duniter-wot

* reveal smith certification quirck

smith certification do not require smith membership request

pallets/duniter-wot/src/lib.rs

@@ -208,7 +208,22 @@ where
 impl<T: Config<I>, I: 'static> pallet_certification::traits::CheckCertAllowed<IdtyIndex>
     for Pallet<T, I>
 {
+    // check the following:
+    // - issuer has identity
+    // - issuer identity is validated
+    // - receiver has identity
+    // - receiver identity is confirmed or validated
+    //
+    // /!\ do not check the following:
+    // - receiver has membership
+    // - issuer has membership
+    // this has the following consequences:
+    // - receiver can receive smith certification without having requested membership
+    // - issuer can issue cert even if he lost his membership
+    //   (not renewed or passed below cert threshold and above again without claiming membership)
+    // this is counterintuitive behavior but not a big problem
     fn check_cert_allowed(issuer: IdtyIndex, receiver: IdtyIndex) -> Result<(), DispatchError> {
+        // ensure issuer has validated identity
         if let Some(issuer_data) = pallet_identity::Pallet::<T>::identity(issuer) {
             ensure!(
                 issuer_data.status == IdtyStatus::Validated,
@@ -217,6 +232,7 @@ impl<T: Config<I>, I: 'static> pallet_certification::traits::CheckCertAllowed<Id
         } else {
             return Err(Error::<T, I>::IdtyNotFound.into());
         }
+        // ensure receiver has confirmed or validated identity
         if let Some(receiver_data) = pallet_identity::Pallet::<T>::identity(receiver) {
             match receiver_data.status {
                 IdtyStatus::ConfirmedByOwner | IdtyStatus::Validated => {} // able to receive cert

pallets/duniter-wot/src/tests.rs

@@ -516,3 +516,118 @@ fn test_certification_expire() {
         ));
     })
 }
+
+/// test some cases where identity should not be able to issue cert
+// - when source or target is not member (sub wot)
+// - when source or target membership is pending (both wot)
+#[test]
+fn test_cert_can_not_be_issued() {
+    new_test_ext(4, 3).execute_with(|| {
+        // smith cert Bob → Alice not renewed
+        // cert Bob → Alice not renewed
+        // --- BLOCK 2 ---
+        run_to_block(2);
+        assert_ok!(SmithCert::add_cert(RuntimeOrigin::signed(1), 1, 2)); // +10
+        assert_ok!(SmithCert::add_cert(RuntimeOrigin::signed(2), 2, 3)); // +10
+        assert_ok!(SmithCert::add_cert(RuntimeOrigin::signed(3), 3, 1)); // +10
+        assert_ok!(Cert::add_cert(RuntimeOrigin::signed(1), 1, 2)); // +20
+        assert_ok!(Cert::add_cert(RuntimeOrigin::signed(2), 2, 3)); // +20
+        assert_ok!(Cert::add_cert(RuntimeOrigin::signed(3), 3, 4)); // +20
+        assert_ok!(Cert::add_cert(RuntimeOrigin::signed(4), 4, 1)); // +20
+                                                                    // --- BLOCK 4 ---
+        run_to_block(4);
+        assert_ok!(SmithCert::add_cert(RuntimeOrigin::signed(1), 1, 3)); // +10
+        assert_ok!(SmithCert::add_cert(RuntimeOrigin::signed(3), 3, 2)); // +10
+        assert_ok!(Cert::add_cert(RuntimeOrigin::signed(2), 2, 4)); // +20
+        assert_ok!(Cert::add_cert(RuntimeOrigin::signed(3), 3, 2)); // +20
+        assert_ok!(Cert::add_cert(RuntimeOrigin::signed(4), 4, 3)); // +20
+                                                                    // --- BLOCK 7 ---
+        run_to_block(7);
+        assert_ok!(SmithCert::add_cert(RuntimeOrigin::signed(3), 3, 1)); // +10
+        assert_ok!(SmithMembership::renew_membership(RuntimeOrigin::signed(1))); // +20
+        assert_ok!(SmithMembership::renew_membership(RuntimeOrigin::signed(2))); // +20
+        assert_ok!(SmithMembership::renew_membership(RuntimeOrigin::signed(3))); // +20
+        assert_ok!(Membership::renew_membership(RuntimeOrigin::signed(1))); // + 8
+        assert_ok!(Membership::renew_membership(RuntimeOrigin::signed(2))); // + 8
+        assert_ok!(Membership::renew_membership(RuntimeOrigin::signed(3))); // + 8
+        assert_ok!(Membership::renew_membership(RuntimeOrigin::signed(4))); // + 8
+
+        // smith cert Bob → Alice expires at block 10
+        run_to_block(10);
+        // println!("{:?}", System::events());
+        System::assert_has_event(RuntimeEvent::SmithCert(
+            pallet_certification::Event::RemovedCert {
+                issuer: 2,                  // Bob
+                issuer_issued_count: 1,     // Bob → Charlie only
+                receiver: 1,                // Alice
+                receiver_received_count: 1, // Charlie → Alice only
+                expiration: true,
+            },
+        ));
+        // in consequence, since Alice has only 1/2 smith certification remaining, she looses smith membership
+        System::assert_has_event(RuntimeEvent::SmithMembership(
+            pallet_membership::Event::MembershipExpired(1),
+        ));
+
+        run_to_block(11);
+        // /!\ COUNTERINTUITIVE BEHAVIOR
+        // Dave should not be able to receive a smith cert since he did not request smith membership
+        assert_ok!(SmithCert::add_cert(RuntimeOrigin::signed(3), 3, 4));
+        // Bob renews his smith certification towards Alice
+        assert_ok!(SmithCert::add_cert(RuntimeOrigin::signed(2), 2, 1));
+        // now Alice has enough certs
+        assert_eq!(
+            SmithCert::idty_cert_meta(1),
+            pallet_certification::IdtyCertMeta {
+                issued_count: 2, // → Bob, → Charlie
+                // since Alice got back to min number of received certs to be able to issue cert
+                // her next_issuable_on was updated to block_number (11) + cert_period (2)
+                next_issuable_on: 13, // 11 + 2
+                received_count: 2     // ← Bob, ← Charlie
+            }
+        );
+
+        // because Alice did not claim membership, she is not member at this point
+        assert_eq!(SmithMembership::membership(1), None);
+        run_to_block(13);
+        // /!\ COUNTERINTUITIVE BEHAVIOR
+        // Alice is not smith member, she should not be able to issue cert
+        assert_ok!(SmithCert::add_cert(RuntimeOrigin::signed(1), 1, 2));
+
+        run_to_block(14);
+        assert_ok!(Membership::renew_membership(RuntimeOrigin::signed(1))); // + 8
+        assert_ok!(Membership::renew_membership(RuntimeOrigin::signed(2))); // + 8
+        assert_ok!(Membership::renew_membership(RuntimeOrigin::signed(3))); // + 8
+        assert_ok!(Membership::renew_membership(RuntimeOrigin::signed(4))); // + 8
+
+        run_to_block(20);
+        // println!("{:?}", System::events());
+        System::assert_has_event(RuntimeEvent::Cert(
+            pallet_certification::Event::RemovedCert {
+                issuer: 2,                  // Bob
+                issuer_issued_count: 2,     // depends of the order of cert expiration
+                receiver: 1,                // Alice
+                receiver_received_count: 2, // depends of the order of cert expiration
+                expiration: true,
+            },
+        ));
+        // other certifications expire, but not Dave → Alice
+        // in consequence, since Alice has only 1/2 certification remaining, she looses membership
+        System::assert_has_event(RuntimeEvent::Membership(
+            pallet_membership::Event::MembershipExpired(1), // pending membership expires at 23
+        ));
+
+        run_to_block(21);
+        // println!("{:?}", System::events());
+        // Charlie certifies Alice so she again has enough certs
+        assert_ok!(Cert::add_cert(RuntimeOrigin::signed(3), 3, 1));
+        assert_ok!(Cert::add_cert(RuntimeOrigin::signed(4), 4, 1)); // renew
+                                                                    // Alice did not claim membership, she is not member
+                                                                    // but her cert delay has been reset (→ 23)
+        assert_eq!(Membership::membership(1), None);
+
+        // run_to_block(23);
+        // if identity of alice was not removed because pending for too long
+        // she would have been able to emit a cert without being member
+    })
+}

runtime/gdev/tests/integration_tests.rs

@@ -632,6 +632,42 @@ fn test_remove_smith_identity() {
     });
 }
 
+#[test]
+fn test_smith_certification() {
+    // 3 smith (1. alice, 2. bob, 3. charlie)
+    // 4 identities (4. dave)
+    // no identity 5. eve
+    ExtBuilder::new(1, 3, 4).build().execute_with(|| {
+        run_to_block(1);
+
+        // alice can renew smith cert to bob
+        assert_ok!(SmithCert::add_cert(
+            frame_system::RawOrigin::Signed(AccountKeyring::Alice.to_account_id()).into(),
+            1, // alice
+            2  // bob
+        ));
+
+        // THIS IS STRANGE BEHAVIOR
+        // bob can add new smith cert to to dave even he did not requested smith membership
+        assert_ok!(SmithCert::add_cert(
+            frame_system::RawOrigin::Signed(AccountKeyring::Bob.to_account_id()).into(),
+            2, // bob
+            4  // dave
+        ));
+
+        // charlie can not add new cert to eve (no identity)
+        assert_noop!(
+            SmithCert::add_cert(
+                frame_system::RawOrigin::Signed(AccountKeyring::Charlie.to_account_id()).into(),
+                3, // charlie
+                5  // eve
+            ),
+            // SmithSubWot::Error::IdtyNotFound,
+            pallet_duniter_wot::Error::<gdev_runtime::Runtime, pallet_certification::Instance2>::IdtyNotFound,
+        );
+    });
+}
+
 /// test create new account with balance lower than existential deposit
 // the treasury gets the dust
 #[test]