make membership claim no-op an error

pallets/distance/src/benchmarking.rs

@@ -75,7 +75,7 @@ benchmarks! {
             |idty_val| idty_val.as_mut().unwrap().status = pallet_identity::IdtyStatus::Unvalidated);
     }: _<T::RuntimeOrigin>(caller_origin.clone(), target)
     verify {
-        assert!(PendingEvaluationRequest::<T>::get(&target) == Some(caller.clone()), "Request not added");
+        assert!(PendingEvaluationRequest::<T>::get(target) == Some(caller.clone()), "Request not added");
         assert_has_event::<T>(Event::<T>::EvaluationRequested { idty_index: target, who: caller }.into());
     }
 

pallets/distance/src/lib.rs

@@ -464,6 +464,8 @@ pub mod pallet {
             Pallet::<T>::mutate_current_pool(
                 pallet_session::CurrentIndex::<T>::get(), // TODO look
                 |current_pool| {
+                    // TODO not needed if called in extrinsics only
+                    // since extrinsics are transactional by default
                     ensure!(
                         current_pool.evaluations.len() < (MAX_EVALUATIONS_PER_SESSION as usize),
                         Error::<T>::QueueFull

pallets/duniter-wot/src/lib.rs

@@ -366,23 +366,42 @@ impl<T: Config<I> + pallet_distance::Config, I: 'static>
     fn on_valid_distance_status(idty_index: IdtyIndex) {
         if let Some(identity) = pallet_identity::Identities::<T>::get(idty_index) {
             match identity.status {
-                IdtyStatus::Unconfirmed => { /* should not happen */ }
-                IdtyStatus::Unvalidated | IdtyStatus::NotMember => {
-                    // ok to fail
-                    let r = pallet_membership::Pallet::<T, I>::try_claim_membership(idty_index);
-                    sp_std::if_std! {
-                        if let Err(e) = r {
-                            print!("failed to claim identity when distance status was ok: ");
-                            println!("{:?}", idty_index);
-                            println!("reason: {:?}", e);
-                        }
+                IdtyStatus::Unconfirmed | IdtyStatus::Revoked => {
+                    // IdtyStatus::Unconfirmed
+                    // distance evaluation request should never happen for unconfirmed identity
+                    // IdtyStatus::Revoked
+                    // the identity can have been revoked during distance evaluation by the oracle
                 }
+
+                IdtyStatus::Unvalidated | IdtyStatus::NotMember => {
+                    // IdtyStatus::Unvalidated
+                    // normal scenario for first entry
+                    // IdtyStatus::NotMember
+                    // normal scenario for re-entry
+                    // the following can fail if a certification expired during distance evaluation
+                    // otherwise it should succeed
+                    let _ = pallet_membership::Pallet::<T, I>::try_claim_membership(idty_index);
+                    // sp_std::if_std! {
+                    //     if let Err(e) = r {
+                    //         print!("failed to claim identity when distance status was found ok: ");
+                    //         println!("{:?}", idty_index);
+                    //         println!("reason: {:?}", e);
+                    //     }
+                    // }
                 }
                 IdtyStatus::Member => {
-                    // ok to fail
+                    // IdtyStatus::Member
+                    // normal scenario for renewal
+                    // should succeed
                     let _ = pallet_membership::Pallet::<T, I>::try_renew_membership(idty_index);
+                    // sp_std::if_std! {
+                    //     if let Err(e) = r {
+                    //         print!("failed to renew identity when distance status was found ok: ");
+                    //         println!("{:?}", idty_index);
+                    //         println!("reason: {:?}", e);
+                    //     }
+                    // }
                 }
-                IdtyStatus::Revoked => { /* should not happen */ }
             }
         } else {
             // identity was removed before distance status was found

pallets/membership/src/lib.rs

@@ -174,6 +174,8 @@ pub mod pallet {
         MembershipAlreadyAcquired,
         /// Membership not found.
         MembershipNotFound,
+        /// Already member, can not claim membership
+        AlreadyMember,
     }
 
     // HOOKS //
@@ -260,6 +262,12 @@ pub mod pallet {
 
         /// check that membership can be claimed
         pub fn check_allowed_to_claim(idty_id: T::IdtyId) -> Result<(), DispatchError> {
+            // no-op is error
+            ensure!(
+                Membership::<T, I>::get(idty_id).is_none(),
+                Error::<T, I>::AlreadyMember
+            );
+
             // enough certifications and distance rule for example
             T::CheckMembershipCallAllowed::check_idty_allowed_to_claim_membership(&idty_id)?;
             Ok(())

runtime/gdev/tests/integration_tests.rs

@@ -310,7 +310,7 @@ fn test_remove_identity() {
     });
 }
 
-/// test identity is validated when membership is claimed
+/// test identity is "validated" (= membership is claimed) when distance is evaluated positively
 #[test]
 fn test_validate_identity_when_claim() {
     ExtBuilder::new(1, 3, 4)
@@ -345,11 +345,14 @@ fn test_validate_identity_when_claim() {
                 5
             ));
 
+            // eve request distance evaluation for herself
             assert_ok!(Distance::request_distance_evaluation(
                 frame_system::RawOrigin::Signed(AccountKeyring::Eve.to_account_id()).into(),
             ));
 
-            run_to_block(51); // Pass 2 sessions
+            // Pass 2 sessions
+            run_to_block(51);
+            // simulate an evaluation published by smith Alice
             assert_ok!(Distance::force_update_evaluation(
                 frame_system::RawOrigin::Root.into(),
                 AccountKeyring::Alice.to_account_id(),
@@ -357,22 +360,28 @@ fn test_validate_identity_when_claim() {
                     distances: vec![Perbill::one()],
                 }
             ));
-            run_to_block(76); // Pass 1 session
+            run_to_block(75); // Pass 1 session
+            System::assert_has_event(RuntimeEvent::Distance(
+                pallet_distance::Event::EvaluatedValid { idty_index: 5 },
+            ));
 
-            // eve can claim her membership
-            assert_ok!(Membership::claim_membership(
+            // eve can not claim her membership manually because it is done automatically
+            assert_noop!(
+                Membership::claim_membership(
                     frame_system::RawOrigin::Signed(AccountKeyring::Eve.to_account_id()).into(),
-            ));
+                ),
+                pallet_membership::Error::<Runtime, pallet_membership::Instance1>::AlreadyMember
+            );
 
+            // println!("{:?}", System::events());
             System::assert_has_event(RuntimeEvent::Membership(
                 pallet_membership::Event::MembershipAdded {
                     member: 5,
-                    expire_on: 76
+                    expire_on: 75
                         + <Runtime as pallet_membership::Config<Instance1>>::MembershipPeriod::get(
                         ),
                 },
             ));
-            // not possible anymore to validate identity of someone else
         });
 }
 
@@ -677,14 +686,18 @@ fn test_ud_claimed_membership_on_and_off() {
             },
         ));
 
-        // alice claims back her membership
+        // alice claims back her membership through distance evaluation
         assert_ok!(Distance::force_valid_distance_status(
             frame_system::RawOrigin::Root.into(),
             1,
         ));
-        assert_ok!(Membership::claim_membership(
-            frame_system::RawOrigin::Signed(AccountKeyring::Alice.to_account_id()).into()
-        ));
+        // it can not be done manually
+        assert_noop!(
+            Membership::claim_membership(
+                frame_system::RawOrigin::Signed(AccountKeyring::Alice.to_account_id()).into(),
+            ),
+            pallet_membership::Error::<Runtime, pallet_membership::Instance1>::AlreadyMember
+        );
         System::assert_has_event(RuntimeEvent::Membership(
             pallet_membership::Event::MembershipAdded {
                 member: 1,
@@ -1119,20 +1132,25 @@ fn test_validate_new_idty_after_few_uds() {
                 pallet_identity::IdtyName::from("Eve"),
             ));
 
-            // At next block, Bob should be able to certify and validate the new identity
+            // At next block, Bob should be able to certify the new identity
             run_to_block(23);
             assert_ok!(Cert::add_cert(
                 frame_system::RawOrigin::Signed(AccountKeyring::Bob.to_account_id()).into(),
                 2,
                 5,
             ));
+            // valid distance status should trigger identity validation
             assert_ok!(Distance::force_valid_distance_status(
                 frame_system::RawOrigin::Root.into(),
                 5,
             ));
-            assert_ok!(Membership::claim_membership(
+            // and it is not possible to call it manually
+            assert_noop!(
+                Membership::claim_membership(
                     frame_system::RawOrigin::Signed(AccountKeyring::Eve.to_account_id()).into(),
-            ));
+                ),
+                pallet_membership::Error::<Runtime, pallet_membership::Instance1>::AlreadyMember
+            );
 
             // The new member should have first_eligible_ud equal to three
             assert!(Identity::identity(5).is_some());
@@ -1181,14 +1199,18 @@ fn test_claim_memberhsip_after_few_uds() {
                 5,
             ));
 
-            // eve should be able to claim her membership
+            // eve membership should be able to be claimed through distance evaluation
             assert_ok!(Distance::force_valid_distance_status(
                 frame_system::RawOrigin::Root.into(),
                 5,
             ));
-            assert_ok!(Membership::claim_membership(
+            // but not manually
+            assert_noop!(
+                Membership::claim_membership(
                     frame_system::RawOrigin::Signed(AccountKeyring::Eve.to_account_id()).into(),
-            ));
+                ),
+                pallet_membership::Error::<Runtime, pallet_membership::Instance1>::AlreadyMember
+            );
 
             // The new member should have first_eligible_ud equal to three
             assert!(Identity::identity(5).is_some());