From d283ac7b4dd7106cacc3080a2e1762fb1e3a90a1 Mon Sep 17 00:00:00 2001 From: bgallois <benjamin@gallois.cc> Date: Tue, 5 Dec 2023 14:27:16 +0100 Subject: [PATCH] refactor expiration and revokation pallet-membership --- pallets/duniter-wot/src/lib.rs | 9 +------ pallets/duniter-wot/src/tests.rs | 31 ++++++++++++++++----- pallets/membership/src/benchmarking.rs | 2 +- pallets/membership/src/lib.rs | 29 ++++++++++++++------ pallets/membership/src/tests.rs | 13 ++++++--- primitives/membership/src/lib.rs | 6 ++--- runtime/common/src/handlers.rs | 5 ++-- runtime/gdev/tests/fixme_tests.rs | 6 ++++- runtime/gdev/tests/integration_tests.rs | 36 ++++++++++++++++++++----- 9 files changed, 95 insertions(+), 42 deletions(-) diff --git a/pallets/duniter-wot/src/lib.rs b/pallets/duniter-wot/src/lib.rs index 946fa6686..8a9b2822a 100644 --- a/pallets/duniter-wot/src/lib.rs +++ b/pallets/duniter-wot/src/lib.rs @@ -335,14 +335,7 @@ where pallet_identity::Pallet::<T>::try_validate_identity(*idty_index); } } - sp_membership::Event::<IdtyIndex>::MembershipExpired(_) => {} - // Membership revocation cases: - // - Triggered by main identity removal: the underlying identity will be removed by the - // caller. - // - Triggered by the membership pallet: it's only possible for a sub-wot, so we - // should not remove the underlying identity - // So, in any case, we must do nothing - sp_membership::Event::<IdtyIndex>::MembershipRevoked(_) => {} + sp_membership::Event::<IdtyIndex>::MembershipTerminated(_) => {} sp_membership::Event::<IdtyIndex>::MembershipRenewed(_) => {} sp_membership::Event::<IdtyIndex>::MembershipRequested(_) => {} sp_membership::Event::<IdtyIndex>::PendingMembershipExpired(idty_index) => { diff --git a/pallets/duniter-wot/src/tests.rs b/pallets/duniter-wot/src/tests.rs index 9356a6880..14b50845d 100644 --- a/pallets/duniter-wot/src/tests.rs +++ b/pallets/duniter-wot/src/tests.rs @@ -24,6 +24,7 @@ use pallet_identity::{ IdtyIndexAccountIdPayload, IdtyName, IdtyStatus, RevocationPayload, NEW_OWNER_KEY_PAYLOAD_PREFIX, REVOCATION_PAYLOAD_PREFIX, }; +use pallet_membership::MembershipTerminationReason; use sp_runtime::testing::TestSignature; /// test that genesis builder creates the good number of identities @@ -110,7 +111,10 @@ fn test_smith_certs_expirations_should_expire_smith_membership() { // After block #10, alice membership should be revoked due to smith certs expiration run_to_block(10); System::assert_has_event(RuntimeEvent::SmithMembership( - pallet_membership::Event::MembershipExpired { member: 1 }, + pallet_membership::Event::MembershipTerminated { + member: 1, + reason: MembershipTerminationReason::Expired, + }, )); }); } @@ -349,7 +353,10 @@ fn test_idty_membership_expire() { assert!(Membership::membership(3).is_none()); System::assert_has_event(RuntimeEvent::Membership( - pallet_membership::Event::MembershipExpired { member: 3 }, + pallet_membership::Event::MembershipTerminated { + member: 3, + reason: MembershipTerminationReason::Expired, + }, )); // membership expiry should not trigger identity removal assert!(!System::events().iter().any(|record| record.event @@ -453,7 +460,10 @@ fn test_certification_expire() { )); // 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 { member: 1 }, + pallet_membership::Event::MembershipTerminated { + member: 1, + reason: MembershipTerminationReason::Expired, + }, )); // --- BLOCK 14 --- @@ -476,7 +486,10 @@ fn test_certification_expire() { )); // in consequence, since Alice has only 1/2 normal certification remaining, she looses normal membership System::assert_has_event(RuntimeEvent::Membership( - pallet_membership::Event::MembershipExpired { member: 1 }, + pallet_membership::Event::MembershipTerminated { + member: 1, + reason: MembershipTerminationReason::Expired, + }, )); // --- BLOCK 21 --- @@ -565,7 +578,10 @@ fn test_cert_can_not_be_issued() { )); // 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 { member: 1 }, + pallet_membership::Event::MembershipTerminated { + member: 1, + reason: MembershipTerminationReason::Expired, + }, )); run_to_block(11); @@ -613,7 +629,10 @@ fn test_cert_can_not_be_issued() { // 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 { member: 1 }, // pending membership expires at 23 + pallet_membership::Event::MembershipTerminated { + member: 1, + reason: MembershipTerminationReason::Expired, + }, // pending membership expires at 23 )); run_to_block(21); diff --git a/pallets/membership/src/benchmarking.rs b/pallets/membership/src/benchmarking.rs index 20dc89944..ccc88cf3c 100644 --- a/pallets/membership/src/benchmarking.rs +++ b/pallets/membership/src/benchmarking.rs @@ -82,7 +82,7 @@ benchmarks_instance_pallet! { let caller_origin: <T as frame_system::Config>::RuntimeOrigin = RawOrigin::Signed(caller.clone()).into(); }: _<T::RuntimeOrigin>(caller_origin) verify { - assert_has_event::<T, I>(Event::<T, I>::MembershipRevoked{member: idty}.into()); + assert_has_event::<T, I>(Event::<T, I>::MembershipTerminated{member: idty, reason: MembershipTerminationReason::Revoked}.into()); } // Base weight of an empty initialize on_initialize { diff --git a/pallets/membership/src/lib.rs b/pallets/membership/src/lib.rs index 63d69b312..bdb9f07b3 100644 --- a/pallets/membership/src/lib.rs +++ b/pallets/membership/src/lib.rs @@ -54,6 +54,12 @@ impl<IdtyId, AccountId> SetupBenchmark<IdtyId, AccountId> for () { fn add_cert(_issuer: &IdtyId, _receiver: &IdtyId) -> () {} } +#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo)] +pub enum MembershipTerminationReason { + Expired, + Revoked, +} + #[frame_support::pallet] pub mod pallet { use super::*; @@ -157,14 +163,15 @@ pub mod pallet { pub enum Event<T: Config<I>, I: 'static = ()> { /// A membership was acquired. MembershipAcquired { member: T::IdtyId }, - /// A membership expired. - MembershipExpired { member: T::IdtyId }, + /// A membership was terminated. + MembershipTerminated { + member: T::IdtyId, + reason: MembershipTerminationReason, + }, /// A membership was renewed. MembershipRenewed { member: T::IdtyId }, /// A membership was requested. MembershipRequested { member: T::IdtyId }, - /// A membership was revoked. - MembershipRevoked { member: T::IdtyId }, /// A pending membership request has expired. PendingMembershipExpired { member: T::IdtyId }, } @@ -362,8 +369,11 @@ pub mod pallet { fn do_revoke_membership(idty_id: T::IdtyId) { if let Some(membership_data) = Membership::<T, I>::take(idty_id) { Self::unschedule_membership_expiry(idty_id, membership_data.expire_on); - Self::deposit_event(Event::MembershipRevoked { member: idty_id }); - T::OnEvent::on_event(&sp_membership::Event::MembershipRevoked(idty_id)); + Self::deposit_event(Event::MembershipTerminated { + member: idty_id, + reason: MembershipTerminationReason::Revoked, + }); + T::OnEvent::on_event(&sp_membership::Event::MembershipTerminated(idty_id)); } } @@ -375,8 +385,11 @@ pub mod pallet { PendingMembershipsExpireOn::<T, I>::append(expire_on, idty_id); } // else should not happen - Self::deposit_event(Event::MembershipExpired { member: idty_id }); - T::OnEvent::on_event(&sp_membership::Event::MembershipExpired(idty_id)); + Self::deposit_event(Event::MembershipTerminated { + member: idty_id, + reason: MembershipTerminationReason::Expired, + }); + T::OnEvent::on_event(&sp_membership::Event::MembershipTerminated(idty_id)); } /// check the origin and get identity id if valid diff --git a/pallets/membership/src/tests.rs b/pallets/membership/src/tests.rs index 54bf1a406..fe969df97 100644 --- a/pallets/membership/src/tests.rs +++ b/pallets/membership/src/tests.rs @@ -15,6 +15,7 @@ // along with Duniter-v2S. If not, see <https://www.gnu.org/licenses/>. use crate::mock::*; +use crate::MembershipTerminationReason; use crate::{Error, Event}; use frame_support::{assert_noop, assert_ok}; use maplit::btreemap; @@ -83,8 +84,9 @@ fn test_membership_expiration() { // Membership 0 should expire on block #3 run_to_block(3); assert!(!DefaultMembership::is_member(&0)); - System::assert_has_event(RtEvent::DefaultMembership(Event::MembershipExpired { + System::assert_has_event(RtEvent::DefaultMembership(Event::MembershipTerminated { member: 0, + reason: MembershipTerminationReason::Expired, })); // it should be added to pending membership and expire on block #6 run_to_block(6); @@ -117,8 +119,9 @@ fn test_membership_renewal() { // membership should expire at block 7 (2+5) run_to_block(7); assert!(!DefaultMembership::is_member(&0)); - System::assert_has_event(RtEvent::DefaultMembership(Event::MembershipExpired { + System::assert_has_event(RtEvent::DefaultMembership(Event::MembershipTerminated { member: 0, + reason: MembershipTerminationReason::Expired, })); }); } @@ -148,8 +151,9 @@ fn test_membership_revocation() { assert_ok!(DefaultMembership::revoke_membership(RuntimeOrigin::signed( 0 ),)); - System::assert_has_event(RtEvent::DefaultMembership(Event::MembershipRevoked { + System::assert_has_event(RtEvent::DefaultMembership(Event::MembershipTerminated { member: 0, + reason: MembershipTerminationReason::Revoked, })); // Membership 0 can re-request membership @@ -228,8 +232,9 @@ fn test_membership_workflow() { // - Then, idty 0 should expire after membership period run_to_block(7); // 2 + 5 assert!(!DefaultMembership::is_member(&0)); - System::assert_has_event(RtEvent::DefaultMembership(Event::MembershipExpired { + System::assert_has_event(RtEvent::DefaultMembership(Event::MembershipTerminated { member: 0, + reason: MembershipTerminationReason::Expired, })); }); } diff --git a/primitives/membership/src/lib.rs b/primitives/membership/src/lib.rs index f59fcde5d..042c4835e 100644 --- a/primitives/membership/src/lib.rs +++ b/primitives/membership/src/lib.rs @@ -30,14 +30,12 @@ use serde::{Deserialize, Serialize}; pub enum Event<IdtyId> { /// A membership has acquired MembershipAcquired(IdtyId), - /// A membership has expired - MembershipExpired(IdtyId), + /// A membership was terminated. + MembershipTerminated(IdtyId), /// A membership has renewed MembershipRenewed(IdtyId), /// An identity requested membership MembershipRequested(IdtyId), - /// A membership has revoked - MembershipRevoked(IdtyId), /// A pending membership request has expired PendingMembershipExpired(IdtyId), } diff --git a/runtime/common/src/handlers.rs b/runtime/common/src/handlers.rs index a6eecb1c2..b91e07953 100644 --- a/runtime/common/src/handlers.rs +++ b/runtime/common/src/handlers.rs @@ -79,8 +79,7 @@ impl< fn on_event(membership_event: &sp_membership::Event<IdtyIndex>) { (match membership_event { // when membership is removed, call on_removed_member handler which auto claims UD - sp_membership::Event::MembershipRevoked(idty_index) - | sp_membership::Event::MembershipExpired(idty_index) => { + sp_membership::Event::MembershipTerminated(idty_index) => { if let Some(idty_value) = pallet_identity::Identities::<Runtime>::get(idty_index) { if let Some(first_ud_index) = idty_value.data.first_eligible_ud.into() { pallet_universal_dividend::Pallet::<Runtime>::on_removed_member( @@ -130,7 +129,7 @@ impl< // nothing when smith membership acquired // user will have to claim authority membership } - sp_membership::Event::MembershipRevoked(idty_index) => { + sp_membership::Event::MembershipTerminated(idty_index) => { let call = pallet_authority_members::Call::<Runtime>::remove_member { member_id: *idty_index, }; diff --git a/runtime/gdev/tests/fixme_tests.rs b/runtime/gdev/tests/fixme_tests.rs index 9ca62d127..db10c71c0 100644 --- a/runtime/gdev/tests/fixme_tests.rs +++ b/runtime/gdev/tests/fixme_tests.rs @@ -22,6 +22,7 @@ mod common; use common::*; use frame_support::assert_ok; use gdev_runtime::*; +use pallet_membership::MembershipTerminationReason; use sp_keyring::AccountKeyring; /// issue #136 @@ -56,7 +57,10 @@ fn can_still_issue_cert_when_membership_lost() { 2, // Bob )); System::assert_has_event(RuntimeEvent::Membership( - pallet_membership::Event::MembershipExpired { member: 2 }, + pallet_membership::Event::MembershipTerminated { + member: 2, + reason: MembershipTerminationReason::Expired, + }, )); // FIXME this should not be possible diff --git a/runtime/gdev/tests/integration_tests.rs b/runtime/gdev/tests/integration_tests.rs index 374902776..c4c53d658 100644 --- a/runtime/gdev/tests/integration_tests.rs +++ b/runtime/gdev/tests/integration_tests.rs @@ -24,6 +24,7 @@ use frame_support::{assert_noop, assert_ok}; use frame_support::{StorageHasher, Twox128}; use gdev_runtime::*; use pallet_duniter_wot::IdtyRemovalWotReason; +use pallet_membership::MembershipTerminationReason; use sp_core::Encode; use sp_keyring::AccountKeyring; use sp_runtime::MultiAddress; @@ -296,7 +297,10 @@ fn test_force_remove_identity() { pallet_identity::IdtyRemovalReason::Manual )); System::assert_has_event(RuntimeEvent::Membership( - pallet_membership::Event::MembershipRevoked { member: 4 }, + pallet_membership::Event::MembershipTerminated { + member: 4, + reason: MembershipTerminationReason::Revoked, + }, )); System::assert_has_event(RuntimeEvent::Identity( pallet_identity::Event::IdtyRemoved { @@ -386,7 +390,10 @@ fn test_membership_expiry() { ExtBuilder::new(1, 3, 4).build().execute_with(|| { run_to_block(100); System::assert_has_event(RuntimeEvent::Membership( - pallet_membership::Event::MembershipExpired { member: 1 }, + pallet_membership::Event::MembershipTerminated { + member: 1, + reason: MembershipTerminationReason::Expired, + }, )); // membership expiry should not trigger identity removal assert!(!System::events().iter().any(|record| record.event @@ -403,7 +410,10 @@ fn test_membership_expiry_with_identity_removal() { run_to_block(100); System::assert_has_event(RuntimeEvent::Membership( - pallet_membership::Event::MembershipExpired { member: 4 }, + pallet_membership::Event::MembershipTerminated { + member: 4, + reason: MembershipTerminationReason::Expired, + }, )); // Trigger pending membership expiry @@ -463,7 +473,10 @@ fn test_membership_renewal() { // should expire at block 177 = 77+100 run_to_block(177); System::assert_has_event(RuntimeEvent::Membership( - pallet_membership::Event::MembershipExpired { member: 1 }, + pallet_membership::Event::MembershipTerminated { + member: 1, + reason: MembershipTerminationReason::Expired, + }, )); }); } @@ -516,7 +529,10 @@ fn test_remove_identity_after_one_ud() { })); // membership and identity were actually removed System::assert_has_event(RuntimeEvent::Membership( - pallet_membership::Event::MembershipRevoked { member: 4 }, + pallet_membership::Event::MembershipTerminated { + member: 4, + reason: MembershipTerminationReason::Revoked, + }, )); System::assert_has_event(RuntimeEvent::Identity( pallet_identity::Event::IdtyRemoved { @@ -644,13 +660,19 @@ fn test_remove_smith_identity() { )); // Verify events System::assert_has_event(RuntimeEvent::SmithMembership( - pallet_membership::Event::MembershipRevoked { member: 3 }, + pallet_membership::Event::MembershipTerminated { + member: 3, + reason: MembershipTerminationReason::Revoked, + }, )); System::assert_has_event(RuntimeEvent::AuthorityMembers( pallet_authority_members::Event::MemberRemoved { member: 3 }, )); System::assert_has_event(RuntimeEvent::Membership( - pallet_membership::Event::MembershipRevoked { member: 3 }, + pallet_membership::Event::MembershipTerminated { + member: 3, + reason: MembershipTerminationReason::Revoked, + }, )); System::assert_has_event(RuntimeEvent::Identity( pallet_identity::Event::IdtyRemoved { -- GitLab