From cbb011aba0d3b8d41e60c50d66420c4f30cc5d87 Mon Sep 17 00:00:00 2001
From: Hugo Trentesaux <hugo@trentesaux.fr>
Date: Thu, 11 Jan 2024 11:52:20 +0100
Subject: [PATCH] make membership claim no-op an error
---
pallets/distance/src/benchmarking.rs | 2 +-
pallets/distance/src/lib.rs | 2 +
pallets/duniter-wot/src/lib.rs | 43 ++++++++++++-----
pallets/membership/src/lib.rs | 8 ++++
runtime/gdev/tests/integration_tests.rs | 64 +++++++++++++++++--------
5 files changed, 85 insertions(+), 34 deletions(-)
diff --git a/pallets/distance/src/benchmarking.rs b/pallets/distance/src/benchmarking.rs
index cb7283931..c389b354c 100644
--- a/pallets/distance/src/benchmarking.rs
+++ b/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());
}
diff --git a/pallets/distance/src/lib.rs b/pallets/distance/src/lib.rs
index 8f0e7df82..3a0f01ff4 100644
--- a/pallets/distance/src/lib.rs
+++ b/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
diff --git a/pallets/duniter-wot/src/lib.rs b/pallets/duniter-wot/src/lib.rs
index 0f6bed36a..b0264bc1f 100644
--- a/pallets/duniter-wot/src/lib.rs
+++ b/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::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 => {
- // 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::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
diff --git a/pallets/membership/src/lib.rs b/pallets/membership/src/lib.rs
index 99f9a317a..c098d4bb5 100644
--- a/pallets/membership/src/lib.rs
+++ b/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(())
diff --git a/runtime/gdev/tests/integration_tests.rs b/runtime/gdev/tests/integration_tests.rs
index 177efe07d..573b249f8 100644
--- a/runtime/gdev/tests/integration_tests.rs
+++ b/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
-
- // eve can claim her membership
- assert_ok!(Membership::claim_membership(
- frame_system::RawOrigin::Signed(AccountKeyring::Eve.to_account_id()).into(),
+ run_to_block(75); // Pass 1 session
+ System::assert_has_event(RuntimeEvent::Distance(
+ pallet_distance::Event::EvaluatedValid { idty_index: 5 },
));
+ // 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(
- frame_system::RawOrigin::Signed(AccountKeyring::Eve.to_account_id()).into(),
- ));
+ // 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(
- frame_system::RawOrigin::Signed(AccountKeyring::Eve.to_account_id()).into(),
- ));
+ // 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());
--
GitLab